[RISCV] Use two ADDIs to do some stack pointer adjustments.

[craig.topper]

Summary

If the adjustment doesn't fit in 12 bits, try to break it into
two 12 bit values before falling back to movImm+add/sub.

This is based on a similar idea from isel.

[jrtc27]

Doesn't this run the risk of temporarily misaligning the stack (e.g. for Val == 2064)? The psABI says using such a non-standard ABI within a function is fine and that kernels should realign stacks before calling signal handlers. Both FreeBSD and Linux correctly do this for signal handlers so as to not make assumptions about whether userspace is following the standard ABI, but both assume that the kernel itself is following the standard ABI, and so do not realign the stack in their exception entry points, causing the stack to be misaligned if an interrupt is taken in between. Whether this is a bug or not in the kernels is debatable, but with a more careful splitting of Val it should be possible to avoid anyway, with the exception of 4080?

[jrtc27]

For reference, this is the exact text in the psABI spec:

In the standard ABI, the stack pointer must remain aligned throughout procedure execution. Non-standard ABI code must realign the stack pointer prior to invoking standard ABI procedures. The operating system must realign the stack pointer prior to invoking a signal handler; hence, POSIX signal handlers need not realign the stack pointer. In systems that service interrupts using the interruptee’s stack, the interrupt service routine must realign the stack pointer if linked with any code that uses a non-standard stack-alignment discipline, but need not realign the stack pointer if all code adheres to the standard ABI.

[craig.topper]

In D126392#3537748, @jrtc27 wrote:

Doesn't this run the risk of temporarily misaligning the stack (e.g. for Val == 2064)? The psABI says using such a non-standard ABI within a function is fine and that kernels should realign stacks before calling signal handlers. Both FreeBSD and Linux correctly do this for signal handlers so as to not make assumptions about whether userspace is following the standard ABI, but both assume that the kernel itself is following the standard ABI, and so do not realign the stack in their exception entry points, causing the stack to be misaligned if an interrupt is taken in between. Whether this is a bug or not in the kernels is debatable, but with a more careful splitting of Val it should be possible to avoid anyway, with the exception of 4080?

Good point. I copied the split from what we do in isel without thinking about it. I'll try a better split.

[craig.topper]

Instead of dividing in half, use a large immediate, either -2048 or (2048-stackalign),
to do a first adjustment. Then a smaller adjustment for the remainder.

[reames]

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

309–320

Is there anything here that guarantees stack alignment must be smaller than 12 bits? If not, I don't think your statement about -2048 always being aligned holds.

On the positive vs negative direction, shouldn't we always be able to use the same constant by simply swapping the ADD for a SUB?

[craig.topper]

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

309–320

As far as I know the stack align is part of the RISCVFrameLowering constructor. It doesn’t change per function. Unless someone changes it from 16 to a huge value it should be true.

There is no SUBI instruction.

[jrtc27]

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

321

llvm/test/CodeGen/RISCV/rvv/emergency-slot.mir

157

... which shows up here

llvm/test/CodeGen/RISCV/rvv/large-rvv-stack-size.mir

36

... and here

[craig.topper]

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

321

Thanks I started over forgot to change it after I copy/pasted. I had it right in the first patch.

[craig.topper]

Use DestReg as the source for the second operation.

[arichardson]

This may break assumptions that some unwinding code uses (e.g. an kernel backtrace without debug info). But then again I don't know if any of them handle non-immediate offsets anyway.

[luismarques]

In D126392#3539458, @arichardson wrote:

This may break assumptions that some unwinding code uses (e.g. an kernel backtrace without debug info). But then again I don't know if any of them handle non-immediate offsets anyway.

What do you mean? Unwinding with neither the frame pointer nor CFI?

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

315

Nit: MaxAdj -> MaxAdjStep

[arichardson]

In D126392#3540178, @luismarques wrote:

In D126392#3539458, @arichardson wrote:

