[Bug rtl-optimization/58517] ifcvt (after combine) puts ccreg clobbering insn between ccset insn and ccreg use
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58517 --- Comment #19 from Andrew Pinski --- (In reply to Andrew Pinski from comment #18) > Created attachment 55250 [details] > This patch also fixes the issue by looking back > > Here is a patch which does the lookback if the reg is a hard register and it > is considered a small reg class. Note with this version of the patch, you need to disable check against currently_expanding_to_rtl in movsicc too. Otherwise you won't get the ifcvt happening in general.
[Bug rtl-optimization/58517] ifcvt (after combine) puts ccreg clobbering insn between ccset insn and ccreg use
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58517 --- Comment #18 from Andrew Pinski --- Created attachment 55250 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=55250&action=edit This patch also fixes the issue by looking back Here is a patch which does the lookback if the reg is a hard register and it is considered a small reg class.
[Bug rtl-optimization/58517] ifcvt (after combine) puts ccreg clobbering insn between ccset insn and ccreg use
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58517 --- Comment #17 from Oleg Endo --- (In reply to Andrew Pinski from comment #16) > Now I am curious if T_REG should be BImode rather than SImode ... Then > ifcvt.cc would not have to be modified. I Know BImode is newer than when sh > target was added but maybe if someone cares about the sh target could try to > modify the backend to use BImode for T_REG here. Yeah, I know. Back then I was entertaining the idea. But T-bit being SImode is so intertwined with all other things in the backend, it looks like a big job to undo/redo that. For example there are patterns that use the T bit in other arithmetic insns patterns. Would need to check what would happen to those. Sounds like a can of worms.
[Bug rtl-optimization/58517] ifcvt (after combine) puts ccreg clobbering insn between ccset insn and ccreg use
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58517 --- Comment #16 from Andrew Pinski --- Now I am curious if T_REG should be BImode rather than SImode ... Then ifcvt.cc would not have to be modified. I Know BImode is newer than when sh target was added but maybe if someone cares about the sh target could try to modify the backend to use BImode for T_REG here.
[Bug rtl-optimization/58517] ifcvt (after combine) puts ccreg clobbering insn between ccset insn and ccreg use
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58517 --- Comment #15 from Andrew Pinski --- I was thinking of improving this is modify noce_get_condition here (we still need the previous patch just in case we still get the T register). Right now noce_get_condition will stop at: (eq (reg:SI 147 t) (const_int 0 [0])) Because of: /* If the condition variable is a register and is MODE_INT, accept it. */ cond = XEXP (SET_SRC (set), 0); tmp = XEXP (cond, 0); if (REG_P (tmp) && GET_MODE_CLASS (GET_MODE (tmp)) == MODE_INT && (GET_MODE (tmp) != BImode || !targetm.small_register_classes_for_mode_p (BImode))) But since 147 is a hard register but maybe since it is hard register and REGNO_REG_CLASS (147) == T_REGS and the register class of T_REGS is only one register so consider a small register class. This I think is related to the option 2 in comment #3.
[Bug rtl-optimization/58517] ifcvt (after combine) puts ccreg clobbering insn between ccset insn and ccreg use
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58517 --- Comment #14 from Andrew Pinski --- (In reply to Oleg Endo from comment #13) > (In reply to Andrew Pinski from comment #12) > > Created attachment 55239 [details] > > Patch which does work on this > > > > Though, I need to double to make sure it works for other cases still. > > sh is the case where we have a non-CC mode register for the flags which is > > why this was harder than other targets. > > Thanks for looking at this (after 10 years .. wow, time flies). > > > > diff --git a/gcc/config/sh/sh.md b/gcc/config/sh/sh.md > > index 4622dba0121..ae1a0f7eb7c 100644 > > --- a/gcc/config/sh/sh.md > > +++ b/gcc/config/sh/sh.md > > @@ -1510,7 +1510,7 @@ (define_expand "movsicc" > >rtx op0 = XEXP (operands[1], 0); > >rtx op1 = XEXP (operands[1], 1); > > > > - if (! currently_expanding_to_rtl) > > + if (0 && ! currently_expanding_to_rtl) > > FAIL; > > > > switch (code) > > diff --git a/gcc/config/sh/t-sh b/gcc/config/sh/t-sh > > index 1cc8c39d2a8..8ffcc288cf3 100644 > > --- a/gcc/config/sh/t-sh > > +++ b/gcc/config/sh/t-sh > > @@ -89,8 +89,8 @@ MULTILIB_OSDIRNAMES = \ > > m4a=!m4a $(OTHER_ENDIAN)/m4a=!$(OTHER_ENDIAN)/m4a \ > > m4al=!m4al $(OTHER_ENDIAN)/m4al=!$(OTHER_ENDIAN)/m4al > > > > -$(out_object_file): gt-sh.h > > -gt-sh.h : s-gtype ; @true > > +#$(out_object_file): gt-sh.h > > +#gt-sh.h : s-gtype ; @true > > > > # Local Variables: > > # mode: Makefile > > Are these hunks intentional? No. The second one was me trying to figure out why if I did make clean, the build would then fail, I thought it wad related to that but it was a m2 (the language) vs having a multilib dir named m2.
[Bug rtl-optimization/58517] ifcvt (after combine) puts ccreg clobbering insn between ccset insn and ccreg use
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58517 --- Comment #13 from Oleg Endo --- (In reply to Andrew Pinski from comment #12) > Created attachment 55239 [details] > Patch which does work on this > > Though, I need to double to make sure it works for other cases still. > sh is the case where we have a non-CC mode register for the flags which is > why this was harder than other targets. Thanks for looking at this (after 10 years .. wow, time flies). > diff --git a/gcc/config/sh/sh.md b/gcc/config/sh/sh.md > index 4622dba0121..ae1a0f7eb7c 100644 > --- a/gcc/config/sh/sh.md > +++ b/gcc/config/sh/sh.md > @@ -1510,7 +1510,7 @@ (define_expand "movsicc" >rtx op0 = XEXP (operands[1], 0); >rtx op1 = XEXP (operands[1], 1); > > - if (! currently_expanding_to_rtl) > + if (0 && ! currently_expanding_to_rtl) > FAIL; > > switch (code) > diff --git a/gcc/config/sh/t-sh b/gcc/config/sh/t-sh > index 1cc8c39d2a8..8ffcc288cf3 100644 > --- a/gcc/config/sh/t-sh > +++ b/gcc/config/sh/t-sh > @@ -89,8 +89,8 @@ MULTILIB_OSDIRNAMES = \ > m4a=!m4a $(OTHER_ENDIAN)/m4a=!$(OTHER_ENDIAN)/m4a \ > m4al=!m4al $(OTHER_ENDIAN)/m4al=!$(OTHER_ENDIAN)/m4al > > -$(out_object_file): gt-sh.h > -gt-sh.h : s-gtype ; @true > +#$(out_object_file): gt-sh.h > +#gt-sh.h : s-gtype ; @true > > # Local Variables: > # mode: Makefile Are these hunks intentional?
[Bug rtl-optimization/58517] ifcvt (after combine) puts ccreg clobbering insn between ccset insn and ccreg use
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58517 Andrew Pinski changed: What|Removed |Added Attachment #55237|0 |1 is obsolete|| --- Comment #12 from Andrew Pinski --- Created attachment 55239 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=55239&action=edit Patch which does work on this Though, I need to double to make sure it works for other cases still. sh is the case where we have a non-CC mode register for the flags which is why this was harder than other targets.
[Bug rtl-optimization/58517] ifcvt (after combine) puts ccreg clobbering insn between ccset insn and ccreg use
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58517 --- Comment #11 from Andrew Pinski --- (In reply to Andrew Pinski from comment #9) > The reason why r6-3654-g6b7e867187889 didn't fix this case is because it was > not looking into clobbers only the set side. > > Note the conditional in my patch should have been > if (reg_overlap_mentioned_p (DF_REF_REG (def), cond)) > > Maybe it should be bb_valid_for_noce_process_p instead, I am not 100% sure > on that. This does not fix the issue as (In reply to Andrew Pinski from comment #9) > The reason why r6-3654-g6b7e867187889 didn't fix this case is because it was > not looking into clobbers only the set side. > > Note the conditional in my patch should have been > if (reg_overlap_mentioned_p (DF_REF_REG (def), cond)) > > Maybe it should be bb_valid_for_noce_process_p instead, I am not 100% sure > on that. This patch is not enough. Though the patch in comment #4 is not enough any more either.
[Bug rtl-optimization/58517] ifcvt (after combine) puts ccreg clobbering insn between ccset insn and ccreg use
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58517 Andrew Pinski changed: What|Removed |Added Last reconfirmed|2013-10-05 00:00:00 |2023-6-1 --- Comment #10 from Andrew Pinski --- Note the wrong code can be still reproduced in GCC 13 too.
[Bug rtl-optimization/58517] ifcvt (after combine) puts ccreg clobbering insn between ccset insn and ccreg use
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58517 --- Comment #9 from Andrew Pinski --- The reason why r6-3654-g6b7e867187889 didn't fix this case is because it was not looking into clobbers only the set side. Note the conditional in my patch should have been if (reg_overlap_mentioned_p (DF_REF_REG (def), cond)) Maybe it should be bb_valid_for_noce_process_p instead, I am not 100% sure on that.
[Bug rtl-optimization/58517] ifcvt (after combine) puts ccreg clobbering insn between ccset insn and ccreg use
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58517 --- Comment #8 from Andrew Pinski --- Created attachment 55237 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=55237&action=edit Patch which I think will fix this This is option 1 of comment #3 though with an updated version. I have not tested this at all but I think it will work.
[Bug rtl-optimization/58517] ifcvt (after combine) puts ccreg clobbering insn between ccset insn and ccreg use
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58517 --- Comment #7 from Oleg Endo --- (In reply to ktkachov from comment #6) > Huh, I wasn't aware of this BZ. > This looks eerily similar to what I think is the root cause of PR 67481, > which I'm working on now. > > After I'm done with PR 67481 it would be interesting to have a look whether > they are indeed the same issue OK, thanks!
[Bug rtl-optimization/58517] ifcvt (after combine) puts ccreg clobbering insn between ccset insn and ccreg use
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58517 ktkachov at gcc dot gnu.org changed: What|Removed |Added CC||ktkachov at gcc dot gnu.org --- Comment #6 from ktkachov at gcc dot gnu.org --- (In reply to Oleg Endo from comment #5) > Kyrill, since you are currently working on ifcvt related things, could you > maybe have a look at this issue? I haven't checked it in a while, so I > don't know if it's still a problem. Huh, I wasn't aware of this BZ. This looks eerily similar to what I think is the root cause of PR 67481, which I'm working on now. After I'm done with PR 67481 it would be interesting to have a look whether they are indeed the same issue
[Bug rtl-optimization/58517] ifcvt (after combine) puts ccreg clobbering insn between ccset insn and ccreg use
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58517 Oleg Endo changed: What|Removed |Added CC||kyrylo.tkachov at arm dot com --- Comment #5 from Oleg Endo --- Kyrill, since you are currently working on ifcvt related things, could you maybe have a look at this issue? I haven't checked it in a while, so I don't know if it's still a problem.
