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

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

[Uros Bizjak]

[-- Attachment #1: Type: text/plain, Size: 944 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.

Skip the modification of CC-using operation if *cc_use_loc is not COMPARISON_P.

    PR middle-end/112560

gcc/ChangeLog:

    * combine.cc (try_combine): Skip the modification of CC-using
    operation if *cc_use_loc is not COMPARISON_P.

Bootstrapped and regression tested on x86_64-linux-gnu {,-m32}.

OK for master?

Uros.

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

diff --git a/gcc/combine.cc b/gcc/combine.cc
index 6344cd3c9f2..a270138b59f 100644
--- a/gcc/combine.cc
+++ b/gcc/combine.cc
@@ -3184,11 +3184,16 @@ 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
+	    cc_use_loc = NULL;
 	}
 
       /* Do the rest only if op1 is const0_rtx, which may be the

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

[Richard Biener]

On Wed, Nov 29, 2023 at 10:35 AM Uros Bizjak <ubizjak@gmail.com> 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.
>
> Skip the modification of CC-using operation if *cc_use_loc is not COMPARISON_P.
>
>     PR middle-end/112560
>
> gcc/ChangeLog:
>
>     * combine.cc (try_combine): Skip the modification of CC-using
>     operation if *cc_use_loc is not COMPARISON_P.
>
> Bootstrapped and regression tested on x86_64-linux-gnu {,-m32}.
>
> OK for master?

Don't we need to stop the attempt to combine when we cannot handle a use?
Simply not adjusting another use doesn't look correct, does it?

Richard.

>
> Uros.

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

[Uros Bizjak]

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

On Wed, Nov 29, 2023 at 1:25 PM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Wed, Nov 29, 2023 at 10:35 AM Uros Bizjak <ubizjak@gmail.com> 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.
> >
> > Skip the modification of CC-using operation if *cc_use_loc is not COMPARISON_P.
> >
> >     PR middle-end/112560
> >
> > gcc/ChangeLog:
> >
> >     * combine.cc (try_combine): Skip the modification of CC-using
> >     operation if *cc_use_loc is not COMPARISON_P.
> >
> > Bootstrapped and regression tested on x86_64-linux-gnu {,-m32}.
> >
> > OK for master?
>
> Don't we need to stop the attempt to combine when we cannot handle a use?
> Simply not adjusting another use doesn't look correct, does it?

I was assuming that if the CC reg is not used inside the comparison,
then the mode of CC reg is irrelevant. We can still combine the
instructions into new insn, without updating the use of CC reg.

In this particular case, the combined insn is rejected, but
UNSPEC_PUSHFL does not care about the mode of the CC reg and would
handle combined insn just fine.

Alternatively, the attached patch skips the combination altogether.

Thanks,
Uros.

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

diff --git a/gcc/combine.cc b/gcc/combine.cc
index 6344cd3c9f2..e533631d0e6 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

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

[Segher Boessenkool]

Hi!

On Wed, Nov 29, 2023 at 02:20:03PM +0100, Uros Bizjak wrote:
> On Wed, Nov 29, 2023 at 1:25 PM Richard Biener
> <richard.guenther@gmail.com> wrote:
> > On Wed, Nov 29, 2023 at 10:35 AM Uros Bizjak <ubizjak@gmail.com> wrote:
> I was assuming that if the CC reg is not used inside the comparison,
> then the mode of CC reg is irrelevant. We can still combine the
> instructions into new insn, without updating the use of CC reg.

It should never happen that the CC reg is not used, so what does this
mean?

Where it is used might not be a comparison of course, as in your
example.

>  	  && (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");

"Where the CCmode register is used is not a comparison".  But more
compact if you can manage (I cannot).


Segher

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

[Uros Bizjak]

On Thu, Nov 30, 2023 at 9:21 AM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> Hi!
>
> On Wed, Nov 29, 2023 at 02:20:03PM +0100, Uros Bizjak wrote:
> > On Wed, Nov 29, 2023 at 1:25 PM Richard Biener
> > <richard.guenther@gmail.com> wrote:
> > > On Wed, Nov 29, 2023 at 10:35 AM Uros Bizjak <ubizjak@gmail.com> wrote:
> > I was assuming that if the CC reg is not used inside the comparison,
> > then the mode of CC reg is irrelevant. We can still combine the
> > instructions into new insn, without updating the use of CC reg.
>
> It should never happen that the CC reg is not used, so what does this
> mean?

I was trying to say that when CC reg is used inside the comparison, 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)]))]
  ""
  "set%C1\t%0"

and through the%C operand modifier we call put_condition_code, the
mode of CC reg is checked and a different instruction is emitted,
depending on the CC reg mode. Please also note that not all comparison
operators are handled, so when the combine rewrites the comparison, it
isn't necessarily supported.

This is in contrast with:

(define_insn "@pushfl<mode>2"
  [(set (match_operand:W 0 "push_operand" "=<")
    (unspec:W [(match_operand:CC 1 "flags_reg_operand")]
          UNSPEC_PUSHFL))]

where we don't care about CC reg mode at all. The CC reg is not in the
comparison, so the mode does not matter. The combination of
instructions should not be limited by CC reg user in this case.

>
> Where it is used might not be a comparison of course, as in your
> example.
>
> >         && (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");
>
> "Where the CCmode register is used is not a comparison".  But more
> compact if you can manage (I cannot).

Perhaps "CCmode register user is not a comparison." ?

Uros.

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

[Uros Bizjak]

On Wed, Nov 29, 2023 at 1:25 PM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Wed, Nov 29, 2023 at 10:35 AM Uros Bizjak <ubizjak@gmail.com> 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.
> >
> > Skip the modification of CC-using operation if *cc_use_loc is not COMPARISON_P.
> >
> >     PR middle-end/112560
> >
> > gcc/ChangeLog:
> >
> >     * combine.cc (try_combine): Skip the modification of CC-using
> >     operation if *cc_use_loc is not COMPARISON_P.
> >
> > Bootstrapped and regression tested on x86_64-linux-gnu {,-m32}.
> >
> > OK for master?
>
> Don't we need to stop the attempt to combine when we cannot handle a use?
> Simply not adjusting another use doesn't look correct, does it?

I have analysed [1] all targets that define SELECT_CC_MODE, and almost
all use CC_REG exclusively in comparison. Besides i386 that defines:

(define_insn "@pushfl<mode>2"
  [(set (match_operand:W 0 "push_operand" "=<")
    (unspec:W [(match_operand:CC 1 "flags_reg_operand")]
          UNSPEC_PUSHFL))]

other non-comparison pre-reload uses of CC_REG are:

arm:

(define_insn "get_fpscr_nzcvqc"
 [(set (match_operand:SI 0 "register_operand" "=r")
   (unspec_volatile:SI [(reg:SI VFPCC_REGNUM)] UNSPEC_GET_FPSCR_NZCVQC))]

(define_insn "mve_vadcq_<supf>v4si"
  [(set (match_operand:V4SI 0 "s_register_operand" "=w")
    (unspec:V4SI [(match_operand:V4SI 1 "s_register_operand" "w")
               (match_operand:V4SI 2 "s_register_operand" "w")]
     VADCQ))
   (set (reg:SI VFPCC_REGNUM)
    (unspec:SI [(reg:SI VFPCC_REGNUM)]
     VADCQ))
  ]

rs6000:

(define_insn "prologue_movesi_from_cr"
  [(set (match_operand:SI 0 "gpc_reg_operand" "=r")
        (unspec:SI [(reg:CC CR2_REGNO) (reg:CC CR3_REGNO)
            (reg:CC CR4_REGNO)]
           UNSPEC_MOVESI_FROM_CR))]

and just for reference s390:

(define_insn_and_split "*ccraw_to_int"
  [(set (pc)
    (if_then_else
     (match_operator 0 "s390_eqne_operator"
             [(reg:CCRAW CC_REGNUM)
              (match_operand 1 "const_int_operand" "")])
     (label_ref (match_operand 2 "" ""))
     (pc)))
   (set (match_operand:SI 3 "register_operand" "=d")
    (unspec:SI [(reg:CCRAW CC_REGNUM)] UNSPEC_CC_TO_INT))]

The above is not single_use, so the issue does not apply here.

These uses can all break with checking enabled at the mentioned spot
in combine.cc in the same way as x86. Actually, it is undesirable to
change the mode in the "other instruction" - the machine instruction
doesn't care about mode at all, but the insn pattern may fail
recognition due to CC mode change.

Based on the above analysis, I propose we proceed with my original patch.

[1] Starting with 'egrep -v -w "set|clobber" *.md | grep <CC_REG>' and
analysing all hits

Uros.

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

[Richard Biener]

On Mon, Dec 4, 2023 at 10:34 AM Uros Bizjak <ubizjak@gmail.com> wrote:
>
> On Wed, Nov 29, 2023 at 1:25 PM Richard Biener
> <richard.guenther@gmail.com> wrote:
> >
> > On Wed, Nov 29, 2023 at 10:35 AM Uros Bizjak <ubizjak@gmail.com> 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.
> > >
> > > Skip the modification of CC-using operation if *cc_use_loc is not COMPARISON_P.
> > >
> > >     PR middle-end/112560
> > >
> > > gcc/ChangeLog:
> > >
> > >     * combine.cc (try_combine): Skip the modification of CC-using
> > >     operation if *cc_use_loc is not COMPARISON_P.
> > >
> > > Bootstrapped and regression tested on x86_64-linux-gnu {,-m32}.
> > >
> > > OK for master?
> >
> > Don't we need to stop the attempt to combine when we cannot handle a use?
> > Simply not adjusting another use doesn't look correct, does it?
>
> I have analysed [1] all targets that define SELECT_CC_MODE, and almost
> all use CC_REG exclusively in comparison. Besides i386 that defines:
>
> (define_insn "@pushfl<mode>2"
>   [(set (match_operand:W 0 "push_operand" "=<")
>     (unspec:W [(match_operand:CC 1 "flags_reg_operand")]
>           UNSPEC_PUSHFL))]
>
> other non-comparison pre-reload uses of CC_REG are:
>
> arm:
>
> (define_insn "get_fpscr_nzcvqc"
>  [(set (match_operand:SI 0 "register_operand" "=r")
>    (unspec_volatile:SI [(reg:SI VFPCC_REGNUM)] UNSPEC_GET_FPSCR_NZCVQC))]
>
> (define_insn "mve_vadcq_<supf>v4si"
>   [(set (match_operand:V4SI 0 "s_register_operand" "=w")
>     (unspec:V4SI [(match_operand:V4SI 1 "s_register_operand" "w")
>                (match_operand:V4SI 2 "s_register_operand" "w")]
>      VADCQ))
>    (set (reg:SI VFPCC_REGNUM)
>     (unspec:SI [(reg:SI VFPCC_REGNUM)]
>      VADCQ))
>   ]
>
> rs6000:
>
> (define_insn "prologue_movesi_from_cr"
>   [(set (match_operand:SI 0 "gpc_reg_operand" "=r")
>         (unspec:SI [(reg:CC CR2_REGNO) (reg:CC CR3_REGNO)
>             (reg:CC CR4_REGNO)]
>            UNSPEC_MOVESI_FROM_CR))]
>
> and just for reference s390:
>
> (define_insn_and_split "*ccraw_to_int"
>   [(set (pc)
>     (if_then_else
>      (match_operator 0 "s390_eqne_operator"
>              [(reg:CCRAW CC_REGNUM)
>               (match_operand 1 "const_int_operand" "")])
>      (label_ref (match_operand 2 "" ""))
>      (pc)))
>    (set (match_operand:SI 3 "register_operand" "=d")
>     (unspec:SI [(reg:CCRAW CC_REGNUM)] UNSPEC_CC_TO_INT))]
>
> The above is not single_use, so the issue does not apply here.
>
> These uses can all break with checking enabled at the mentioned spot
> in combine.cc in the same way as x86. Actually, it is undesirable to
> change the mode in the "other instruction" - the machine instruction
> doesn't care about mode at all, but the insn pattern may fail
> recognition due to CC mode change.
>
> Based on the above analysis, I propose we proceed with my original patch.

I'm not familiar enough with combine nor CC reg uses to approve the patch
(I just wanted to point out what I noticed when browsing patches).  I'll defer
to Segher for approval.

Thanks,
Richard.

> [1] Starting with 'egrep -v -w "set|clobber" *.md | grep <CC_REG>' and
> analysing all hits
>
> Uros.