[PATCH] D134599: [RISCV] Readjusting the framestack for Zcmp

[KaiYi via Phabricator via llvm-commits]

KYG added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:260
+
+void reallocPushStackFream(MachineFunction &MF) {
+  auto *RVFI = MF.getInfo<RISCVMachineFunctionInfo>();
----------------
VincentWu wrote:
> KYG wrote:
> > reallocPushStackFream -> reallocPushStackFrame ?
> > Is it necessary to setObjectOffset for non-pushed registers? I though their offset has already been set in 
> > PEI::calculateFrameObjectOffsets (correct me if I'm wrong).
> IMHO, I think it might be necessary in some cases.
> Please refer to the `callee_with_irq` in `llvm/test/CodeGen/RISCV/push-pop-popret.ll`.
> It will generate following code in `RV32I`
> ```
> addi    sp, sp, -144
> sw      ra, 140(sp)                     # 4-byte Folded Spill
> sw      t0, 136(sp)                     # 4-byte Folded Spill
> sw      t1, 132(sp)                     # 4-byte Folded Spill
> sw      t2, 128(sp)                     # 4-byte Folded Spill
> sw      s0, 124(sp)                     # 4-byte Folded Spill
> sw      s1, 120(sp)                     # 4-byte Folded Spill
> sw      a0, 116(sp)                     # 4-byte Folded Spill
> sw      a1, 112(sp)                     # 4-byte Folded Spill
> sw      a2, 108(sp)                     # 4-byte Folded Spill
> sw      a3, 104(sp)                     # 4-byte Folded Spill
> sw      a4, 100(sp)                     # 4-byte Folded Spill
> sw      a5, 96(sp)                      # 4-byte Folded Spill
> sw      a6, 92(sp)                      # 4-byte Folded Spill
> sw      a7, 88(sp)                      # 4-byte Folded Spill
> sw      s2, 84(sp)                      # 4-byte Folded Spill
> sw      s3, 80(sp)                      # 4-byte Folded Spill
> sw      s4, 76(sp)                      # 4-byte Folded Spill
> sw      s5, 72(sp)                      # 4-byte Folded Spill
> sw      s6, 68(sp)                      # 4-byte Folded Spill
> sw      s7, 64(sp)                      # 4-byte Folded Spill
> sw      s8, 60(sp)                      # 4-byte Folded Spill
> sw      s9, 56(sp)                      # 4-byte Folded Spill
> sw      s10, 52(sp)                     # 4-byte Folded Spill
> sw      s11, 48(sp)                     # 4-byte Folded Spill
> sw      t3, 44(sp)                      # 4-byte Folded Spill
> sw      t4, 40(sp)                      # 4-byte Folded Spill
> sw      t5, 36(sp)                      # 4-byte Folded Spill
> sw      t6, 32(sp)
> ```
> 
> if we don't realloc non-pushed registers in `RV32IZcmp`, it will generate code like
> ```
> cm.push {ra, s0-s11}, -112
> addi    sp, sp, -32
> sw      t0, 136(sp)                     # 4-byte Folded Spill
> sw      t1, 132(sp)                     # 4-byte Folded Spill
> sw      t2, 128(sp)                     # 4-byte Folded Spill
> sw      a0, 116(sp)                     # 4-byte Folded Spill
> sw      a1, 112(sp)                     # 4-byte Folded Spill
> sw      a2, 108(sp)                     # 4-byte Folded Spill
> sw      a3, 104(sp)                     # 4-byte Folded Spill
> sw      a4, 100(sp)                     # 4-byte Folded Spill
> sw      a5, 96(sp)                      # 4-byte Folded Spill
> sw      a6, 92(sp)                      # 4-byte Folded Spill
> sw      a7, 88(sp)                      # 4-byte Folded Spill
> sw      t3, 44(sp)                      # 4-byte Folded Spill
> sw      t4, 40(sp)                      # 4-byte Folded Spill
> sw      t5, 36(sp)                      # 4-byte Folded Spill
> sw      t6, 32(sp)                      # 4-byte Folded Spill
> ```
> 
> meanwhile, accronding to Operation of `cm.push` defiend in spec
> {F27245522}
> 
> You may find both `s0` and `t0` will be saved to `sp+136`. It will cause problems.
> So I think I have to re-alloc non-pushed registers. please correct me if I'm wrong or there is a better solution )
Ah yes, you're right.
However, seems like it's only necessary when push/pop change any of the offset, which happens only in irq handler.
So maybe it's better to `reallocPushStackFrame` only for functions with interrupt attribute.


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:1474-1475
 
-  const char *RestoreLibCall = getRestoreLibCallName(*MF, CSI);
-  if (RestoreLibCall) {
-    // Add restore libcall via tail call.
-    MachineBasicBlock::iterator NewMI =
-        BuildMI(MBB, MI, DL, TII.get(RISCV::PseudoTAIL))
-            .addExternalSymbol(RestoreLibCall, RISCVII::MO_CALL)
-            .setMIFlag(MachineInstr::FrameDestroy);
-
-    // Remove trailing returns, since the terminator is now a tail call to the
-    // restore function.
-    if (MI != MBB.end() && MI->getOpcode() == RISCV::PseudoRET) {
-      NewMI->copyImplicitOps(*MF, *MI);
-      MI->eraseFromParent();
+    MachineInstrBuilder PopBuilder =
+        BuildMI(MBB, MI, DL, TII.get(RISCV::CM_POP));
+    // Use encoded number to represent registers to restore.
----------------
VincentWu wrote:
> KYG wrote:
> > Need to notify CM_POP kill (def) callee-saved registers, otherwise MIScheduler could not see the dependency, but I'm not sure how to do this (addDef to each poped register?).
> I think I have got what you mean, but can you tell me which function I should use to kill the callee-saved register? 
> Or where the LW/LD instruction does？
Normally we should add those registers to the "Defs" list in .td, but I don't know how to do this since the rlist is a compile time variable.
So I just add implicit def to the MI as a convenient workaround.
```
for (int x = 0; RegList[x] <= MaxReg && RegList[x] > 0; ++x)
  PopBuilder.addDef(RegList[x], RegState::Implicit);
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134599/new/

https://reviews.llvm.org/D134599