[PATCH] Change the behavior of predicate check failure on cbranchcc4 operand0 in prepare_cmp_insn
Hi, I want to enable "have_cbranchcc4" on rs6000. But not all combinations of comparison codes and sub CC modes are benefited to generate cbranchcc4 insns on rs6000. There is an predicate for operand0 of cbranchcc4 to bypass some combinations. It gets assertion failure in prepare_cmp_insn. I think we shouldn't suppose that all comparison codes and sub CC modes are supported and throw an assertion failure in prepare_cmp_insn. It might check the predicate and go to fail if the predicate can't be satisfied. This patch changes the behavior of those codes. Bootstrapped and tested on powerpc64-linux BE/LE and x86 with no regressions. Is this okay for trunk? Any recommendations? Thanks a lot. ChangeLog 2022-11-23 Haochen Gui gcc/ * optabs.cc (prepare_cmp_insn): Go to fail other than assert it when predicate check of "cbranchcc4" operand[0] fails. patch.diff diff --git a/gcc/optabs.cc b/gcc/optabs.cc index 165f8d1fa22..3ec8f6b17ba 100644 --- a/gcc/optabs.cc +++ b/gcc/optabs.cc @@ -4484,8 +4484,9 @@ prepare_cmp_insn (rtx x, rtx y, enum rtx_code comparison, rtx size, { enum insn_code icode = optab_handler (cbranch_optab, CCmode); test = gen_rtx_fmt_ee (comparison, VOIDmode, x, y); - gcc_assert (icode != CODE_FOR_nothing - && insn_operand_matches (icode, 0, test)); + gcc_assert (icode != CODE_FOR_nothing); + if (!insn_operand_matches (icode, 0, test)) + goto fail; *ptest = test; return; }
Ping [PATCH] Change the behavior of predicate check failure on cbranchcc4 operand0 in prepare_cmp_insn
Hi, Gentle ping this: https://gcc.gnu.org/pipermail/gcc-patches/2022-November/607083.html Thanks Gui Haochen 在 2022/11/23 10:54, HAO CHEN GUI 写道: > Hi, > I want to enable "have_cbranchcc4" on rs6000. But not all combinations of > comparison codes and sub CC modes are benefited to generate cbranchcc4 insns > on rs6000. There is an predicate for operand0 of cbranchcc4 to bypass > some combinations. It gets assertion failure in prepare_cmp_insn. I think > we shouldn't suppose that all comparison codes and sub CC modes are supported > and throw an assertion failure in prepare_cmp_insn. It might check the > predicate and go to fail if the predicate can't be satisfied. This patch > changes the behavior of those codes. > > Bootstrapped and tested on powerpc64-linux BE/LE and x86 with no > regressions. > Is this okay for trunk? Any recommendations? Thanks a lot. > > > ChangeLog > 2022-11-23 Haochen Gui > > gcc/ > * optabs.cc (prepare_cmp_insn): Go to fail other than assert it when > predicate check of "cbranchcc4" operand[0] fails. > > patch.diff > diff --git a/gcc/optabs.cc b/gcc/optabs.cc > index 165f8d1fa22..3ec8f6b17ba 100644 > --- a/gcc/optabs.cc > +++ b/gcc/optabs.cc > @@ -4484,8 +4484,9 @@ prepare_cmp_insn (rtx x, rtx y, enum rtx_code > comparison, rtx size, > { >enum insn_code icode = optab_handler (cbranch_optab, CCmode); >test = gen_rtx_fmt_ee (comparison, VOIDmode, x, y); > - gcc_assert (icode != CODE_FOR_nothing > - && insn_operand_matches (icode, 0, test)); > + gcc_assert (icode != CODE_FOR_nothing); > + if (!insn_operand_matches (icode, 0, test)) > + goto fail; >*ptest = test; >return; > }
Re: [PATCH] Change the behavior of predicate check failure on cbranchcc4 operand0 in prepare_cmp_insn
Add more experts in CC. on 2022/11/23 10:54, HAO CHEN GUI wrote: > Hi, > I want to enable "have_cbranchcc4" on rs6000. But not all combinations of > comparison codes and sub CC modes are benefited to generate cbranchcc4 insns > on rs6000. There is an predicate for operand0 of cbranchcc4 to bypass > some combinations. It gets assertion failure in prepare_cmp_insn. I think > we shouldn't suppose that all comparison codes and sub CC modes are supported > and throw an assertion failure in prepare_cmp_insn. It might check the > predicate and go to fail if the predicate can't be satisfied. This patch > changes the behavior of those codes. > > Bootstrapped and tested on powerpc64-linux BE/LE and x86 with no > regressions. > Is this okay for trunk? Any recommendations? Thanks a lot. > > > ChangeLog > 2022-11-23 Haochen Gui > > gcc/ > * optabs.cc (prepare_cmp_insn): Go to fail other than assert it when > predicate check of "cbranchcc4" operand[0] fails. > > patch.diff > diff --git a/gcc/optabs.cc b/gcc/optabs.cc > index 165f8d1fa22..3ec8f6b17ba 100644 > --- a/gcc/optabs.cc > +++ b/gcc/optabs.cc > @@ -4484,8 +4484,9 @@ prepare_cmp_insn (rtx x, rtx y, enum rtx_code > comparison, rtx size, > { >enum insn_code icode = optab_handler (cbranch_optab, CCmode); >test = gen_rtx_fmt_ee (comparison, VOIDmode, x, y); > - gcc_assert (icode != CODE_FOR_nothing > - && insn_operand_matches (icode, 0, test)); > + gcc_assert (icode != CODE_FOR_nothing); > + if (!insn_operand_matches (icode, 0, test)) > + goto fail; IMHO, this change looks to accord with the other code in prepare_cmp_insn, which allows the preparation to fail with NULL_RTX ptest. Its caller can make its own decision (ICE due to unexpected, or try other ways) when ptest is null. If this direction is sensible, maybe we can make it goto fail too if the icode == CODE_FOR_nothing, since we already try to relax the restriction. BR, Kewen >*ptest = test; >return; > }
Re: Ping [PATCH] Change the behavior of predicate check failure on cbranchcc4 operand0 in prepare_cmp_insn
On Mon, Nov 28, 2022 at 6:33 AM HAO CHEN GUI via Gcc-patches wrote: > > Hi, >Gentle ping this: > https://gcc.gnu.org/pipermail/gcc-patches/2022-November/607083.html > Thanks > Gui Haochen > > 在 2022/11/23 10:54, HAO CHEN GUI 写道: > > Hi, > > I want to enable "have_cbranchcc4" on rs6000. But not all combinations of > > comparison codes and sub CC modes are benefited to generate cbranchcc4 insns > > on rs6000. There is an predicate for operand0 of cbranchcc4 to bypass > > some combinations. It gets assertion failure in prepare_cmp_insn. I think > > we shouldn't suppose that all comparison codes and sub CC modes are > > supported > > and throw an assertion failure in prepare_cmp_insn. It might check the > > predicate and go to fail if the predicate can't be satisfied. This patch > > changes the behavior of those codes. > > > > Bootstrapped and tested on powerpc64-linux BE/LE and x86 with no > > regressions. > > Is this okay for trunk? Any recommendations? Thanks a lot. Since the function seems to be allowed to fail the patch looks reasonable - still I wonder what the "fallback" for a MODE_CC style compare-and-branch is? There are callers of this function that do not seem to expect failure at least, some suspiciously looking like MODE_CC candiates. Richard. > > > > > > ChangeLog > > 2022-11-23 Haochen Gui > > > > gcc/ > > * optabs.cc (prepare_cmp_insn): Go to fail other than assert it when > > predicate check of "cbranchcc4" operand[0] fails. > > > > patch.diff > > diff --git a/gcc/optabs.cc b/gcc/optabs.cc > > index 165f8d1fa22..3ec8f6b17ba 100644 > > --- a/gcc/optabs.cc > > +++ b/gcc/optabs.cc > > @@ -4484,8 +4484,9 @@ prepare_cmp_insn (rtx x, rtx y, enum rtx_code > > comparison, rtx size, > > { > >enum insn_code icode = optab_handler (cbranch_optab, CCmode); > >test = gen_rtx_fmt_ee (comparison, VOIDmode, x, y); > > - gcc_assert (icode != CODE_FOR_nothing > > - && insn_operand_matches (icode, 0, test)); > > + gcc_assert (icode != CODE_FOR_nothing); > > + if (!insn_operand_matches (icode, 0, test)) > > + goto fail; > >*ptest = test; > >return; > > }
Re: Ping [PATCH] Change the behavior of predicate check failure on cbranchcc4 operand0 in prepare_cmp_insn
On Mon, Nov 28, 2022 at 09:42:05AM +0100, Richard Biener wrote: > Since the function seems to be allowed to fail the patch looks > reasonable - still I wonder > what the "fallback" for a MODE_CC style compare-and-branch is? There > are callers > of this function that do not seem to expect failure at least, some > suspiciously looking > like MODE_CC candiates. Hi! cbranchcc4 is *not* a compare-and-branch, like ccbranch4 for other modes are. Instead, it is a conditional branch. I still think it is a bad idea to use this same pattern name for a completely different (and much more basic) concept, it just confuses many things, makes us need exceptions in most users of cbranch4 :-( cbranchcc4 does not do a comparison. Instead, it uses the result of some previous comparison in some CC register (or anything else that set such a register). We want to use a cbranchcc4 to reuse some earlier comparison here. Which is great of course! But, redoing the (potentially expensive) computation to prepare the CC for a more complicated condition is not a good idea. Also, Power's conditional branch insns just branch on one of the 32 condition bits (either set or unset), not on a logical combination of multiple of those bits, as we need with LTGT, UNLT, UNGT, UNEQ, and LE and GE without fastmath. So it is much cleaner (and causes fewer problems later on) if we only allow those codes we do support. Example of LTGT: fcmpu 0,0,1 # compare f0 <=> f1 to cr0 (exactly one of # cr0.lt, cr0.gt, cr0.eq, cr0.un will be set) cror 2,0,1# cr0.eq = cr0.lt | cr0.gt beq 0 # branch if cr0.eq is set So, we want the cbranchcc4 here to just do that last insn, not the last two insns (or all three as any other cbranch4 is!) Segher
Re: Ping [PATCH] Change the behavior of predicate check failure on cbranchcc4 operand0 in prepare_cmp_insn
On Mon, Nov 28, 2022 at 6:58 PM Segher Boessenkool wrote: > > On Mon, Nov 28, 2022 at 09:42:05AM +0100, Richard Biener wrote: > > Since the function seems to be allowed to fail the patch looks > > reasonable - still I wonder > > what the "fallback" for a MODE_CC style compare-and-branch is? There > > are callers > > of this function that do not seem to expect failure at least, some > > suspiciously looking > > like MODE_CC candiates. > > Hi! > > cbranchcc4 is *not* a compare-and-branch, like ccbranch4 for other > modes are. Instead, it is a conditional branch. I still think it is a > bad idea to use this same pattern name for a completely different (and > much more basic) concept, it just confuses many things, makes us need > exceptions in most users of cbranch4 :-( > > cbranchcc4 does not do a comparison. Instead, it uses the result of > some previous comparison in some CC register (or anything else that set > such a register). We want to use a cbranchcc4 to reuse some earlier > comparison here. Which is great of course! But, redoing the > (potentially expensive) computation to prepare the CC for a more > complicated condition is not a good idea. Also, Power's conditional > branch insns just branch on one of the 32 condition bits (either set or > unset), not on a logical combination of multiple of those bits, as we > need with LTGT, UNLT, UNGT, UNEQ, and LE and GE without fastmath. So it > is much cleaner (and causes fewer problems later on) if we only allow > those codes we do support. > > Example of LTGT: > fcmpu 0,0,1 # compare f0 <=> f1 to cr0 (exactly one of > # cr0.lt, cr0.gt, cr0.eq, cr0.un will be set) > cror 2,0,1# cr0.eq = cr0.lt | cr0.gt > beq 0 # branch if cr0.eq is set > > So, we want the cbranchcc4 here to just do that last insn, not the last > two insns (or all three as any other cbranch4 is!) Anyhow - my question still stands - what's the fallback for the callers that do not check for failure? How are we sure we're not running into these when relaxing the requirement that a MODE_CC prepare_cmp_insn must not fail? Richard. > > Segher
Re: Ping [PATCH] Change the behavior of predicate check failure on cbranchcc4 operand0 in prepare_cmp_insn
On Mon, Nov 28, 2022 at 07:46:07PM +0100, Richard Biener wrote: > Anyhow - my question still stands - what's the fallback for the callers > that do not check for failure? How are we sure we're not running into > these when relaxing the requirement that a MODE_CC prepare_cmp_insn > must not fail? This will work the same as with any other define_expand? If the caller of gen_blablabla does not check for failure, you end up with a NULL_RTX in the instruction stream, which will ICE sooner or later. Not pretty, sure, but at least it is a reliable ICE :-) Segher
Re: Ping [PATCH] Change the behavior of predicate check failure on cbranchcc4 operand0 in prepare_cmp_insn
Hi Richard, 在 2022/11/29 2:46, Richard Biener 写道: > Anyhow - my question still stands - what's the fallback for the callers > that do not check for failure? How are we sure we're not running into > these when relaxing the requirement that a MODE_CC prepare_cmp_insn > must not fail? I examed the code and found that currently callers should be fine with returning a NULL_RTX for MODE_CC processing. The prepare_cmp_insn is called by following callers. 1 gen_cond_trap which doesn't uses MODE_CC 2 prepare_cmp_insn itself where is after MODE_CC processing, so it never hits MODE_CC 3 emit_cmp_and_jump_insns which doesn't uses MODE_CC 4 emit_conditional_move which checks the output is null or not 5 emit_conditional_add which checks the output is null or not Not sure if I missed something. Looking forward to your advice. Thanks a lot Gui Haochen
Re: Ping [PATCH] Change the behavior of predicate check failure on cbranchcc4 operand0 in prepare_cmp_insn
On Tue, Nov 29, 2022 at 2:15 AM HAO CHEN GUI wrote: > > Hi Richard, > > 在 2022/11/29 2:46, Richard Biener 写道: > > Anyhow - my question still stands - what's the fallback for the callers > > that do not check for failure? How are we sure we're not running into > > these when relaxing the requirement that a MODE_CC prepare_cmp_insn > > must not fail? > > I examed the code and found that currently callers should be fine with > returning a NULL_RTX for MODE_CC processing. The prepare_cmp_insn is called > by following callers. > > 1 gen_cond_trap which doesn't uses MODE_CC > 2 prepare_cmp_insn itself where is after MODE_CC processing, so it never > hits MODE_CC > 3 emit_cmp_and_jump_insns which doesn't uses MODE_CC > 4 emit_conditional_move which checks the output is null or not > 5 emit_conditional_add which checks the output is null or not Thanks for checking. > Not sure if I missed something. Looking forward to your advice. I'd then say the non-presence of the optab should be handled the same as a mismatching predicate as the other comment on the patch indicates. thanks, Richard. > Thanks a lot > Gui Haochen >