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 --