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

[Yeting Kuo via Phabricator via llvm-commits]

fakepaper56 added inline comments.


================
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.
----------------
KYG wrote:
> VincentWu wrote:
> > KYG wrote:
> > > 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);
> > > ```
> > is it `RegState::Implicit`? did you mean `RegState::Kill` ?
> > 
> > or I have add `.setMIFlag(MachineInstr::FrameDestroy)`, does it useful for this point?
> It should be `RegState::ImplicitDefine` or `RegState::Define`, since CM_POP is defining new values to registers rather than using their values (RegState::Kill means the last use of a register).
> I don't think `.setMIFlag(MachineInstr::FrameDestroy)` is enough for this purpose, it doesn't contain information about which registers are being assigned.
Why not use `ImplicitDefine` here? I think using `ImplicitDefine` can avoid using variadic operand for `CM_PUSH` and `CM_POP`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134599