This may break assumptions that some unwinding code uses (e.g. an kernel backtrace without debug info). But then again I don't know if any of them handle non-immediate offsets anyway.

What do you mean? Unwinding with neither the frame pointer nor CFI?

Yeah I've seen such implementations for MIPS, maybe there are some for RISC-V too. This was not meant as a request to change anything, no one should rely on specific code generation patterns unless it's mandated by a specification.

[reames]

LGTM.

Minor optional comment from me, and remember to address nit from previous reviewer before landing.

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

309–320

I'd vague remembered there being more to the stack alignment on x86, and went and glanced. That's specific to the "stackrealign" attribute which we don't appear to support on RISC-V. I'm fine leaving that is for now.

Do me a favor and add the assert that stack align is a reasonable value w.r.t. your chosen constant for future proofing this?

[reames]

Minor improvement noted after LGTM. Feel free to roll into approved patch if you agree with my comment.

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

315

Hm, I think you can do slightly better here.

2048 is an 11 bit value. This makes sense as you need the high bit to be zero (since immediates are sign extended).

However, since you're subtracting off a non-zero value, I think you can actually use 4096 as your starting value. That *isn't* representable as a positive 12 bit value, but every value *less than* that is.

[reames]

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

315

Ignore this. I managed to misread my hex values. 2048 is correct here in exactly the way I was suggesting for 4096.

[craig.topper]

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

315

Maybe I should mention that this line was taken directly from RISCVFrameLowering::getFirstSPAdjustAmount.

[craig.topper]

Rename variable. Cleanup comment slightly.

[luismarques]

LGTM.

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

void RISCVFrameLowering::adjustReg(MachineBasicBlock &MBB,
   if (DestReg == SrcReg && Val == 0)
     return;
 
   if (isInt<12>(Val)) {
     BuildMI(MBB, MBBI, DL, TII->get(RISCV::ADDI), DestReg)
         .addReg(SrcReg)
         .addImm(Val)
         .setMIFlag(Flag);
-  } else {
-    unsigned Opc = RISCV::ADD;
-    bool IsSub = Val < 0;
-    if (IsSub) {
-      Val = -Val;
-      Opc = RISCV::SUB;
-    }
+    return;
+  }
+
+  // Try to split the offset across two ADDIs. We need to keep the stack pointer
+  // aligned after each ADDI. We need to determine the maximum value we can put
+  // in each ADDI. In the negative direction, we can use -2048 which is always
+  // sufficiently aligned. In the positive direction, we need to find the
+  // largest 12-bit immediate that is aligned. Exclude -4096 since it can be
+  // created with LUI.
+  assert(getStackAlign().value() < 2048 && "Stack alignment too large");
+  int64_t MaxPosAdjStep = 2048 - getStackAlign().value();
+  if (Val > -4096 && Val <= (2 * MaxPosAdjStep)) {
+    int64_t FirstAdj = Val < 0 ? -2048 : MaxPosAdjStep;
+    Val -= FirstAdj;
+    BuildMI(MBB, MBBI, DL, TII->get(RISCV::ADDI), DestReg)
+        .addReg(SrcReg)
+        .addImm(FirstAdj)
+        .setMIFlag(Flag);
+    BuildMI(MBB, MBBI, DL, TII->get(RISCV::ADDI), DestReg)
+        .addReg(DestReg, RegState::Kill)
+        .addImm(Val)
+        .setMIFlag(Flag);
+    return;
+  }
+
+  if (Val < 0) {
 
-    Register ScratchReg = MRI.createVirtualRegister(&RISCV::GPRRegClass);
-    TII->movImm(MBB, MBBI, DL, ScratchReg, Val, Flag);
-    BuildMI(MBB, MBBI, DL, TII->get(Opc), DestReg)
-        .addReg(SrcReg)
-        .addReg(ScratchReg, RegState::Kill)
-        .setMIFlag(Flag);
-  }
-}
 
 // Returns the register used to hold the frame pointer.
 static Register getFPReg(const RISCVSubtarget &STI) { return RISCV::X8; }
 
 // Returns the register used to hold the stack pointer.
 static Register getSPReg(const RISCVSubtarget &STI) { return RISCV::X2; }
 
 static SmallVector<CalleeSavedInfo, 8>

