> -----Original Message-----
> From: Richard Biener <richard.guent...@gmail.com>
> Sent: Thursday, December 14, 2023 5:23 AM
> To: Andrew Pinski (QUIC) <quic_apin...@quicinc.com>
> Cc: gcc-patches@gcc.gnu.org
> Subject: Re: [PATCH] middle-end: Fix up constant handling in
> emit_conditional_move [PR111260]
>
> On Wed, Dec 13, 2023 at 5:51 PM Andrew Pinski
> <quic_apin...@quicinc.com> wrote:
> >
> > After r14-2667-gceae1400cf24f329393e96dd9720, we force a constant to
> a
> > register if it is shared with one of the other operands. The problem
> > is used the comparison mode for the register but that could be
> > different from the operand mode. This causes some issues on some targets.
> > To fix it, we either need to have the modes match or if it is an
> > integer mode, then we can use the lower part for the smaller mode.
> >
> > Bootstrapped and tested on both aarch64-linux-gnu and x86_64-linux.
>
> I think to fulfil the original purpose requiring matching modes is enough, the
> x86 backend checks for equality here, a subreg wouldn't be enough.
> In fact the whole point preserving equality doesn't work then.
>
> So please just check the modes are equal (I also see I used 'mode' for the
> cost
> check - I've really split out the check done by prepare_cmp_insn here btw).
> This seemed to be the simplest solution at the time, rather than for example
> trying to postpone legitimizing the constants (so rtx_equal_p could continue
> to be lazy with slight mode mismatches).
I was originally deciding between that patch and what this one. I decided to
go with this
one in the end as I was trying to keep the extra optimization. Anyways I test
and commit
the one with just the checking of the modes to be equal.
Also it is not that rtx_equal_p is lazy on mode mismatches but rather all
CONST_INT
don't have a mode (all have VOIDmode); this is why you have to pass around the
cmpmode
and the outer mode of the conditional.
Thanks,
Andrew
>
> Richard.
>
> > PR middle-end/111260
> >
> > gcc/ChangeLog:
> >
> > * optabs.cc (emit_conditional_move): Fix up mode handling for
> > forcing the constant to a register.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * gcc.c-torture/compile/condmove-1.c: New test.
> >
> > Signed-off-by: Andrew Pinski <quic_apin...@quicinc.com>
> > ---
> > gcc/optabs.cc | 40 +++++++++++++++++--
> > .../gcc.c-torture/compile/condmove-1.c | 9 +++++
> > 2 files changed, 45 insertions(+), 4 deletions(-) create mode 100644
> > gcc/testsuite/gcc.c-torture/compile/condmove-1.c
> >
> > diff --git a/gcc/optabs.cc b/gcc/optabs.cc index
> > f0a048a6bdb..573cf22760e 100644
> > --- a/gcc/optabs.cc
> > +++ b/gcc/optabs.cc
> > @@ -5131,26 +5131,58 @@ emit_conditional_move (rtx target, struct
> rtx_comparison comp,
> > /* If we are optimizing, force expensive constants into a register
> > but preserve an eventual equality with op2/op3. */
> > if (CONSTANT_P (orig_op0) && optimize
> > + && (cmpmode == mode
> > + || (GET_MODE_CLASS (cmpmode) == MODE_INT
> > + && GET_MODE_CLASS (mode) == MODE_INT))
> > && (rtx_cost (orig_op0, mode, COMPARE, 0,
> > optimize_insn_for_speed_p ())
> > > COSTS_N_INSNS (1))
> > && can_create_pseudo_p ())
> > {
> > + machine_mode new_mode;
> > + if (known_le (GET_MODE_PRECISION (cmpmode),
> GET_MODE_PRECISION (mode)))
> > + new_mode = mode;
> > + else
> > + new_mode = cmpmode;
> > if (rtx_equal_p (orig_op0, op2))
> > - op2p = XEXP (comparison, 0) = force_reg (cmpmode, orig_op0);
> > + {
> > + rtx r = force_reg (new_mode, orig_op0);
> > + op2p = gen_lowpart (mode, r);
> > + XEXP (comparison, 0) = gen_lowpart (cmpmode, r);
> > + }
> > else if (rtx_equal_p (orig_op0, op3))
> > - op3p = XEXP (comparison, 0) = force_reg (cmpmode, orig_op0);
> > + {
> > + rtx r = force_reg (new_mode, orig_op0);
> > + op3p = gen_lowpart (mode, r);
> > + XEXP (comparison, 0) = gen_lowpart (cmpmode, r);
> > + }
> > }
> > if (CONSTANT_P (orig_op1) && optimize
> > + && (cmpmode == mode
> > + || (GET_MODE_CLASS (cmpmode) == MODE_INT
> > + && GET_MODE_CLASS (mode) == MODE_INT))
> > && (rtx_cost (orig_op1, mode, COMPARE, 0,
> > optimize_insn_for_speed_p ())
> > > COSTS_N_INSNS (1))
> > && can_create_pseudo_p ())
> > {
> > + machine_mode new_mode;
> > + if (known_le (GET_MODE_PRECISION (cmpmode),
> GET_MODE_PRECISION (mode)))
> > + new_mode = mode;
> > + else
> > + new_mode = cmpmode;
> > if (rtx_equal_p (orig_op1, op2))
> > - op2p = XEXP (comparison, 1) = force_reg (cmpmode, orig_op1);
> > + {
> > + rtx r = force_reg (new_mode, orig_op1);
> > + op2p = gen_lowpart (mode, r);
> > + XEXP (comparison, 1) = gen_lowpart (cmpmode, r);
> > + }
> > else if (rtx_equal_p (orig_op1, op3))
> > - op3p = XEXP (comparison, 1) = force_reg (cmpmode, orig_op1);
> > + {
> > + rtx r = force_reg (new_mode, orig_op1);
> > + op3p = gen_lowpart (mode, r);
> > + XEXP (comparison, 1) = gen_lowpart (cmpmode, r);
> > + }
> > }
> > prepare_cmp_insn (XEXP (comparison, 0), XEXP (comparison, 1),
> > GET_CODE (comparison), NULL_RTX,
> > unsignedp, diff --git
> > a/gcc/testsuite/gcc.c-torture/compile/condmove-1.c
> > b/gcc/testsuite/gcc.c-torture/compile/condmove-1.c
> > new file mode 100644
> > index 00000000000..3fcc591af00
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.c-torture/compile/condmove-1.c
> > @@ -0,0 +1,9 @@
> > +/* PR middle-end/111260 */
> > +
> > +/* Used to ICE while expansion of the `(a == b) ? b : 0;` */ int
> > +f1(long long a) {
> > + int b = 822920;
> > + int t = a == b;
> > + return t * (int)b;
> > +}
> > --
> > 2.39.3
> >
