Dimitar Dimitrov <dimi...@dinux.eu> writes:
> When a paradoxical subreg is detected, validate_subreg exits early, thus
> skipping the important checks later in the function.
>
> Fix by continuing with the checks instead of declaring early that the
> paradoxical subreg is valid.
>
> One of the newly allowed subsequent checks needed to be disabled for
> paradoxical subregs. It turned out that combine attempts to create
> a paradoxical subreg of mem even for strict-alignment targets.
> That is invalid and should eventually be rejected, but is
> temporarily left allowed to prevent regressions for
> armv8l-unknown-linux-gnueabihf.
>
> Tests I did:
> - No regressions were found for C and C++ for the following targets:
> - native x86_64-pc-linux-gnu
> - cross riscv64-unknown-linux-gnu
> - cross riscv32-none-elf
> - Sanity checked armv8l-unknown-linux-gnueabihf by cross-building
> up to including libgcc. I'll monitor Linaro CI bot for the
> full regression test results.
> - Sanity checked powerpc64-unknown-linux-gnu by building native
> toolchain, but could not setup qemu-user for DejaGnu testing.
>
> PR target/119966
>
> gcc/ChangeLog:
>
> * emit-rtl.cc (validate_subreg): Do not exit immediately for
> paradoxical subregs. Filter subsequent tests which are
> not valid for paradoxical subregs.
>
> Co-authored-by: Richard Sandiford <richard.sandif...@arm.com>
> Signed-off-by: Dimitar Dimitrov <dimi...@dinux.eu>
> ---
> gcc/emit-rtl.cc | 25 ++++++++++++++++++-------
> 1 file changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/gcc/emit-rtl.cc b/gcc/emit-rtl.cc
> index 3e2c4309dee..e46b0f9eac4 100644
> --- a/gcc/emit-rtl.cc
> +++ b/gcc/emit-rtl.cc
> @@ -969,10 +969,10 @@ validate_subreg (machine_mode omode, machine_mode imode,
> }
>
> /* Paradoxical subregs must have offset zero. */
> - if (maybe_gt (osize, isize))
> - return known_eq (offset, 0U);
> + if (maybe_gt (osize, isize) && !known_eq (offset, 0U))
> + return false;
>
> - /* This is a normal subreg. Verify that the offset is representable. */
> + /* Verify that the offset is representable. */
>
> /* For hard registers, we already have most of these rules collected in
> subreg_offset_representable_p. */
> @@ -988,9 +988,13 @@ validate_subreg (machine_mode omode, machine_mode imode,
>
> return subreg_offset_representable_p (regno, imode, offset, omode);
> }
> - /* Do not allow SUBREG with stricter alignment than the inner MEM. */
> + /* Do not allow normal SUBREG with stricter alignment than the inner MEM.
> +
> + FIXME: Combine can create paradoxical mem subregs even for
> + strict-alignment targets. Allow it until combine is fixed. */
Are the details captured in bugzilla somewhere? If not, could you file
a PR and explain when this happens, or add a comment to PR119966?
I think this should have a reference to a particular bugzilla comment
that describes the problem, otherwise it would be hard to tell later
whether the problem has been fixed.
OK with that change, thanks.
Richard
> else if (reg && MEM_P (reg) && STRICT_ALIGNMENT
> - && MEM_ALIGN (reg) < GET_MODE_ALIGNMENT (omode))
> + && MEM_ALIGN (reg) < GET_MODE_ALIGNMENT (omode)
> + && known_le (osize, isize))
> return false;
>
> /* The outer size must be ordered wrt the register size, otherwise
> @@ -999,7 +1003,7 @@ validate_subreg (machine_mode omode, machine_mode imode,
> if (!ordered_p (osize, regsize))
> return false;
>
> - /* For pseudo registers, we want most of the same checks. Namely:
> + /* For normal pseudo registers, we want most of the same checks. Namely:
>
> Assume that the pseudo register will be allocated to hard registers
> that can hold REGSIZE bytes each. If OSIZE is not a multiple of
> REGSIZE,
> @@ -1008,8 +1012,15 @@ validate_subreg (machine_mode omode, machine_mode
> imode,
> otherwise it is at the lowest offset.
>
> Given that we've already checked the mode and offset alignment,
> - we only have to check subblock subregs here. */
> + we only have to check subblock subregs here.
> +
> + For paradoxical little-endian registers, this check is redundant. The
> + offset has already been validated to be zero.
> +
> + For paradoxical big-endian registers, this check is not valid
> + because the offset is zero. */
> if (maybe_lt (osize, regsize)
> + && known_le (osize, isize)
> && ! (lra_in_progress && (FLOAT_MODE_P (imode) || FLOAT_MODE_P
> (omode))))
> {
> /* It is invalid for the target to pick a register size for a mode
