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

[Uros Bizjak <ubizjak@gmail.com>]

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
>