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

[Richard Biener <richard.guenther@gmail.com>]

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.