Hi Segher,
The 01/31/2020 12:13, Segher Boessenkool wrote:
> Hi!
>
> On Fri, Jan 31, 2020 at 10:12:08AM +0000, Tamar Christina wrote:
> > This fixes a fall-out from a patch I had submitted two years ago which
> > started
> > allowing simplify-rtx to fold logical right shifts by offsets a followed by
> > b
> > into >> (a + b).
> >
> > However this can generate inefficient code when the resulting shift count
> > ends
> > up being the same as the size of the shift mode. This will create some
> > undefined behavior on most platforms.
> >
> > This patch changes to code to truncate to 0 if the shift amount goes out of
>
> > Note that this doesn't take care of the Arithmetic shift where you could
> > replace
> > the constant with MODE_BITS (mode) - 1, but that's not a regression so
> > punting it.
>
> Aww :-)
>
> > 2020-01-31 Tamar Christina <tamar.christ...@arm.com>
> >
> > PR 91838
> > * simplify-rtx.c (simplify_binary_operation_1): Update LSHIFTRT case
> > to truncate if allowed or reject combination.
>
> PR rtl-optimization/91838 please? But the bug is currently set as a
> target bug; looks like it should be fixed in bugzilla as well.
>
> > diff --git a/gcc/simplify-rtx.c b/gcc/simplify-rtx.c
> > index
> > eff1d07a2533c7bda5f0529cd318f08e6d5209d6..543cd5885105fb0e4568568a3c876c74cc1068bf
> > 100644
> > --- a/gcc/simplify-rtx.c
> > +++ b/gcc/simplify-rtx.c
> > @@ -3647,9 +3647,21 @@ simplify_binary_operation_1 (enum rtx_code code,
> > machine_mode mode,
> > {
> > rtx tmp = gen_int_shift_amount
> > (inner_mode, INTVAL (XEXP (SUBREG_REG (op0), 1)) + INTVAL (op1));
> > - tmp = simplify_gen_binary (code, inner_mode,
> > - XEXP (SUBREG_REG (op0), 0),
> > - tmp);
> > +
> > + /* Combine would usually zero out the value when combining two
> > + local shifts and the range becomes larger or equal to the mode.
> > + However since we fold away one of the shifts here combine won't
> > + see it so we should immediately truncate the shift if it's out of
> > + range. */
>
> "Truncate the shift" is confusing. "Zero the result"?
>
> > + if (code == LSHIFTRT
> > + && INTVAL (tmp) >= GET_MODE_BITSIZE (inner_mode))
> > + tmp = const0_rtx;
> > + else
> > + tmp = simplify_gen_binary (code,
> > + inner_mode,
> > + XEXP (SUBREG_REG (op0), 0),
> > + tmp);
> > +
> > return lowpart_subreg (int_mode, tmp, inner_mode);
> > }
>
> Should this be done for at least left shifts, too?
>
The simplification does indeed only happen for right shifts, I'll open a PR for
supporting the
arithmetic shifts and also left shift and will add those once we're add stage 1
again.
Thanks,
Tamar
> Looks good with those things fixed.
>
>
> Segher
--
