[PATCH] D74303: [CFI] cfi directive insertion for callee-save-registers in CFIInstrInserter pass

[Francis Visoiu Mistrih via Phabricator via llvm-commits]

thegameg added a comment.

Sorry for the delay.

That's right, llvm didn't insert any CFI in epilogues before this pass was added, so backends can start emitting them once this pass is ready to deal with them.

I added some more inline comments, this seems pretty good!



================
Comment at: llvm/lib/CodeGen/CFIInstrInserter.cpp:174
       MBBInfo.MBB->getParent()->getFrameInstructions();
+  SmallVector<unsigned, 8> CSRSaved, CSRRestored;
 
----------------
Can a BitVector be used here too?


================
Comment at: llvm/lib/CodeGen/CFIInstrInserter.cpp:239
+  MBBInfo.OutgoingCSRSaved = MBBInfo.IncomingCSRSaved;
+  llvm::for_each(CSRSaved,
+                 [&](auto Reg) { MBBInfo.OutgoingCSRSaved.set(Reg); });
----------------
If `CSRSaved` is a `BitVector`, this can be just `MBBInfo.OutgoingCSRSaved |= CSRSaved`.


================
Comment at: llvm/lib/CodeGen/CFIInstrInserter.cpp:318
+    SetDifference.reset(MBBInfo.IncomingCSRSaved);
+    for (int Reg = SetDifference.find_first(); Reg >= 0;
+         Reg = SetDifference.find_next(Reg)) {
----------------
`for (int Reg : SetDifference.set_bits())`


================
Comment at: llvm/lib/CodeGen/CFIInstrInserter.cpp:353
+         << " outgoing CSR Saved: ";
+  for (int Reg = Pred.OutgoingCSRSaved.find_first(); Reg >= 0;
+       Reg = Pred.OutgoingCSRSaved.find_next(Reg))
----------------
`set_bits()` here too.


================
Comment at: llvm/lib/CodeGen/CFIInstrInserter.cpp:358-359
+  errs() << "Succ: " << Succ.MBB->getName() << " #" << Succ.MBB->getNumber()
+         << " incoming CSR Saved: ";
+  for (int Reg = Succ.IncomingCSRSaved.find_first(); Reg >= 0;
+       Reg = Succ.IncomingCSRSaved.find_next(Reg))
----------------
`set_bits()` here too.


================
Comment at: llvm/lib/Target/X86/X86FrameLowering.cpp:482
+               MCCFIInstruction::createOffset(nullptr, DwarfReg, Offset));
+    } else if (!MBB.succ_empty() && !MBB.isReturnBlock()) {
+      BuildCFI(MBB, MBBI, DL,
----------------
Can we just not call this function at all on no-return blocks?


================
Comment at: llvm/test/CodeGen/X86/cfi-epilogue-with-return.mir:9
+  define dso_local i64 @_Z3foob(i1 zeroext %cond) local_unnamed_addr #0 {
+  entry:
+    br i1 %cond, label %if.then, label %return, !prof !2, !misexpect !3
----------------
I would also remove all the unnecessary IR here if possible.


================
Comment at: llvm/test/CodeGen/X86/cfi-epilogue-with-return.mir:55
+  bb.1.if.then:
+    renamable $rbx = MOV64rm $rip, 1, $noreg, @a, $noreg :: (dereferenceable load 8 from `i64* getelementptr inbounds ([10 x i64], [10 x i64]* @a, i64 0, i64 0)`, align 16)
+    renamable $r14 = MOV64rm $rip, 1, $noreg, @a + 8, $noreg :: (dereferenceable load 8 from `i64* getelementptr inbounds ([10 x i64], [10 x i64]* @a, i64 0, i64 1)`)
----------------
Maybe you could use `renamable $rbx = IMPLICIT_DEF` for all the movs, for a cleaner test here and in the rest of the tests?


================
Comment at: llvm/test/CodeGen/X86/cfi-inserter-callee-save-register.mir:6
+--- |
+  define void @_Z3foobPl(i64* nocapture readonly %a) {
+  entry:
----------------
wmi wrote:
> thegameg wrote:
> > Does this test actually need the IR?
> The function define is needed, but its body can be further simplified. 
IIRC, you can even remove the function define by removing the whole IR part. For example: `llvm/test/CodeGen/X86/tailcall-pseudo.mir`


Repository:
  rL LLVM

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

https://reviews.llvm.org/D74303