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

-- 

Reply via email to