* [PATCH] Change the behavior of predicate check failure on cbranchcc4 operand0 in prepare_cmp_insn @ 2022-11-23 2:54 HAO CHEN GUI 2022-11-28 5:32 ` Ping " HAO CHEN GUI 2022-11-28 6:16 ` Kewen.Lin 0 siblings, 2 replies; 9+ messages in thread From: HAO CHEN GUI @ 2022-11-23 2:54 UTC (permalink / raw) To: gcc-patches; +Cc: Segher Boessenkool, David, Peter Bergner, Kewen.Lin 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 <guihaoc@linux.ibm.com> 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; } ^ permalink raw reply [flat|nested] 9+ messages in thread
* Ping [PATCH] Change the behavior of predicate check failure on cbranchcc4 operand0 in prepare_cmp_insn 2022-11-23 2:54 [PATCH] Change the behavior of predicate check failure on cbranchcc4 operand0 in prepare_cmp_insn HAO CHEN GUI @ 2022-11-28 5:32 ` HAO CHEN GUI 2022-11-28 8:42 ` Richard Biener 2022-11-28 6:16 ` Kewen.Lin 1 sibling, 1 reply; 9+ messages in thread From: HAO CHEN GUI @ 2022-11-28 5:32 UTC (permalink / raw) To: gcc-patches; +Cc: Segher Boessenkool, David, Peter Bergner, Kewen.Lin 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 <guihaoc@linux.ibm.com> > > 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; > } ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Ping [PATCH] Change the behavior of predicate check failure on cbranchcc4 operand0 in prepare_cmp_insn 2022-11-28 5:32 ` Ping " HAO CHEN GUI @ 2022-11-28 8:42 ` Richard Biener 2022-11-28 17:56 ` Segher Boessenkool 0 siblings, 1 reply; 9+ messages in thread From: Richard Biener @ 2022-11-28 8:42 UTC (permalink / raw) To: HAO CHEN GUI Cc: gcc-patches, Segher Boessenkool, David, Peter Bergner, Kewen.Lin On Mon, Nov 28, 2022 at 6:33 AM HAO CHEN GUI via Gcc-patches <gcc-patches@gcc.gnu.org> 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 <guihaoc@linux.ibm.com> > > > > 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; > > } ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Ping [PATCH] Change the behavior of predicate check failure on cbranchcc4 operand0 in prepare_cmp_insn 2022-11-28 8:42 ` Richard Biener @ 2022-11-28 17:56 ` Segher Boessenkool 2022-11-28 18:46 ` Richard Biener 0 siblings, 1 reply; 9+ messages in thread From: Segher Boessenkool @ 2022-11-28 17:56 UTC (permalink / raw) To: Richard Biener; +Cc: HAO CHEN GUI, gcc-patches, David, Peter Bergner, Kewen.Lin 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 ccbranch<mode>4 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 cbranch<mode>4 :-( 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 cbranch<mode>4 is!) Segher ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Ping [PATCH] Change the behavior of predicate check failure on cbranchcc4 operand0 in prepare_cmp_insn 2022-11-28 17:56 ` Segher Boessenkool @ 2022-11-28 18:46 ` Richard Biener 2022-11-28 23:02 ` Segher Boessenkool 2022-11-29 1:15 ` HAO CHEN GUI 0 siblings, 2 replies; 9+ messages in thread From: Richard Biener @ 2022-11-28 18:46 UTC (permalink / raw) To: Segher Boessenkool Cc: HAO CHEN GUI, gcc-patches, David, Peter Bergner, Kewen.Lin On Mon, Nov 28, 2022 at 6:58 PM Segher Boessenkool <segher@kernel.crashing.org> 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 ccbranch<mode>4 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 cbranch<mode>4 :-( > > 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 cbranch<mode>4 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Ping [PATCH] Change the behavior of predicate check failure on cbranchcc4 operand0 in prepare_cmp_insn 2022-11-28 18:46 ` Richard Biener @ 2022-11-28 23:02 ` Segher Boessenkool 2022-11-29 1:15 ` HAO CHEN GUI 1 sibling, 0 replies; 9+ messages in thread From: Segher Boessenkool @ 2022-11-28 23:02 UTC (permalink / raw) To: Richard Biener; +Cc: HAO CHEN GUI, gcc-patches, David, Peter Bergner, Kewen.Lin 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Ping [PATCH] Change the behavior of predicate check failure on cbranchcc4 operand0 in prepare_cmp_insn 2022-11-28 18:46 ` Richard Biener 2022-11-28 23:02 ` Segher Boessenkool @ 2022-11-29 1:15 ` HAO CHEN GUI 2022-11-29 8:26 ` Richard Biener 1 sibling, 1 reply; 9+ messages in thread From: HAO CHEN GUI @ 2022-11-29 1:15 UTC (permalink / raw) To: Richard Biener Cc: gcc-patches, David, Peter Bergner, Kewen.Lin, Segher Boessenkool 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Ping [PATCH] Change the behavior of predicate check failure on cbranchcc4 operand0 in prepare_cmp_insn 2022-11-29 1:15 ` HAO CHEN GUI @ 2022-11-29 8:26 ` Richard Biener 0 siblings, 0 replies; 9+ messages in thread From: Richard Biener @ 2022-11-29 8:26 UTC (permalink / raw) To: HAO CHEN GUI Cc: gcc-patches, David, Peter Bergner, Kewen.Lin, Segher Boessenkool On Tue, Nov 29, 2022 at 2:15 AM HAO CHEN GUI <guihaoc@linux.ibm.com> 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 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Change the behavior of predicate check failure on cbranchcc4 operand0 in prepare_cmp_insn 2022-11-23 2:54 [PATCH] Change the behavior of predicate check failure on cbranchcc4 operand0 in prepare_cmp_insn HAO CHEN GUI 2022-11-28 5:32 ` Ping " HAO CHEN GUI @ 2022-11-28 6:16 ` Kewen.Lin 1 sibling, 0 replies; 9+ messages in thread From: Kewen.Lin @ 2022-11-28 6:16 UTC (permalink / raw) To: HAO CHEN GUI Cc: Segher Boessenkool, David, Peter Bergner, gcc-patches, Richard Sandiford, Richard Biener, Jeff Law, Jakub Jelinek, Michael Meissner 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 <guihaoc@linux.ibm.com> > > 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; > } ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-11-29 8:26 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-11-23 2:54 [PATCH] Change the behavior of predicate check failure on cbranchcc4 operand0 in prepare_cmp_insn HAO CHEN GUI 2022-11-28 5:32 ` Ping " HAO CHEN GUI 2022-11-28 8:42 ` Richard Biener 2022-11-28 17:56 ` Segher Boessenkool 2022-11-28 18:46 ` Richard Biener 2022-11-28 23:02 ` Segher Boessenkool 2022-11-29 1:15 ` HAO CHEN GUI 2022-11-29 8:26 ` Richard Biener 2022-11-28 6:16 ` Kewen.Lin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).