On Mon, Dec 4, 2023 at 10:34 AM Uros Bizjak <ubiz...@gmail.com> wrote:
>
> 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 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.
