On 26/10/2020 06:51, Segher Boessenkool wrote:
> On Mon, Oct 26, 2020 at 11:06:22AM +0000, Alex Coplan wrote:
> > Well, only the low 32 bits of the subreg are valid. But because those
> > low 32 bits are shifted left 2 times, the low 34 bits of the ashift are
> > valid: the bottom 2 bits of the ashift are zeros, and the 32 bits above
> > those are from the inner SImode reg (with the upper 62 bits being
> > undefined).
>
> Ugh. Yes, I think you are right. One more reason why we should only
> use *explicit* sign/zero extends, none of this confusing subreg
> business :-(
Yeah. IIRC expand_compound_operation() introduces the subreg because it
explicitly wants to rewrite the sign_extend using a pair of shifts (without
using an extend rtx). Something like:
(ashiftrt:DI
(ashift:DI
(subreg:DI (reg:SI r) 0)
(const_int 32))
(const_int 32))
>
> > > > diff --git a/gcc/combine.c b/gcc/combine.c
> > > > index c88382efbd3..fe8eff2b464 100644
> > > > --- a/gcc/combine.c
> > > > +++ b/gcc/combine.c
> > > > @@ -7419,8 +7419,8 @@ expand_compound_operation (rtx x)
> > > > }
> > > >
> > > > /* If we reach here, we want to return a pair of shifts. The inner
> > > > - shift is a left shift of BITSIZE - POS - LEN bits. The outer
> > > > - shift is a right shift of BITSIZE - LEN bits. It is arithmetic or
> > > > + shift is a left shift of MODEWIDTH - POS - LEN bits. The outer
> > > > + shift is a right shift of MODEWIDTH - LEN bits. It is arithmetic
> > > > or
> > > > logical depending on the value of UNSIGNEDP.
> > > >
> > > > If this was a ZERO_EXTEND or ZERO_EXTRACT, this pair of shifts
> > > > will be
> > >
> > > MODEWIDTH isn't defined here yet, it is initialised just below to
> > > MODE_PRECISION (mode).
> >
> > Yes, but bitsize isn't defined at all in this function AFAICT. Are
> > comments not permitted to refer to variables defined immediately beneath
> > them?
>
> Of course you can -- comments are free form text after all -- but as
> written it suggest there already is an initialised variable "modewidth".
>
> Just move the initialisation to above this comment?
Sure, see the revised patch attached.
Thanks,
Alex
diff --git a/gcc/combine.c b/gcc/combine.c
index 4782e1d9dcc..d4793c1c575 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -7418,9 +7418,11 @@ expand_compound_operation (rtx x)
}
+ modewidth = GET_MODE_PRECISION (mode);
+
/* If we reach here, we want to return a pair of shifts. The inner
- shift is a left shift of BITSIZE - POS - LEN bits. The outer
- shift is a right shift of BITSIZE - LEN bits. It is arithmetic or
+ shift is a left shift of MODEWIDTH - POS - LEN bits. The outer
+ shift is a right shift of MODEWIDTH - LEN bits. It is arithmetic or
logical depending on the value of UNSIGNEDP.
If this was a ZERO_EXTEND or ZERO_EXTRACT, this pair of shifts will be
@@ -7433,7 +7435,6 @@ expand_compound_operation (rtx x)
extraction. Then the constant of 31 would be substituted in
to produce such a position. */
- modewidth = GET_MODE_PRECISION (mode);
if (modewidth >= pos + len)
{
tem = gen_lowpart (mode, XEXP (x, 0));
@@ -7650,20 +7651,27 @@ make_extraction (machine_mode mode, rtx inner,
HOST_WIDE_INT pos,
is_mode = GET_MODE (SUBREG_REG (inner));
inner = SUBREG_REG (inner);
}
- else if (GET_CODE (inner) == ASHIFT
+ else if ((GET_CODE (inner) == ASHIFT || GET_CODE (inner) == MULT)
&& CONST_INT_P (XEXP (inner, 1))
- && pos_rtx == 0 && pos == 0
- && len > UINTVAL (XEXP (inner, 1)))
- {
- /* We're extracting the least significant bits of an rtx
- (ashift X (const_int C)), where LEN > C. Extract the
- least significant (LEN - C) bits of X, giving an rtx
- whose mode is MODE, then shift it left C times. */
- new_rtx = make_extraction (mode, XEXP (inner, 0),
- 0, 0, len - INTVAL (XEXP (inner, 1)),
- unsignedp, in_dest, in_compare);
- if (new_rtx != 0)
- return gen_rtx_ASHIFT (mode, new_rtx, XEXP (inner, 1));
+ && pos_rtx == 0 && pos == 0)
+ {
+ const HOST_WIDE_INT ci = INTVAL (XEXP (inner, 1));
+ const auto code = GET_CODE (inner);
+ const HOST_WIDE_INT shift_amt = (code == MULT) ? exact_log2 (ci) : ci;
+
+ if (shift_amt > 0 && len > (unsigned HOST_WIDE_INT)shift_amt)
+ {
+ /* We're extracting the least significant bits of an rtx
+ (ashift X (const_int C)) or (mult X (const_int (2^C))),
+ where LEN > C. Extract the least significant (LEN - C) bits
+ of X, giving an rtx whose mode is MODE, then shift it left
+ C times. */
+ new_rtx = make_extraction (mode, XEXP (inner, 0),
+ 0, 0, len - shift_amt,
+ unsignedp, in_dest, in_compare);
+ if (new_rtx)
+ return gen_rtx_fmt_ee (code, mode, new_rtx, XEXP (inner, 1));
+ }
}
else if (GET_CODE (inner) == TRUNCATE
/* If trying or potentionally trying to extract