llvm/test/CodeGen/RISCV/rvv/emergency-slot.mir

body:             |
   ; CHECK-NEXT: bb.1:
   ; CHECK-NEXT:   successors: %bb.2(0x80000000)
   ; CHECK-NEXT:   liveins: $x1, $x5, $x6, $x7, $x9, $x10, $x11, $x12, $x13, $x14, $x15, $x17, $x18, $x19, $x20, $x21, $x22, $x23, $x24, $x25, $x26, $x27, $x28, $x29, $x30, $x31
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT:   renamable $x10 = COPY killed renamable $x13
   ; CHECK-NEXT:   PseudoBR %bb.2
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT: bb.2:
-  ; CHECK-NEXT:   $x10 = frame-destroy LUI 1
-  ; CHECK-NEXT:   $x10 = frame-destroy ADDIW killed $x10, -1792
-  ; CHECK-NEXT:   $x2 = frame-destroy SUB $x8, killed $x10
+  ; CHECK-NEXT:   $x2 = frame-destroy ADDI $x8, -2048
+  ; CHECK-NEXT:   $x2 = frame-destroy ADDI killed $x2, -256
   ; CHECK-NEXT:   $x2 = frame-destroy ADDI $x2, 272
   ; CHECK-NEXT:   $x1 = LD $x2, 2024 :: (load (s64) from %stack.3)
   ; CHECK-NEXT:   $x8 = LD $x2, 2016 :: (load (s64) from %stack.4)
   ; CHECK-NEXT:   $x18 = LD $x2, 2008 :: (load (s64) from %stack.5)
   ; CHECK-NEXT:   $x19 = LD $x2, 2000 :: (load (s64) from %stack.6)
   ; CHECK-NEXT:   $x20 = LD $x2, 1992 :: (load (s64) from %stack.7)
   ; CHECK-NEXT:   $x21 = LD $x2, 1984 :: (load (s64) from %stack.8)
   ; CHECK-NEXT:   $x22 = LD $x2, 1976 :: (load (s64) from %stack.9)

llvm/test/CodeGen/RISCV/rvv/large-rvv-stack-size.mir

--- |
   ; CHECK-NEXT:    sub sp, sp, a0
   ; CHECK-NEXT:    andi sp, sp, -128
   ; CHECK-NEXT:    lui a0, 1
   ; CHECK-NEXT:    addiw a0, a0, -1808
   ; CHECK-NEXT:    add a0, sp, a0
   ; CHECK-NEXT:    vs1r.v v25, (a0) # Unknown-size Folded Spill
   ; CHECK-NEXT:    ld a0, 8(sp)
   ; CHECK-NEXT:    call spillslot@plt
-  ; CHECK-NEXT:    lui a0, 1
-  ; CHECK-NEXT:    addiw a0, a0, -1792
-  ; CHECK-NEXT:    sub sp, s0, a0
+  ; CHECK-NEXT:    addi sp, s0, -2048
+  ; CHECK-NEXT:    addi sp, sp, -256
   ; CHECK-NEXT:    addi sp, sp, 272
   ; CHECK-NEXT:    ld ra, 2024(sp) # 8-byte Folded Reload
   ; CHECK-NEXT:    ld s0, 2016(sp) # 8-byte Folded Reload
   ; CHECK-NEXT:    addi sp, sp, 2032
   ; CHECK-NEXT:    ret
     ret void
   }
 

