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

[Uros Bizjak <ubizjak@gmail.com>]

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.