| Message ID | CAFULd4ZCs6tpuF2NPvJ2tikPKr39BMBHFKteC92Hi1w1v4oZvw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] combine: Fix ICE in try_combine on pr112494.c [PR112560] | expand |
On Thu, 7 Mar 2024, Uros Bizjak wrote: > The compiler, configured with --enable-checking=yes,rtl,extra ICEs with: > > internal compiler error: RTL check: expected elt 0 type 'e' or 'u', > have 'E' (rtx unspec) in try_combine, at combine.cc:3237 > > This is > > 3236 /* Just replace the CC reg with a new mode. */ > 3237 SUBST (XEXP (*cc_use_loc, 0), newpat_dest); > 3238 undobuf.other_insn = cc_use_insn; > > in combine.cc, where *cc_use_loc is > > (unspec:DI [ > (reg:CC 17 flags) > ] UNSPEC_PUSHFL) > > combine assumes CC must be used inside of a comparison and uses XEXP (..., 0) > without checking on the RTX type of the argument. > > Undo the combination if *cc_use_loc is not COMPARISON_P. > > Also remove buggy and now redundant check for (const 0) RTX as part of > the comparison. > > PR rtl-optimization/112560 > > gcc/ChangeLog: > > * combine.cc (try_combine): Reject the combination > if *cc_use_loc is not COMPARISON_P. > > Bootstrapped and regression tested on x86_64-pc-linux-gnu {,-m32}. > > OK for trunk? Since you CCed me - looking at the code I wonder why we fatally fail. The following might also fix the issue and preserve more of the rest of the flow of the function. If that works I'd prefer it. But I'll defer approval to the combine maintainer which is Segher. Thanks, Richard. diff --git a/gcc/combine.cc b/gcc/combine.cc index a4479f8d836..e280cd72ec7 100644 --- a/gcc/combine.cc +++ b/gcc/combine.cc @@ -3182,7 +3182,8 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0, if (undobuf.other_insn == 0 && (cc_use_loc = find_single_use (SET_DEST (newpat), i3, - &cc_use_insn))) + &cc_use_insn)) + && COMPARISON_P (*cc_use_loc)) { compare_code = orig_compare_code = GET_CODE (*cc_use_loc); if (is_a <scalar_int_mode> (GET_MODE (i2dest), &mode)) @@ -3200,7 +3201,7 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0, the above simplify_compare_const() returned a new comparison operator. undobuf.other_insn is assigned the CC use insn when modifying it. */ - if (cc_use_loc) + if (cc_use_loc && COMPARISON_P (*cc_use_loc)) { #ifdef SELECT_CC_MODE machine_mode new_mode
On Thu, Mar 7, 2024 at 10:56 AM Richard Biener <rguenther@suse.de> wrote: > > On Thu, 7 Mar 2024, Uros Bizjak wrote: > > > The compiler, configured with --enable-checking=yes,rtl,extra ICEs with: > > > > internal compiler error: RTL check: expected elt 0 type 'e' or 'u', > > have 'E' (rtx unspec) in try_combine, at combine.cc:3237 > > > > This is > > > > 3236 /* Just replace the CC reg with a new mode. */ > > 3237 SUBST (XEXP (*cc_use_loc, 0), newpat_dest); > > 3238 undobuf.other_insn = cc_use_insn; > > > > in combine.cc, where *cc_use_loc is > > > > (unspec:DI [ > > (reg:CC 17 flags) > > ] UNSPEC_PUSHFL) > > > > combine assumes CC must be used inside of a comparison and uses XEXP (..., 0) > > without checking on the RTX type of the argument. > > > > Undo the combination if *cc_use_loc is not COMPARISON_P. > > > > Also remove buggy and now redundant check for (const 0) RTX as part of > > the comparison. > > > > PR rtl-optimization/112560 > > > > gcc/ChangeLog: > > > > * combine.cc (try_combine): Reject the combination > > if *cc_use_loc is not COMPARISON_P. > > > > Bootstrapped and regression tested on x86_64-pc-linux-gnu {,-m32}. > > > > OK for trunk? > > Since you CCed me - looking at the code I wonder why we fatally fail. > The following might also fix the issue and preserve more of the > rest of the flow of the function. > > If that works I'd prefer it. But I'll defer approval to the combine > maintainer which is Segher. Your patch is basically what v1 did [1], but it was suggested (in a reply by you ;) ) that we should stop the attempt to combine if we can't handle the use. So, the v2 patch undoes the combine and records a nice message in this case. Also, please note the removal of an existing crude hack that tries to reject non-comparison uses by looking for (const_int 0) in the use RTX, which is wrong as well. [1] https://gcc.gnu.org/pipermail/gcc-patches/2023-November/638589.html Uros. > > Thanks, > Richard. > > diff --git a/gcc/combine.cc b/gcc/combine.cc > index a4479f8d836..e280cd72ec7 100644 > --- a/gcc/combine.cc > +++ b/gcc/combine.cc > @@ -3182,7 +3182,8 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn > *i1, rtx_insn *i0, > > if (undobuf.other_insn == 0 > && (cc_use_loc = find_single_use (SET_DEST (newpat), i3, > - &cc_use_insn))) > + &cc_use_insn)) > + && COMPARISON_P (*cc_use_loc)) > { > compare_code = orig_compare_code = GET_CODE (*cc_use_loc); > if (is_a <scalar_int_mode> (GET_MODE (i2dest), &mode)) > @@ -3200,7 +3201,7 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn > *i1, rtx_insn *i0, > the above simplify_compare_const() returned a new comparison > operator. undobuf.other_insn is assigned the CC use insn > when modifying it. */ > - if (cc_use_loc) > + if (cc_use_loc && COMPARISON_P (*cc_use_loc)) > { > #ifdef SELECT_CC_MODE > machine_mode new_mode >
On Thu, 7 Mar 2024, Uros Bizjak wrote: > On Thu, Mar 7, 2024 at 10:56?AM Richard Biener <rguenther@suse.de> wrote: > > > > On Thu, 7 Mar 2024, Uros Bizjak wrote: > > > > > The compiler, configured with --enable-checking=yes,rtl,extra ICEs with: > > > > > > internal compiler error: RTL check: expected elt 0 type 'e' or 'u', > > > have 'E' (rtx unspec) in try_combine, at combine.cc:3237 > > > > > > This is > > > > > > 3236 /* Just replace the CC reg with a new mode. */ > > > 3237 SUBST (XEXP (*cc_use_loc, 0), newpat_dest); > > > 3238 undobuf.other_insn = cc_use_insn; > > > > > > in combine.cc, where *cc_use_loc is > > > > > > (unspec:DI [ > > > (reg:CC 17 flags) > > > ] UNSPEC_PUSHFL) > > > > > > combine assumes CC must be used inside of a comparison and uses XEXP (..., 0) > > > without checking on the RTX type of the argument. > > > > > > Undo the combination if *cc_use_loc is not COMPARISON_P. > > > > > > Also remove buggy and now redundant check for (const 0) RTX as part of > > > the comparison. > > > > > > PR rtl-optimization/112560 > > > > > > gcc/ChangeLog: > > > > > > * combine.cc (try_combine): Reject the combination > > > if *cc_use_loc is not COMPARISON_P. > > > > > > Bootstrapped and regression tested on x86_64-pc-linux-gnu {,-m32}. > > > > > > OK for trunk? > > > > Since you CCed me - looking at the code I wonder why we fatally fail. > > The following might also fix the issue and preserve more of the > > rest of the flow of the function. > > > > If that works I'd prefer it. But I'll defer approval to the combine > > maintainer which is Segher. > > Your patch is basically what v1 did [1], but it was suggested (in a > reply by you ;) ) that we should stop the attempt to combine if we > can't handle the use. So, the v2 patch undoes the combine and records > a nice message in this case. Ah, sorry. I think I was merely curious how this works at all. Your patch doesn't stop when find_single_use returns NULL for multiple uses, but that case (as compared to "no other use") should be handled the same as a !COMPARISON_P single-use. The original patch preserves the multi-use behavior which might be a lot more unlikely than the !COMPARISON_P use case. That said, in the original mail I questioned whether combine handles the multi-use case correctly at all. Looking at find_single_use it seems to implement find_first_use. Or get_use_of_single_use with the pretext that there actually is only at most a single use (it doesn't seem to assert there is one?). Strange function, that. But I don't know combine, I hope Segher can answer and pick a patch. Richard. > Also, please note the removal of an existing crude hack that tries to > reject non-comparison uses by looking for (const_int 0) in the use > RTX, which is wrong as well. > > [1] https://gcc.gnu.org/pipermail/gcc-patches/2023-November/638589.html > > Uros. > > > > > Thanks, > > Richard. > > > > diff --git a/gcc/combine.cc b/gcc/combine.cc > > index a4479f8d836..e280cd72ec7 100644 > > --- a/gcc/combine.cc > > +++ b/gcc/combine.cc > > @@ -3182,7 +3182,8 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn > > *i1, rtx_insn *i0, > > > > if (undobuf.other_insn == 0 > > && (cc_use_loc = find_single_use (SET_DEST (newpat), i3, > > - &cc_use_insn))) > > + &cc_use_insn)) > > + && COMPARISON_P (*cc_use_loc)) > > { > > compare_code = orig_compare_code = GET_CODE (*cc_use_loc); > > if (is_a <scalar_int_mode> (GET_MODE (i2dest), &mode)) > > @@ -3200,7 +3201,7 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn > > *i1, rtx_insn *i0, > > the above simplify_compare_const() returned a new comparison > > operator. undobuf.other_insn is assigned the CC use insn > > when modifying it. */ > > - if (cc_use_loc) > > + if (cc_use_loc && COMPARISON_P (*cc_use_loc)) > > { > > #ifdef SELECT_CC_MODE > > machine_mode new_mode > > >
On Thu, Mar 07, 2024 at 11:11:35AM +0100, Uros Bizjak wrote: > > Since you CCed me - looking at the code I wonder why we fatally fail. > > The following might also fix the issue and preserve more of the > > rest of the flow of the function. > > > > If that works I'd prefer it. But I'll defer approval to the combine > > maintainer which is Segher. > > Your patch is basically what v1 did [1], but it was suggested (in a > reply by you ;) ) that we should stop the attempt to combine if we > can't handle the use. So, the v2 patch undoes the combine and records > a nice message in this case. My understanding of Richi's patch is that it it treats the non-COMPARISON_P the same as if find_single_use fails, which is a common case that certainly has to be handled right and it doesn't seem that we are giving up completely for that case. So, I think it is reasonable to treat the non-COMPARISON_P *cc_use_loc as NULL cc_use_loc. Jakub
On Thu, 7 Mar 2024, Jakub Jelinek wrote: > On Thu, Mar 07, 2024 at 11:11:35AM +0100, Uros Bizjak wrote: > > > Since you CCed me - looking at the code I wonder why we fatally fail. > > > The following might also fix the issue and preserve more of the > > > rest of the flow of the function. > > > > > > If that works I'd prefer it. But I'll defer approval to the combine > > > maintainer which is Segher. > > > > Your patch is basically what v1 did [1], but it was suggested (in a > > reply by you ;) ) that we should stop the attempt to combine if we > > can't handle the use. So, the v2 patch undoes the combine and records > > a nice message in this case. > > My understanding of Richi's patch is that it it treats the non-COMPARISON_P > the same as if find_single_use fails, which is a common case that certainly > has to be handled right and it doesn't seem that we are giving up completely > for that case. So, I think it is reasonable to treat the non-COMPARISON_P > *cc_use_loc as NULL cc_use_loc. The question is, whether a NULL cc_use_loc (find_single_use returning NULL) means "there is no use" or it can mean "huh, don't know, maybe more than one, maybe I was too stupid to indentify the single use". The implementation suggests it's all broken ;) Richard.
On Thu, Mar 7, 2024 at 11:37 AM Jakub Jelinek <jakub@redhat.com> wrote: > > On Thu, Mar 07, 2024 at 11:11:35AM +0100, Uros Bizjak wrote: > > > Since you CCed me - looking at the code I wonder why we fatally fail. > > > The following might also fix the issue and preserve more of the > > > rest of the flow of the function. > > > > > > If that works I'd prefer it. But I'll defer approval to the combine > > > maintainer which is Segher. > > > > Your patch is basically what v1 did [1], but it was suggested (in a > > reply by you ;) ) that we should stop the attempt to combine if we > > can't handle the use. So, the v2 patch undoes the combine and records > > a nice message in this case. > > My understanding of Richi's patch is that it it treats the non-COMPARISON_P > the same as if find_single_use fails, which is a common case that certainly > has to be handled right and it doesn't seem that we are giving up completely > for that case. So, I think it is reasonable to treat the non-COMPARISON_P > *cc_use_loc as NULL cc_use_loc. Please see the logic in my v1 patch. For COMPARISON_P (*cc_use_loc), we execute the same code in the first hunk of the patch, but for non-COMPARISON_P, my patch zeroes cc_use_loc. The cc_use_loc is used only in the "if (cc_use_loc)" protected part, so clearing cc_use_loc when !COMPARISON_P (*cc_use_loc) has exactly the same effect as adding COMPARISON_P check to existing "if (cc_use_loc) - we can execute the "if" part only when *cc_use_loc is a comparison. The functionality of Richi's patch is exactly the same as my v1 patch which was rejected for the reason mentioned in my previous post. Uros.
On Thu, Mar 7, 2024 at 12:11 PM Richard Biener <rguenther@suse.de> wrote: > > On Thu, 7 Mar 2024, Jakub Jelinek wrote: > > > On Thu, Mar 07, 2024 at 11:11:35AM +0100, Uros Bizjak wrote: > > > > Since you CCed me - looking at the code I wonder why we fatally fail. > > > > The following might also fix the issue and preserve more of the > > > > rest of the flow of the function. > > > > > > > > If that works I'd prefer it. But I'll defer approval to the combine > > > > maintainer which is Segher. > > > > > > Your patch is basically what v1 did [1], but it was suggested (in a > > > reply by you ;) ) that we should stop the attempt to combine if we > > > can't handle the use. So, the v2 patch undoes the combine and records > > > a nice message in this case. > > > > My understanding of Richi's patch is that it it treats the non-COMPARISON_P > > the same as if find_single_use fails, which is a common case that certainly > > has to be handled right and it doesn't seem that we are giving up completely > > for that case. So, I think it is reasonable to treat the non-COMPARISON_P > > *cc_use_loc as NULL cc_use_loc. > > The question is, whether a NULL cc_use_loc (find_single_use returning > NULL) means "there is no use" or it can mean "huh, don't know, maybe > more than one, maybe I was too stupid to indentify the single use". > The implementation suggests it's all broken ;) As I understood find_single_use, it is returning RTX iff DEST is used only a single time in an insn sequence following INSN. find_single_use_1 returns RTX iff argument is used exactly once in DEST. So, find_single_use returns RTX only when DEST is used exactly once in a sequence following INSN. We can reject the combination without worries of multiple uses. Uros,
On Thu, Mar 07, 2024 at 10:55:12AM +0100, Richard Biener wrote: > On Thu, 7 Mar 2024, Uros Bizjak wrote: > > This is > > > > 3236 /* Just replace the CC reg with a new mode. */ > > 3237 SUBST (XEXP (*cc_use_loc, 0), newpat_dest); > > 3238 undobuf.other_insn = cc_use_insn; > > > > in combine.cc, where *cc_use_loc is > > > > (unspec:DI [ > > (reg:CC 17 flags) > > ] UNSPEC_PUSHFL) > > > > combine assumes CC must be used inside of a comparison and uses XEXP (..., 0) No. It has established *this is the case* some time earlier. Lines\ 3155 and on, what begins with /* Many machines have insns that can both perform an arithmetic operation and set the condition code. > > OK for trunk? > > Since you CCed me - looking at the code I wonder why we fatally fail. I did not get this email btw. Some blip in email (on the sender's side) I guess? > The following might also fix the issue and preserve more of the > rest of the flow of the function. > --- a/gcc/combine.cc > +++ b/gcc/combine.cc > @@ -3182,7 +3182,8 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn > *i1, rtx_insn *i0, > > if (undobuf.other_insn == 0 > && (cc_use_loc = find_single_use (SET_DEST (newpat), i3, > - &cc_use_insn))) > + &cc_use_insn)) > + && COMPARISON_P (*cc_use_loc)) Line 3167 already is && GET_CODE (SET_SRC (PATTERN (i3))) == COMPARE so what in your backend is unusual? Segher
On Thu, Mar 07, 2024 at 11:22:12AM +0100, Richard Biener wrote:
> > > > Undo the combination if *cc_use_loc is not COMPARISON_P.
Why, anyway? COMPARISON_P means things like LE. It does not even
include actual RTX COMPARE.
Segher
On Thu, Mar 07, 2024 at 11:45:45AM +0100, Richard Biener wrote: > The question is, whether a NULL cc_use_loc (find_single_use returning > NULL) means "there is no use" or it can mean "huh, don't know, maybe > more than one, maybe I was too stupid to indentify the single use". > The implementation suggests it's all broken ;) It specifically means there is not a *single* use (or it could not find what it was, perhaps). It does not mean there is no use. There is not much in combine that cares about dead code anyway, earier passes should have taken care of that ;-) All as documented. Segher
On Thu, Mar 07, 2024 at 12:22:04PM +0100, Uros Bizjak wrote: > As I understood find_single_use, it is returning RTX iff DEST is used > only a single time in an insn sequence following INSN. Connected by a log_link even, yeah. > We can reject the combination without worries of multiple uses. Yup. That is the whole point of find_single_use: if that test fails, combine knows to get its paws off :-) Segher
On Thu, Mar 7, 2024 at 6:39 PM Segher Boessenkool <segher@kernel.crashing.org> wrote: > > On Thu, Mar 07, 2024 at 10:55:12AM +0100, Richard Biener wrote: > > On Thu, 7 Mar 2024, Uros Bizjak wrote: > > > This is > > > > > > 3236 /* Just replace the CC reg with a new mode. */ > > > 3237 SUBST (XEXP (*cc_use_loc, 0), newpat_dest); > > > 3238 undobuf.other_insn = cc_use_insn; > > > > > > in combine.cc, where *cc_use_loc is > > > > > > (unspec:DI [ > > > (reg:CC 17 flags) > > > ] UNSPEC_PUSHFL) > > > > > > combine assumes CC must be used inside of a comparison and uses XEXP (..., 0) > > No. It has established *this is the case* some time earlier. Lines\ > 3155 and on, what begins with > /* Many machines have insns that can both perform an > arithmetic operation and set the condition code. > > > > OK for trunk? > > > > Since you CCed me - looking at the code I wonder why we fatally fail. > > I did not get this email btw. Some blip in email (on the sender's side) > I guess? > > > The following might also fix the issue and preserve more of the > > rest of the flow of the function. > > > --- a/gcc/combine.cc > > +++ b/gcc/combine.cc > > @@ -3182,7 +3182,8 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn > > *i1, rtx_insn *i0, > > > > if (undobuf.other_insn == 0 > > && (cc_use_loc = find_single_use (SET_DEST (newpat), i3, > > - &cc_use_insn))) > > + &cc_use_insn)) > > + && COMPARISON_P (*cc_use_loc)) > > Line 3167 already is > && GET_CODE (SET_SRC (PATTERN (i3))) == COMPARE > so what in your backend is unusual? When combine tries to combine instructions involving COMPARE RTX, e.g.: (define_insn "*add<mode>_2" [(set (reg FLAGS_REG) (compare (plus:SWI (match_operand:SWI 1 "nonimmediate_operand" "%0,0,<r>,rm,r") (match_operand:SWI 2 "<general_operand>" "<r><i>,<m>,0,r<i>,<m>")) (const_int 0))) (set (match_operand:SWI 0 "nonimmediate_operand" "=<r>m,<r>,<r>,r,r") (plus:SWI (match_dup 1) (match_dup 2)))] it also updates the *user* of the CC register. The *user* is e.g.: (define_insn "*setcc_qi" [(set (match_operand:QI 0 "nonimmediate_operand" "=qm") (match_operator:QI 1 "ix86_comparison_operator" [(reg FLAGS_REG) (const_int 0)]))] where "ix86_comparison_operator" is one of EQ, NE, GE, LT ... RTX codes. The part we want to fix deals with the *user* of the CC register. It is not true that this is always COMPARISON_P, so EQ, NE, GE, LT, ... in the form of (LT:CCGC (reg:CCGC 17 flags) (const_int 0)) but can be something else, such as the above noted (unspec:DI [ (reg:CC 17 flags) ] UNSPEC_PUSHFL) The source code that deals with the *user* of the CC register assumes the former form, so it blindly tries to update the mode of the CC register inside LT comparison RTX (some other nearby source code even checks for (const_int 0) RTX). Obviously, this is not the case with the former form, where the update tries to: SUBST (XEXP (*cc_use_loc, 0), ...) on unspec, which has no XEXP (..., 0). And *this* is what triggers RTX checking assert. Uros.
On Thu, Mar 7, 2024 at 10:04 PM Uros Bizjak <ubizjak@gmail.com> wrote: > The source code that deals with the *user* of the CC register assumes > the former form, so it blindly tries to update the mode of the CC > register inside LT comparison RTX (some other nearby source code even > checks for (const_int 0) RTX). Obviously, this is not the case with > the former form, where the update tries to: Please read the above as: ... Obviously, this won't work with the former form, ... Uros.
On Thu, Mar 07, 2024 at 10:04:32PM +0100, Uros Bizjak wrote: [snip] > The part we want to fix deals with the *user* of the CC register. It > is not true that this is always COMPARISON_P, so EQ, NE, GE, LT, ... > in the form of > > (LT:CCGC (reg:CCGC 17 flags) (const_int 0)) > > but can be something else, such as the above noted > > (unspec:DI [ > (reg:CC 17 flags) > ] UNSPEC_PUSHFL) But that is invalid RTL? The only valid use of a CC is written as cc-compared-to-0. It means "the result of something compared to 0, stored in that cc reg". (And you can copy a CC reg around, but that is not a use ;-) ) > The source code that deals with the *user* of the CC register assumes > the former form, so it blindly tries to update the mode of the CC > register inside LT comparison RTX Would you like it better if there was an assert for this? There are very many RTL requirements that aren't chacked for currently :-/ > (some other nearby source code even > checks for (const_int 0) RTX). Obviously, this is not the case with > the former form, where the update tries to: > > SUBST (XEXP (*cc_use_loc, 0), ...) > > on unspec, which has no XEXP (..., 0). > > And *this* is what triggers RTX checking assert. The unspec should have the CC compared with 0 as argument. Segher
On Thu, Mar 7, 2024 at 10:37 PM Segher Boessenkool <segher@kernel.crashing.org> wrote: > > On Thu, Mar 07, 2024 at 10:04:32PM +0100, Uros Bizjak wrote: > > [snip] > > > The part we want to fix deals with the *user* of the CC register. It > > is not true that this is always COMPARISON_P, so EQ, NE, GE, LT, ... > > in the form of > > > > (LT:CCGC (reg:CCGC 17 flags) (const_int 0)) > > > > but can be something else, such as the above noted > > > > (unspec:DI [ > > (reg:CC 17 flags) > > ] UNSPEC_PUSHFL) > > But that is invalid RTL? The only valid use of a CC is written as > cc-compared-to-0. It means "the result of something compared to 0, > stored in that cc reg". > > (And you can copy a CC reg around, but that is not a use ;-) ) How can we describe a pushfl then? It was changed to its current form in [1], but I suspect that the code will try to update it even when pushfl is implemented as a direct move from a register (as was defined previously). BTW: Unspecs are used in a couple of other places for other targets [2]. [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112494#c5 [2] https://gcc.gnu.org/pipermail/gcc-patches/2023-December/639743.html > > > The source code that deals with the *user* of the CC register assumes > > the former form, so it blindly tries to update the mode of the CC > > register inside LT comparison RTX > > Would you like it better if there was an assert for this? There are > very many RTL requirements that aren't chacked for currently :-/ In this case - yes. Assert signals that something is unsupported (or invalid), way better than silently corrupting some memory, reporting the corruption only with checking enabled. > > > (some other nearby source code even > > checks for (const_int 0) RTX). Obviously, this is not the case with > > the former form, where the update tries to: > > > > SUBST (XEXP (*cc_use_loc, 0), ...) > > > > on unspec, which has no XEXP (..., 0). > > > > And *this* is what triggers RTX checking assert. > > The unspec should have the CC compared with 0 as argument. But this does not do what pushfl does... It pushes the register to the stack. Uros.
On Thu, Mar 07, 2024 at 11:07:18PM +0100, Uros Bizjak wrote: > On Thu, Mar 7, 2024 at 10:37 PM Segher Boessenkool > <segher@kernel.crashing.org> wrote: > > > but can be something else, such as the above noted > > > > > > (unspec:DI [ > > > (reg:CC 17 flags) > > > ] UNSPEC_PUSHFL) > > > > But that is invalid RTL? The only valid use of a CC is written as > > cc-compared-to-0. It means "the result of something compared to 0, > > stored in that cc reg". > > > > (And you can copy a CC reg around, but that is not a use ;-) ) > > How can we describe a pushfl then? (unspec:DI [ (compare:CC) ((reg:CC 17 flags) (const_int 0)) ] UNSPEC_PUSHFL) or something like that? > It was changed to its current form > in [1], but I suspect that the code will try to update it even when > pushfl is implemented as a direct move from a register (as was defined > previously). > > BTW: Unspecs are used in a couple of other places for other targets [2]. > > [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112494#c5 > [2] https://gcc.gnu.org/pipermail/gcc-patches/2023-December/639743.html There is nothing wront with unspecs. You cannot use a CCmode value without comparing it to 0, though. The exact kind of comparison determines what bits are valid (and have what meaning) in your CC reg, even! > > > The source code that deals with the *user* of the CC register assumes > > > the former form, so it blindly tries to update the mode of the CC > > > register inside LT comparison RTX > > > > Would you like it better if there was an assert for this? There are > > very many RTL requirements that aren't chacked for currently :-/ > > In this case - yes. Assert signals that something is unsupported (or > invalid), way better than silently corrupting some memory, reporting > the corruption only with checking enabled. Yeah. The way RTL checking works makes this hard to do in most cases. Hrm. (It cannot easily look at context, only inside of the current RTX). > > The unspec should have the CC compared with 0 as argument. > > But this does not do what pushfl does... It pushes the register to the stack. Can't you just describe the dataflow then, without an unspec? An unspec by definition does some (unspecified) operation on the data. Segher
On Thu, Mar 7, 2024 at 11:07 PM Uros Bizjak <ubizjak@gmail.com> wrote: > > On Thu, Mar 7, 2024 at 10:37 PM Segher Boessenkool > <segher@kernel.crashing.org> wrote: > > > > On Thu, Mar 07, 2024 at 10:04:32PM +0100, Uros Bizjak wrote: > > > > [snip] > > > > > The part we want to fix deals with the *user* of the CC register. It > > > is not true that this is always COMPARISON_P, so EQ, NE, GE, LT, ... > > > in the form of > > > > > > (LT:CCGC (reg:CCGC 17 flags) (const_int 0)) > > > > > > but can be something else, such as the above noted > > > > > > (unspec:DI [ > > > (reg:CC 17 flags) > > > ] UNSPEC_PUSHFL) > > > > But that is invalid RTL? The only valid use of a CC is written as > > cc-compared-to-0. It means "the result of something compared to 0, > > stored in that cc reg". > > > > (And you can copy a CC reg around, but that is not a use ;-) ) Hm... under this premise, we can also say that any form that is not cc-compared-to-0 is not a use. Consequently, if it is not a use, then the CC reg should not be updated at its use location, so my v1 patch, where we simply skip the update (but retain the combined insn), actually makes sense. In this concrete situation, we don't care about CC register mode in the PUSHFL insn. And we should not change CC reg mode of the use, because any other mode than the generic CCmode won't be recognized by the insn pattern. Uros.
On Thu, Mar 7, 2024 at 11:29 PM Segher Boessenkool <segher@kernel.crashing.org> wrote: > > On Thu, Mar 07, 2024 at 11:07:18PM +0100, Uros Bizjak wrote: > > On Thu, Mar 7, 2024 at 10:37 PM Segher Boessenkool > > <segher@kernel.crashing.org> wrote: > > > > but can be something else, such as the above noted > > > > > > > > (unspec:DI [ > > > > (reg:CC 17 flags) > > > > ] UNSPEC_PUSHFL) > > > > > > But that is invalid RTL? The only valid use of a CC is written as > > > cc-compared-to-0. It means "the result of something compared to 0, > > > stored in that cc reg". > > > > > > (And you can copy a CC reg around, but that is not a use ;-) ) > > > > How can we describe a pushfl then? > > (unspec:DI [ > (compare:CC) ((reg:CC 17 flags) (const_int 0)) > ] UNSPEC_PUSHFL) > > or something like that? > > > It was changed to its current form > > in [1], but I suspect that the code will try to update it even when > > pushfl is implemented as a direct move from a register (as was defined > > previously). > > > > BTW: Unspecs are used in a couple of other places for other targets [2]. > > > > [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112494#c5 > > [2] https://gcc.gnu.org/pipermail/gcc-patches/2023-December/639743.html > > There is nothing wront with unspecs. You cannot use a CCmode value > without comparing it to 0, though. The exact kind of comparison > determines what bits are valid (and have what meaning) in your CC reg, > even! The pushfl can be considered as a transparent move, separate bits have no meaning there. I don't see why using unspec should be any different than using "naked" register (please also see below, current source code may update "naked" reg as well). What constitutes use is "(cmp:CC (CC reg) (const_int 0))" around the register and I think that without this RTX around the CC reg its use should not be updated in any way. > > > > The source code that deals with the *user* of the CC register assumes > > > > the former form, so it blindly tries to update the mode of the CC > > > > register inside LT comparison RTX > > > > > > Would you like it better if there was an assert for this? There are > > > very many RTL requirements that aren't chacked for currently :-/ > > > > In this case - yes. Assert signals that something is unsupported (or > > invalid), way better than silently corrupting some memory, reporting > > the corruption only with checking enabled. > > Yeah. The way RTL checking works makes this hard to do in most cases. > Hrm. (It cannot easily look at context, only inside of the current RTX). > > > > The unspec should have the CC compared with 0 as argument. > > > > But this does not do what pushfl does... It pushes the register to the stack. > > Can't you just describe the dataflow then, without an unspec? An unspec > by definition does some (unspecified) operation on the data. Previously, it was defined as: (define_insn "*pushfl<mode>2" [(set (match_operand:W 0 "push_operand" "=<") (match_operand:W 1 "flags_reg_operand"))] But Wmode, AKA SI/DImode is not CCmode. And as said in my last message, nothing prevents current source code to try to update the CC register here. Uros.
On Thu, Mar 07, 2024 at 11:27:28PM +0100, Uros Bizjak wrote: > On Thu, Mar 7, 2024 at 11:07 PM Uros Bizjak <ubizjak@gmail.com> wrote: > > > > (unspec:DI [ > > > > (reg:CC 17 flags) > > > > ] UNSPEC_PUSHFL) > > > > > > But that is invalid RTL? The only valid use of a CC is written as > > > cc-compared-to-0. It means "the result of something compared to 0, > > > stored in that cc reg". > > > > > > (And you can copy a CC reg around, but that is not a use ;-) ) > > Hm... under this premise, we can also say that any form that is not > cc-compared-to-0 is not a use. Well, no. All uses of CC are written as comparisons to 0, or are pure dataflow. Anything else is not "not a use" but just invalid. > Consequently, if it is not a use, then > the CC reg should not be updated at its use location, so my v1 patch, > where we simply skip the update (but retain the combined insn), > actually makes sense. With more asserts, perhaps. > In this concrete situation, we don't care about CC register mode in > the PUSHFL insn. And we should not change CC reg mode of the use, > because any other mode than the generic CCmode won't be recognized by > the insn pattern. You always care about the CC mode, that is why you always write it as comparison, so the backend can choose a mode based on what the flag bits mean in this context. For an extreme example look at 390, but on pretty much any target both signed and unsigned comparisons use the same flag bits, and maybe fp comparisons as well. But pushfl does sound like pure dataflow. Why is this a builtin, is it ever a good idea if the user writes stuff the compiler can do a better job doing itself, not to mention it is way easier for the compiler anyway? :-) Segher
On Thu, Mar 07, 2024 at 11:46:54PM +0100, Uros Bizjak wrote: > > Can't you just describe the dataflow then, without an unspec? An unspec > > by definition does some (unspecified) operation on the data. > > Previously, it was defined as: > > (define_insn "*pushfl<mode>2" > [(set (match_operand:W 0 "push_operand" "=<") > (match_operand:W 1 "flags_reg_operand"))] > > But Wmode, AKA SI/DImode is not CCmode. And as said in my last > message, nothing prevents current source code to try to update the CC > register here. So you can use an unspec just to convert the flags reg to an integer? To make an integer representation of flags reg contents. Or is that what we started with here? Segher
On Mon, Mar 18, 2024 at 3:46 PM Segher Boessenkool <segher@kernel.crashing.org> wrote: > > On Thu, Mar 07, 2024 at 11:27:28PM +0100, Uros Bizjak wrote: > > On Thu, Mar 7, 2024 at 11:07 PM Uros Bizjak <ubizjak@gmail.com> wrote: > > > > > (unspec:DI [ > > > > > (reg:CC 17 flags) > > > > > ] UNSPEC_PUSHFL) > > > > > > > > But that is invalid RTL? The only valid use of a CC is written as > > > > cc-compared-to-0. It means "the result of something compared to 0, > > > > stored in that cc reg". > > > > > > > > (And you can copy a CC reg around, but that is not a use ;-) ) > > > > Hm... under this premise, we can also say that any form that is not > > cc-compared-to-0 is not a use. > > Well, no. All uses of CC are written as comparisons to 0, or are pure > dataflow. Anything else is not "not a use" but just invalid. > > > Consequently, if it is not a use, then > > the CC reg should not be updated at its use location, so my v1 patch, > > where we simply skip the update (but retain the combined insn), > > actually makes sense. > > With more asserts, perhaps. > > > In this concrete situation, we don't care about CC register mode in > > the PUSHFL insn. And we should not change CC reg mode of the use, > > because any other mode than the generic CCmode won't be recognized by > > the insn pattern. > > You always care about the CC mode, that is why you always write it as > comparison, so the backend can choose a mode based on what the flag bits > mean in this context. For an extreme example look at 390, but on pretty > much any target both signed and unsigned comparisons use the same flag > bits, and maybe fp comparisons as well. After some more thoughts, I came up with v3 [1], where the mode is updated also in non-comparison use. But instead of a blind SUBST, we find the correct use location and update the mode accordingly. If the new mode is not recognized in the use insn, then the whole combination is cancelled. Since x86 pushfl does not care about CCmode, I have changed it to handle all CCmodes. Please note, that with this approach, the backend can choose a mode. [1] https://gcc.gnu.org/pipermail/gcc-patches/2024-March/647634.html > But pushfl does sound like pure dataflow. Why is this a builtin, is > it ever a good idea if the user writes stuff the compiler can do a > better job doing itself, not to mention it is way easier for the > compiler anyway? :-) The mode of the pushfl has to be correct, because it operates on stack. Unfortunately, the plain move can't do this due to WORDmode/CCmode mismatch in the rtx, so UNSPEC is necessary. But it IS a pure dataflow instruction - it is a PUSH. Uros.
On Mon, Mar 18, 2024 at 3:51 PM Segher Boessenkool <segher@kernel.crashing.org> wrote: > > On Thu, Mar 07, 2024 at 11:46:54PM +0100, Uros Bizjak wrote: > > > Can't you just describe the dataflow then, without an unspec? An unspec > > > by definition does some (unspecified) operation on the data. > > > > Previously, it was defined as: > > > > (define_insn "*pushfl<mode>2" > > [(set (match_operand:W 0 "push_operand" "=<") > > (match_operand:W 1 "flags_reg_operand"))] > > > > But Wmode, AKA SI/DImode is not CCmode. And as said in my last > > message, nothing prevents current source code to try to update the CC > > register here. > > So you can use an unspec just to convert the flags reg to an integer? > To make an integer representation of flags reg contents. Yes, this is correct. But please note the v3 patch, where the mode update is made at the correct location. Quote from the patch: Replace cc_use_loc with the entire new RTX only in case cc_use_loc satisfies COMPARISON_P predicate. Otherwise scan the entire cc_use_loc RTX for CC reg to be updated with a new mode. > Or is that what we started with here? The reason for the patch is that when CC reg is used outside comparison RTX, the combine tries to update CC reg mode where it is used after the combined instruction. This happens on extremely rare occasions, but when it happens, combine assumes that it is used exclusively in the comparison RTX and uses "SUBST (XEXP (*cc_use_loc, 0), ...);". XEXP (*cc_use_loc, 0) will segfault when CC reg is referred in a simple SET assignment, not only when it is referred in an UNSPEC. Please note that the comparison RTX is handled a few lines above, where my patch also fixes the "???" issue. Uros.
diff --git a/gcc/combine.cc b/gcc/combine.cc index a4479f8d836..6dac9ffca85 100644 --- a/gcc/combine.cc +++ b/gcc/combine.cc @@ -3184,11 +3184,21 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0, && (cc_use_loc = find_single_use (SET_DEST (newpat), i3, &cc_use_insn))) { - compare_code = orig_compare_code = GET_CODE (*cc_use_loc); - if (is_a <scalar_int_mode> (GET_MODE (i2dest), &mode)) - compare_code = simplify_compare_const (compare_code, mode, - &op0, &op1); - target_canonicalize_comparison (&compare_code, &op0, &op1, 1); + if (COMPARISON_P (*cc_use_loc)) + { + compare_code = orig_compare_code = GET_CODE (*cc_use_loc); + if (is_a <scalar_int_mode> (GET_MODE (i2dest), &mode)) + compare_code = simplify_compare_const (compare_code, mode, + &op0, &op1); + target_canonicalize_comparison (&compare_code, &op0, &op1, 1); + } + else + { + if (dump_file && (dump_flags & TDF_DETAILS)) + fprintf (dump_file, "CC register not used in comparison.\n"); + undo_all (); + return 0; + } } /* Do the rest only if op1 is const0_rtx, which may be the @@ -3221,9 +3231,7 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0, } #endif /* Cases for modifying the CC-using comparison. */ - if (compare_code != orig_compare_code - /* ??? Do we need to verify the zero rtx? */ - && XEXP (*cc_use_loc, 1) == const0_rtx) + if (compare_code != orig_compare_code) { /* Replace cc_use_loc with entire new RTX. */ SUBST (*cc_use_loc,
The compiler, configured with --enable-checking=yes,rtl,extra ICEs with: internal compiler error: RTL check: expected elt 0 type 'e' or 'u', have 'E' (rtx unspec) in try_combine, at combine.cc:3237 This is 3236 /* Just replace the CC reg with a new mode. */ 3237 SUBST (XEXP (*cc_use_loc, 0), newpat_dest); 3238 undobuf.other_insn = cc_use_insn; in combine.cc, where *cc_use_loc is (unspec:DI [ (reg:CC 17 flags) ] UNSPEC_PUSHFL) combine assumes CC must be used inside of a comparison and uses XEXP (..., 0) without checking on the RTX type of the argument. Undo the combination if *cc_use_loc is not COMPARISON_P. Also remove buggy and now redundant check for (const 0) RTX as part of the comparison. PR rtl-optimization/112560 gcc/ChangeLog: * combine.cc (try_combine): Reject the combination if *cc_use_loc is not COMPARISON_P. Bootstrapped and regression tested on x86_64-pc-linux-gnu {,-m32}. OK for trunk? Uros.