[PATCH] D18046: [X86] Providing correct unwind info in function epilogue

[Kyle Butt via Phabricator via llvm-commits]

iteratee added a comment.

OK. It's looking pretty good overall. I looked a lot closer at the actual CFI Code. Mostly it looks great. I don't think you need AdjustCFAOffset at all. You spend a lot of work maintaining it, but in the one case that it's used, it's just (OutgoingOffset - IncomingOffset). It's only used with blocks that don't contain an offset def.

That was the hardest part for me to understand, and I don't think you need it. If you can remove it, feel free to mark the comments as done by way of code removal.



================
Comment at: include/llvm/CodeGen/MachineBasicBlock.h:764
+  unsigned OutgoingCFARegister = 0;
+  // Amount of adjust cfa offset in effect for this basic block.
+  int AdjustCFAOffset = 0;
----------------
Should this always be equal to OutgoingCFAOffset - IncomingCFAOffset?

I think I get it. It's the total amount of adjustments that occur in this block, either since the beginning or since the last def_cfa_offset directive in the function. Can you be more explicit in the comments?


================
Comment at: lib/CodeGen/CFIInfoVerifier.cpp:64
+  for (auto &CurrMBB : MF) {
+    for (auto I : CurrMBB.predecessors()) {
+      // Check that outgoing offset values of predecessors match the incoming
----------------
Can you name this something like Pred?


================
Comment at: lib/CodeGen/CFIInfoVerifier.cpp:89
+
+    for (auto I : CurrMBB.successors()) {
+      // Check that incoming offset values of successors match the outgoing
----------------
Same here.


================
Comment at: lib/CodeGen/CFIInstrInserter.cpp:65
+
+  return InsertedCFIInstr;
+}
----------------
you should clear this value in CorrectCFA.


================
Comment at: lib/CodeGen/CFIInstrInserter.cpp:88
+            nullptr, MBB.getIncomingCFARegister(),
+            -MBB.getIncomingCFAOffset()));
+        BuildMI(MBB, MBBI, DL, TII->get(TargetOpcode::CFI_INSTRUCTION))
----------------
The negative here is confusing. Can you make it so you don't have to remember to flip the sign here?


================
Comment at: lib/CodeGen/CFIInstrInserter.cpp:97
+            MF.addFrameInst(MCCFIInstruction::createDefCfaOffset(
+                nullptr, -MBB.getIncomingCFAOffset()));
+        BuildMI(MBB, MBBI, DL, TII->get(TargetOpcode::CFI_INSTRUCTION))
----------------
Same here.


================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:1349
+        case MCCFIInstruction::OpAdjustCfaOffset:
+          setAdjustCFAOffset(
+              getAdjustCFAOffset() +
----------------
I think this would be more clear if you used a local variable.


================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:1420
+          if (CFIType == MCCFIInstruction::OpDefCfa) ShouldSetRegister = false;
+          break;
+        case MCCFIInstruction::OpDefCfa:
----------------
Isn't there a missing case here, where if you encounter both a def_cfa_offset and a def_cfa_register, you can early return, like below?


================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:1473
+    if (ProcessedMBBs.find(Succ) != ProcessedMBBs.end()) continue;
+    Succ->setIncomingCFAOffset(getOutgoingCFAOffset());
+    Succ->setIncomingCFARegister(getOutgoingCFARegister());
----------------
Should this short-circuit if they already match?


================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:1501
+      if (!CurrSucc->hasDefOffset())
+        Succ->setIncomingCFAOffset(CurrSucc->getOutgoingCFAOffset());
+      // If CurrSucc doesn't have a def_cfa_register or def_cfa directive,
----------------
Same here. It looks like you could short-circuit if they're equal.


================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:1564
+  // Recalculate AdjustCFAOffset.
+  updateAdjustCFAOffset();
+}
----------------
It feels weird to me that you would scan the block twice. This could be done in the loop above, couldn't it?


Repository:
  rL LLVM

https://reviews.llvm.org/D18046