On Tue, May 06, 2025 at 01:17:40PM +0100, Richard Sandiford wrote:
> Dimitar Dimitrov <dimi...@dinux.eu> writes:
> > After r16-160-ge6f89d78c1a752, late_combine2 started transforming the
> > following RTL for pru-unknown-elf:
> >
> > (insn 3949 3948 3951 255 (set (reg:QI 56 r14.b0 [orig:1856 _619 ] [1856])
> > (and:QI (reg:QI 1 r0.b1 [orig:1855 _201 ] [1855])
> > (const_int 3 [0x3])))
> > (nil))
> > ...
> > (insn 3961 7067 3962 255 (set (reg:SI 56 r14.b0)
> > (zero_extend:SI (reg:QI 56 r14.b0 [orig:1856 _619 ] [1856])))
> > (nil))
> >
> > into:
> >
> > (insn 3961 7067 3962 255 (set (reg:SI 56 r14.b0)
> > (and:SI (subreg:SI (reg:QI 1 r0.b1 [orig:1855 _201 ] [1855]) 0)
> > (const_int 3 [0x3])))
> > (nil))
> >
> > That caused libbacktrace build to break for pru-unknown-elf. Register
> > r0.b1 (regno 1) is not valid for SImode, which validate_subreg failed to
> > reject.
> >
> > Fix by calling HARD_REGNO_MODE_OK to ensure that both inner and outer
> > modes are valid for the hardware subreg. Remove the premature "success"
> > return for paradoxical subregs, in order to allow subsequent validity
> > checks to be executed.
> >
> > This patch fixes the broken PRU toolchain build. It leaves only two
> > test case regressions for PRU, caused by rnreg pass renaming a valid
> > paradoxical subreg into an invalid one.
> > gcc.c-torture/execute/20040709-1.c
> > gcc.c-torture/execute/20040709-2.c
> > I consider these two a separate issue.
> >
> > I ensured that test results with and without this patch for
> > x86_64-pc-linux-gnu are the same for C and C++.
> >
> > Ok for trunk?
> >
> > PR target/119966
> >
> > gcc/ChangeLog:
> >
> > * emit-rtl.cc (validate_subreg): Do not exit immediately for
> > paradoxical subregs. Add mode checks for validity of
> > hardware subregs.
> >
> > Signed-off-by: Dimitar Dimitrov <dimi...@dinux.eu>
> > ---
> > gcc/emit-rtl.cc | 9 ++++++---
> > 1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/gcc/emit-rtl.cc b/gcc/emit-rtl.cc
> > index 3e2c4309dee..d63543038bb 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. */
>
> This part seems fine, but...
Linaro CI bot notified me that this chunk caused ICE regression for
armv8l-unknown-linux-gnueabihf. Combine creates paradoxical MEM
subregs, which fail the validate_subreg check a few lines below:
/* Do not allow SUBREG with stricter alignment than the inner MEM. */
else if (reg && MEM_P (reg) && STRICT_ALIGNMENT
&& MEM_ALIGN (reg) < GET_MODE_ALIGNMENT (omode))
return false;
And then the following assert is triggered:
rtx
gen_rtx_SUBREG (machine_mode mode, rtx reg, poly_uint64 offset)
{
gcc_assert (validate_subreg (mode, GET_MODE (reg), reg, offset));
If I understand the documentation correctly, this is an issue with
combine. A subreg of mem should have never been created for aarch64
since it defines INSN_SCHEDULING.
>
> > @@ -983,6 +983,9 @@ validate_subreg (machine_mode omode, machine_mode imode,
> > if ((COMPLEX_MODE_P (imode) || VECTOR_MODE_P (imode))
> > && GET_MODE_INNER (imode) == omode)
> > ;
> > + else if (!targetm.hard_regno_mode_ok (regno, imode)
> > + || !targetm.hard_regno_mode_ok (regno, omode))
> > + return false;
> > else if (!REG_CAN_CHANGE_MODE_P (regno, imode, omode))
> > return false;
>
> ...this is IMO a logically separate change...
I'll send this chunk as a separate patch since it is required to
fix the regression for pru-unknown-elf.
> ...Instead, I think we want
> to add known_le (osize, isize) to:
>
> if (maybe_lt (osize, regsize)
> && ! (lra_in_progress && (FLOAT_MODE_P (imode) || FLOAT_MODE_P
> (omode))))
>
> since the test is not correct for big-endian paradoxical subregs,
> and is redundant for little-endian paradoxical subregs.
>
That change indeed fixes libgcc build for powerpc64-unknown-linux-gnu.
I'll add it in the next patch revision.
Regards,
Dimitar
> Thanks,
> Richard
