[WebAssembly] Use the frame pointer instead of the stack pointer

[jgravelle-google]

Summary

When we have dynamic allcoas we need a frame pointer, and when we're lowering frame indexes we should make sure we use it.

[dschuff]

lib/Target/WebAssembly/WebAssemblyRegisterInfo.cpp

69 ↗

(On Diff #72380)

how about "and don't use FrameIndex operands"

test/CodeGen/WebAssembly/userstack.ll

185 ↗

(On Diff #72380)

This comment is a little confusing to me. Is it just referring to the copy_local/tee_local hop?

211 ↗

(On Diff #72380)

Maybe add another comment line something like "Ensure that the addressing is still correct after the second SP adjustment"

[jgravelle-google]

lib/Target/WebAssembly/WebAssemblyRegisterInfo.cpp

69 ↗

(On Diff #72380)

Sounds good.

test/CodeGen/WebAssembly/userstack.ll

185 ↗

(On Diff #72380)

I think so? It's confusing to me as well, and seems wholly redundant. Will remove.

[jgravelle-google]

As a note, this fixes https://llvm.org/bugs/show_bug.cgi?id=30215

[jgravelle-google]

Modified comments, made unit test slightly stricter

[jgravelle-google]

Hopefully include both commits in this patch

llvm/trunk/lib/Target/WebAssembly/WebAssemblyRegisterInfo.cpp

void WebAssemblyRegisterInfo::eliminateFrameIndex(
 
   MachineBasicBlock &MBB = *MI.getParent();
   MachineFunction &MF = *MBB.getParent();
   MachineRegisterInfo &MRI = MF.getRegInfo();
   int FrameIndex = MI.getOperand(FIOperandNum).getIndex();
   const MachineFrameInfo &MFI = MF.getFrameInfo();
   int64_t FrameOffset = MFI.getStackSize() + MFI.getObjectOffset(FrameIndex);
 
+  assert(MFI.getObjectSize(FrameIndex) != 0 &&
+         "We assume that variable-sized objects have already been lowered, "
+         "and don't use FrameIndex operands.");
+  unsigned FrameRegister = getFrameRegister(MF);
+
   // If this is the address operand of a load or store, make it relative to SP
   // and fold the frame offset directly in.
   if (MI.mayLoadOrStore() && FIOperandNum == WebAssembly::MemOpAddressOperandNo) {
     assert(FrameOffset >= 0 && MI.getOperand(1).getImm() >= 0);
     int64_t Offset = MI.getOperand(1).getImm() + FrameOffset;
 
     if (static_cast<uint64_t>(Offset) <= std::numeric_limits<uint32_t>::max()) {
       MI.getOperand(FIOperandNum - 1).setImm(Offset);
       MI.getOperand(FIOperandNum)
-          .ChangeToRegister(WebAssembly::SP32, /*IsDef=*/false);
+          .ChangeToRegister(FrameRegister, /*IsDef=*/false);
       return;
     }
   }
 
   // If this is an address being added to a constant, fold the frame offset
   // into the constant.
   if (MI.getOpcode() == WebAssembly::ADD_I32) {
     MachineOperand &OtherMO = MI.getOperand(3 - FIOperandNum);
     if (OtherMO.isReg()) {
       unsigned OtherMOReg = OtherMO.getReg();
       if (TargetRegisterInfo::isVirtualRegister(OtherMOReg)) {
         MachineInstr *Def = MF.getRegInfo().getUniqueVRegDef(OtherMOReg);
         // TODO: For now we just opportunistically do this in the case where
         // the CONST_I32 happens to have exactly one def and one use. We
         // should generalize this to optimize in more cases.
         if (Def && Def->getOpcode() == WebAssembly::CONST_I32 &&
             MRI.hasOneNonDBGUse(Def->getOperand(0).getReg())) {
           MachineOperand &ImmMO = Def->getOperand(1);
           ImmMO.setImm(ImmMO.getImm() + uint32_t(FrameOffset));
           MI.getOperand(FIOperandNum)
-              .ChangeToRegister(WebAssembly::SP32, /*IsDef=*/false);
+              .ChangeToRegister(FrameRegister, /*IsDef=*/false);
           return;
         }
       }
     }
   }
 
   // Otherwise create an i32.add SP, offset and make it the operand.
   const auto *TII = MF.getSubtarget<WebAssemblySubtarget>().getInstrInfo();
 
-  unsigned FIRegOperand = WebAssembly::SP32;
+  unsigned FIRegOperand = FrameRegister;
   if (FrameOffset) {
     // Create i32.add SP, offset and make it the operand.
     const TargetRegisterClass *PtrRC =
         MRI.getTargetRegisterInfo()->getPointerRegClass(MF);
     unsigned OffsetOp = MRI.createVirtualRegister(PtrRC);
     BuildMI(MBB, *II, II->getDebugLoc(), TII->get(WebAssembly::CONST_I32),
             OffsetOp)
         .addImm(FrameOffset);
     FIRegOperand = MRI.createVirtualRegister(PtrRC);
     BuildMI(MBB, *II, II->getDebugLoc(), TII->get(WebAssembly::ADD_I32),
             FIRegOperand)
-        .addReg(WebAssembly::SP32)
+        .addReg(FrameRegister)
         .addReg(OffsetOp);
   }
   MI.getOperand(FIOperandNum).ChangeToRegister(FIRegOperand, /*IsDef=*/false);
 }
 
 unsigned
 WebAssemblyRegisterInfo::getFrameRegister(const MachineFunction &MF) const {
   static const unsigned Regs[2][2] = {

llvm/trunk/test/CodeGen/WebAssembly/userstack.ll

  store i32 0, i32* %r
  ; CHECK-NEXT: return
  ret void
 }
 
 ; CHECK-LABEL: dynamic_static_alloca:
 define void @dynamic_static_alloca(i32 %alloc) noredzone {
  ; Decrement SP in the prolog by the static amount and writeback to memory.
- ; CHECK: i32.const $push[[L11:.+]]=, 0{{$}}
- ; CHECK: i32.const $push[[L8:.+]]=, 0{{$}}
- ; CHECK-NEXT: i32.load $push[[L9:.+]]=, __stack_pointer($pop[[L8]])
- ; CHECK-NEXT: i32.const $push[[L10:.+]]=, 16
- ; CHECK-NEXT: i32.sub $push[[L20:.+]]=, $pop[[L9]], $pop[[L10]]
- ; CHECK-NEXT: tee_local $push[[L19:.+]]=, $[[FP:.+]]=, $pop[[L20]]
- ; CHECK:      i32.store $drop=, __stack_pointer($pop{{.+}}), $pop{{.+}}
+ ; CHECK: i32.const $push[[L13:.+]]=, 0{{$}}
+ ; CHECK: i32.const $push[[L10:.+]]=, 0{{$}}
+ ; CHECK-NEXT: i32.load $push[[L11:.+]]=, __stack_pointer($pop[[L10]])
+ ; CHECK-NEXT: i32.const $push[[L12:.+]]=, 16
+ ; CHECK-NEXT: i32.sub $push[[L23:.+]]=, $pop[[L11]], $pop[[L12]]
+ ; CHECK-NEXT: tee_local $push[[L22:.+]]=, $[[SP:.+]]=, $pop[[L23]]
+ ; CHECK-NEXT: i32.store $drop=, __stack_pointer($pop{{.+}}), $pop[[L22]]
+
+ ; Alloc and write to a static alloca
+ ; CHECK: copy_local $push[[L21:.+]]=, $[[SP]]
+ ; CHECK-NEXT: tee_local $push[[pushedFP:.+]]=, $[[FP:.+]]=, $pop[[L21]]
+ ; CHECK-NEXT: i32.const $push[[L0:.+]]=, 101
+ ; CHECK-NEXT: i32.store $drop=, [[static_offset:.+]]($pop[[pushedFP]]), $pop[[L0]]
+ %static = alloca i32
+ store volatile i32 101, i32* %static
+
+ ; Decrement SP in the body by the dynamic amount.
+ ; CHECK: i32.sub
+ ; CHECK: tee_local $push{{.+}}=, $[[dynamic_local:.+]]=, $pop{{.+}}
+ ; CHECK: i32.store {{.*}} __stack_pointer
+ %dynamic = alloca i32, i32 %alloc
+
+ ; Ensure we don't modify the frame pointer after assigning it.
+ ; CHECK-NOT: $[[FP]]=
+
+ ; Ensure the static address doesn't change after modifying the stack pointer.
+ ; CHECK: i32.const $push[[L7:.+]]=, 102
+ ; CHECK-NEXT: i32.store $drop=, [[static_offset]]($[[FP]]), $pop[[L7]]
+ ; CHECK-NEXT: i32.const $push[[L8:.+]]=, 103
+ ; CHECK-NEXT: i32.store $drop=, 0($[[dynamic_local]]), $pop[[L8]]
+ store volatile i32 102, i32* %static
+ store volatile i32 103, i32* %dynamic
+
  ; Decrement SP in the body by the dynamic amount.
  ; CHECK: i32.sub
+ ; CHECK: tee_local $push{{.+}}=, $[[dynamic2_local:.+]]=, $pop{{.+}}
+ %dynamic.2 = alloca i32, i32 %alloc
+
+ ; CHECK-NOT: $[[FP]]=
+
+ ; Ensure neither the static nor dynamic address changes after the second
+ ; modification of the stack pointer.
+ ; CHECK: i32.const $push[[L9:.+]]=, 104
+ ; CHECK-NEXT: i32.store $drop=, [[static_offset]]($[[FP]]), $pop[[L9]]
+ ; CHECK-NEXT: i32.const $push[[L10:.+]]=, 105
+ ; CHECK-NEXT: i32.store $drop=, 0($[[dynamic_local]]), $pop[[L10]]
+ ; CHECK-NEXT: i32.const $push[[L11:.+]]=, 106
+ ; CHECK-NEXT: i32.store $drop=, 0($[[dynamic2_local]]), $pop[[L11]]
+ store volatile i32 104, i32* %static
+ store volatile i32 105, i32* %dynamic
+ store volatile i32 106, i32* %dynamic.2
+
  ; Writeback to memory.
- ; CHECK: i32.store $drop=, __stack_pointer($pop{{.+}}), $pop{{.+}}
- %r1 = alloca i32
- %r = alloca i32, i32 %alloc
- store i32 0, i32* %r
- ; CHEC: i32.store $drop=, 0($pop{{.+}}), $pop{{.+}}
+ ; CHECK: i32.const $push[[L17:.+]]=, 16
+ ; CHECK-NEXT: i32.add $push[[L18:.+]]=, $[[FP]], $pop[[L17]]
+ ; CHECK-NEXT: i32.store $drop=, __stack_pointer($pop{{.+}}), $pop[[L18]]
  ret void
 }
 
 ; The use of the alloca in a phi causes a CopyToReg DAG node to be generated,
 ; which has to have special handling because CopyToReg can't have a FI operand
 ; CHECK-LABEL: copytoreg_fi:
 define void @copytoreg_fi(i1 %cond, i32* %b) {
 entry: