[PATCH] D134599: [RISCV] Add CodeGen support of RISCV zcmp Extension

[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>();
----------------
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).


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:287
+  auto &Operand = MBBI->getOperand(OpNum - 1);
+  int RegisterOffset = Operand.getImm();
+  RequiredAdj -= RegisterOffset;
----------------
total_sp_adj = base_sp_adj (rlist) + additional_sp_adj (Spimm)
I think it's better to get base_sp_adj through rlist, instead of assuming that the init Spimm value is actually base_sp_adj then correct it to additional_sp_adj here.


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:568
+  bool PushEnabled = isCSIpushable(MF, CSI);
+  if (PushEnabled && (CSI.size() != 0)) {
+    // Check at what offset spilling of registers starts and allocate space
----------------
Already checked in isCSIpushable().


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:1398
+      } else
+        NonePushCSI.push_back(CS);
+    }
----------------
I think we should have something like getNonLibcallCSI(), maybe getNonPushPopCSI()?


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:1407-1412
+    // Calculate SpImm Base adjustment, and SpImm field will be updated
+    // through adjSPInPushPop.
+    bool isRV64 = STI.getFeatureBits()[RISCV::Feature64Bit];
+    bool isEABI = false; // Reserved for future implementation
+    uint32_t SpImmBase = RISCVZC::getStackAdjBase(RegEnc, isRV64, isEABI);
+    PushBuilder.addImm(SpImmBase);
----------------
Better just addImm(0) then update it in adjSPInPushPop(), since the Spimm field never mean to store the base sp adjustment.


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:1459
 
-  // Manually restore values not restored by libcall.
-  // Keep the same order as in the prologue. There is no need to reverse the
-  // order in the epilogue. In addition, the return address will be restored
-  // first in the epilogue. It increases the opportunity to avoid the
-  // load-to-use data hazard between loading RA and return by RA.
-  // loadRegFromStackSlot can insert multiple instructions.
-  const auto &NonLibcallCSI = getNonLibcallCSI(*MF, CSI);
-  for (auto &CS : NonLibcallCSI) {
-    Register Reg = CS.getReg();
-    const TargetRegisterClass *RC = TRI->getMinimalPhysRegClass(Reg);
-    TII.loadRegFromStackSlot(MBB, MI, Reg, CS.getFrameIdx(), RC, TRI,
-                             Register());
-    assert(MI != MBB.begin() && "loadRegFromStackSlot didn't insert any code!");
-  }
+  if (isCSIpushable(*MF, CSI.vec())) {
+    Register MaxReg = RISCV::NoRegister;
----------------
Is it possible to cache the result (isPushalbe, and maybe rlist)?
Since push/pop uses a reversed order, using different rlist between prologue and epilogue will cause issue, so maybe just check isCSIpushable once then cache the rlist value.


================
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.
----------------
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?).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134599