Re: [gcc-15 2/3] RISC-V: avoid LUI based const mat: keep stack offsets aligned

[Andrew Waterman <andrew@sifive.com>]

On Tue, Mar 19, 2024 at 1:05 PM Vineet Gupta <vineetg@rivosinc.com> wrote:
>
>
>
> On 3/19/24 06:10, Jeff Law wrote:
> > On 3/19/24 12:48 AM, Andrew Waterman wrote:
> >> On Mon, Mar 18, 2024 at 5:28 PM Vineet Gupta <vineetg@rivosinc.com> wrote:
> >>> On 3/16/24 13:21, Jeff Law wrote:
> >>>> |   59944:    add     s0,sp,2047  <----
> >>>> |   59948:    mv      a2,a0
> >>>> |   5994c:    mv      a3,a1
> >>>> |   59950:    mv      a0,sp
> >>>> |   59954:    li      a4,1
> >>>> |   59958:    lui     a1,0x1
> >>>> |   5995c:    add     s0,s0,1     <---
> >>>> |   59960:    jal     59a3c
> >>>>
> >>>> SP here becomes unaligned, even if transitively which is undesirable as
> >>>> well as incorrect:
> >>>>    - ABI requires stack to be 8 byte aligned
> >>>>    - asm code looks weird and unexpected
> >>>>    - to the user it might falsely seem like a compiler bug even when not,
> >>>>      specially when staring at asm for debugging unrelated issue.
> >>>> It's not ideal, but I think it's still ABI compliant as-is.  If it
> >>>> wasn't, then I suspect things like virtual origins in Ada couldn't be
> >>>> made ABI compliant.
> >>> To be clear are u suggesting ADD sp, sp, 2047 is ABI compliant ?
> >>> I'd still like to avoid it as I'm sure someone will complain about it.
> >>>
> >>>>> With the patch, we get following correct code instead:
> >>>>>
> >>>>> | ..
> >>>>> | 59944:     add     s0,sp,2032
> >>>>> | ..
> >>>>> | 5995c:     add     s0,s0,16
> >>>> Alternately you could tighten the positive side of the range of the
> >>>> splitter from patch 1/3 so that you could always use 2032 rather than
> >>>> 2047 on the first addi.   ie instead of allowing 2048..4094, allow
> >>>> 2048..4064.
> >>> 2033..4064 vs. 2048..4094
> >>>
> >>> Yeah I was a bit split about this as well. Since you are OK with either,
> >>> I'll keep them as-is and perhaps add this observation to commitlog.
> >> There's a subset of embedded use cases where an interrupt service
> >> routine continues on the same stack as the interrupted thread,
> >> requiring sp to always have an ABI-compliant value (i.e. 16B aligned,
> >> and with no important data on the stack at an address below sp).
> >>
> >> Although not all use cases care about this property, it seems more
> >> straightforward to maintain the invariant everywhere, rather than
> >> selectively enforce it.
> > Just to be clear, the changes don't misalign the stack pointer at all.
> > They merely have the potential to create *another* pointer into the
> > stack which may or may not be aligned.  Which is totally normal, it's no
> > different than taking the address of a char on the stack.
>
> Right I never saw any sp,sp,2047 getting generated - not even in the
> first version of patch which lacked any filtering of stack regs via
> riscv_reg_frame_related () and obviously didn't have the stack variant
> of splitter. I don't know if that is just being lucky and not enough
> testing exposure (I only spot checked buildroot libc, vmlinux) or
> something somewhere enforces that.
>
> However given that misaligned pointer off of stack is a non-issue, I
> think we can do the following:
>
> 1. keep just one splitter with 2047 based predicates and constraint (and
> not 2032) for both stack-related and general regs.
> 2. gate the splitter on only operands[0] being not stack related
> (currently it checks for either [0] or [1]) - this allows the prominent
> case where SP is simply a src, and avoids when any potential shenanigans
> to SP itself.

Agreed.  I misread the original code (add s0,sp,2047 looks a lot like
add sp,sp,2047 from a quick glance on a cell phone).

>
> -Vineet