Dimitar Dimitrov <dimi...@dinux.eu> writes:
> 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));
Ah, right, I missed that one. I think that too should be guarded
by known_le (osize, isize), since it will almost inevitably fail
for paradoxical subregs of mems on strict-alignment targets, and
doesn't make conceptual sense for them.
Thanks,
Richard
