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

[Uros Bizjak <ubizjak@gmail.com>]

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.