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?
Looks good with those things fixed.
Segher
