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 <vine...@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.
-Vineet