llvm/test/CodeGen/RISCV/stack-realignment.ll

 ; RV32I-NEXT:    addi sp, sp, -2032
 ; RV32I-NEXT:    .cfi_def_cfa_offset 2032
 ; RV32I-NEXT:    sw ra, 2028(sp) # 4-byte Folded Spill
 ; RV32I-NEXT:    sw s0, 2024(sp) # 4-byte Folded Spill
 ; RV32I-NEXT:    .cfi_offset ra, -4
 ; RV32I-NEXT:    .cfi_offset s0, -8
 ; RV32I-NEXT:    addi s0, sp, 2032
 ; RV32I-NEXT:    .cfi_def_cfa s0, 0
-; RV32I-NEXT:    lui a0, 1
-; RV32I-NEXT:    addi a0, a0, -2032
-; RV32I-NEXT:    sub sp, sp, a0
+; RV32I-NEXT:    addi sp, sp, -2048
+; RV32I-NEXT:    addi sp, sp, -16
 ; RV32I-NEXT:    andi sp, sp, -2048
 ; RV32I-NEXT:    lui a0, 1
 ; RV32I-NEXT:    addi a0, a0, -2048
 ; RV32I-NEXT:    add a0, sp, a0
 ; RV32I-NEXT:    call callee@plt
 ; RV32I-NEXT:    lui a0, 1
 ; RV32I-NEXT:    sub sp, s0, a0
-; RV32I-NEXT:    lui a0, 1
-; RV32I-NEXT:    addi a0, a0, -2032
-; RV32I-NEXT:    add sp, sp, a0
+; RV32I-NEXT:    addi sp, sp, 2032
+; RV32I-NEXT:    addi sp, sp, 32
 ; RV32I-NEXT:    lw ra, 2028(sp) # 4-byte Folded Reload
 ; RV32I-NEXT:    lw s0, 2024(sp) # 4-byte Folded Reload
 ; RV32I-NEXT:    addi sp, sp, 2032
 ; RV32I-NEXT:    ret
 ;
 ; RV64I-LABEL: caller2048:
 ; RV64I:       # %bb.0:
 ; RV64I-NEXT:    addi sp, sp, -2032
 ; RV64I-NEXT:    .cfi_def_cfa_offset 2032
 ; RV64I-NEXT:    sd ra, 2024(sp) # 8-byte Folded Spill
 ; RV64I-NEXT:    sd s0, 2016(sp) # 8-byte Folded Spill
 ; RV64I-NEXT:    .cfi_offset ra, -8
 ; RV64I-NEXT:    .cfi_offset s0, -16
 ; RV64I-NEXT:    addi s0, sp, 2032
 ; RV64I-NEXT:    .cfi_def_cfa s0, 0
-; RV64I-NEXT:    lui a0, 1
-; RV64I-NEXT:    addiw a0, a0, -2032
-; RV64I-NEXT:    sub sp, sp, a0
+; RV64I-NEXT:    addi sp, sp, -2048
+; RV64I-NEXT:    addi sp, sp, -16
 ; RV64I-NEXT:    andi sp, sp, -2048
 ; RV64I-NEXT:    lui a0, 1
 ; RV64I-NEXT:    addiw a0, a0, -2048
 ; RV64I-NEXT:    add a0, sp, a0
 ; RV64I-NEXT:    call callee@plt
 ; RV64I-NEXT:    lui a0, 1
 ; RV64I-NEXT:    sub sp, s0, a0
-; RV64I-NEXT:    lui a0, 1
-; RV64I-NEXT:    addiw a0, a0, -2032
-; RV64I-NEXT:    add sp, sp, a0
+; RV64I-NEXT:    addi sp, sp, 2032
+; RV64I-NEXT:    addi sp, sp, 32
 ; RV64I-NEXT:    ld ra, 2024(sp) # 8-byte Folded Reload
 ; RV64I-NEXT:    ld s0, 2016(sp) # 8-byte Folded Reload
 ; RV64I-NEXT:    addi sp, sp, 2032
 ; RV64I-NEXT:    ret
   %1 = alloca i8, align 2048
   call void @callee(i8* %1)
   ret void
 }