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.guent...@gmail.com> wrote:
>
> On Wed, Nov 29, 2023 at 10:35 AM Uros Bizjak <ubiz...@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.

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