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

[Palmer Dabbelt <palmer@rivosinc.com>]

On Tue, 19 Mar 2024 13:05:54 PDT (-0700), 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

It's 16-byte aligned in the default ABI, for Q, but that doesn't really 
matter.

>>>>>    - 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.

IIRC the "always"-ness of the stack pointer alignment has come up 
before, and we decided to keep it aligned for these embedded 
interrupt-related reasons that Andrew points out.  That's a bit 
different than most other ABI requirements, where we're just enforcing 
them on function boundaries.

That said, this one sounds like just a terminology issue: it's not SP 
that's misaligned, but intemediate SP-based addressing calculations.  
I'm not sure if there's a word for these SP-based intermediate values, 
during last week's team meeting we came up with "stack anchors".

> 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.

I guess we could run the tests with something like

    diff --git a/target/riscv/translate.c b/target/riscv/translate.c
    index ab18899122..e87cc83067 100644
    --- a/target/riscv/translate.c
    +++ b/target/riscv/translate.c
    @@ -320,12 +320,18 @@ static void gen_goto_tb(DisasContext *ctx, int n, target_long diff)
      */
     static TCGv get_gpr(DisasContext *ctx, int reg_num, DisasExtend ext)
     {
    -    TCGv t;
    +    TCGv t, gpr;
    
         if (reg_num == 0) {
             return ctx->zero;
         }
    
    +    if (reg_num == 2) {
    +        gpr = tcg_gen_temp_new();
    +        tcg_gen_andi_tl(gpr, cpu_gpr[reg_num], 0xF);
    +    } else
    +        gpr = cpu_gpr[reg_num];
    +
         switch (get_ol(ctx)) {
         case MXL_RV32:
             switch (ext) {
    @@ -333,11 +339,11 @@ static TCGv get_gpr(DisasContext *ctx, int reg_num, DisasExtend ext)
                 break;
             case EXT_SIGN:
                 t = tcg_temp_new();
    -            tcg_gen_ext32s_tl(t, cpu_gpr[reg_num]);
    +            tcg_gen_ext32s_tl(t, gpr);
                 return t;
             case EXT_ZERO:
                 t = tcg_temp_new();
    -            tcg_gen_ext32u_tl(t, cpu_gpr[reg_num]);
    +            tcg_gen_ext32u_tl(t, gpr);
                 return t;
             default:
                 g_assert_not_reached();
    @@ -349,7 +355,7 @@ static TCGv get_gpr(DisasContext *ctx, int reg_num, DisasExtend ext)
         default:
             g_assert_not_reached();
         }
    -    return cpu_gpr[reg_num];
    +    return gpr;
     }
    
     static TCGv get_gprh(DisasContext *ctx, int reg_num)

and see if anything blows up?  I'm not sure it's worth opening that can 
of worms, though...

> 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.

That seems reasonable to me.

>
> -Vineet