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

[Jeff Law <jeffreyalaw@gmail.com>]

On 3/19/24 2:05 PM, Vineet Gupta 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.
Sounds reasonable to me.

Jeff