[PATCH v2] combine: Fix ICE in try_combine on pr112494.c [PR112560]

[PATCH v2] combine: Fix ICE in try_combine on pr112494.c [PR112560]

[Uros Bizjak]

[-- Attachment #1: Type: text/plain, Size: 996 bytes --]

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.

[-- Attachment #2: p.diff.txt --]
[-- Type: text/plain, Size: 1617 bytes --]

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,

Re: [PATCH v2] combine: Fix ICE in try_combine on pr112494.c [PR112560]

[Richard Biener]

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

Re: [PATCH v2] combine: Fix ICE in try_combine on pr112494.c [PR112560]

[Uros Bizjak]

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
>

Re: [PATCH v2] combine: Fix ICE in try_combine on pr112494.c [PR112560]

[Richard Biener]

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
> >
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)

Re: [PATCH v2] combine: Fix ICE in try_combine on pr112494.c [PR112560]

[Segher Boessenkool]

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

Re: [PATCH v2] combine: Fix ICE in try_combine on pr112494.c [PR112560]

[Jakub Jelinek]

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

Re: [PATCH v2] combine: Fix ICE in try_combine on pr112494.c [PR112560]

[Richard Biener]

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.

Re: [PATCH v2] combine: Fix ICE in try_combine on pr112494.c [PR112560]

[Uros Bizjak]

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,

Re: [PATCH v2] combine: Fix ICE in try_combine on pr112494.c [PR112560]

[Segher Boessenkool]

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

Re: [PATCH v2] combine: Fix ICE in try_combine on pr112494.c [PR112560]

[Segher Boessenkool]

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

Re: [PATCH v2] combine: Fix ICE in try_combine on pr112494.c [PR112560]

[Uros Bizjak]

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.

Re: [PATCH v2] combine: Fix ICE in try_combine on pr112494.c [PR112560]

[Segher Boessenkool]

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

Re: [PATCH v2] combine: Fix ICE in try_combine on pr112494.c [PR112560]

[Uros Bizjak]

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.

Re: [PATCH v2] combine: Fix ICE in try_combine on pr112494.c [PR112560]

[Uros Bizjak]

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.

Re: [PATCH v2] combine: Fix ICE in try_combine on pr112494.c [PR112560]

[Segher Boessenkool]

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

Re: [PATCH v2] combine: Fix ICE in try_combine on pr112494.c [PR112560]

[Uros Bizjak]

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.

Re: [PATCH v2] combine: Fix ICE in try_combine on pr112494.c [PR112560]

[Segher Boessenkool]

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

Re: [PATCH v2] combine: Fix ICE in try_combine on pr112494.c [PR112560]

[Uros Bizjak]

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.

Re: [PATCH v2] combine: Fix ICE in try_combine on pr112494.c [PR112560]

[Segher Boessenkool]

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

Re: [PATCH v2] combine: Fix ICE in try_combine on pr112494.c [PR112560]

[Uros Bizjak]

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.

Re: [PATCH v2] combine: Fix ICE in try_combine on pr112494.c [PR112560]

[Uros Bizjak]

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.

Re: [PATCH v2] combine: Fix ICE in try_combine on pr112494.c [PR112560]

[Segher Boessenkool]

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

Re: [PATCH v2] combine: Fix ICE in try_combine on pr112494.c [PR112560]

[Uros Bizjak]

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.