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

[Michael Kuperstein via llvm-commits]

mkuper added a comment.

Hi Violeta,

I'm in the middle of the review, but there's one thing I want to say about the general approach. What bothers me a bit is that recognizing the instructions that modify the stack pointer is separated from where these are actually are emitted. So, if we start making sp changes in the epilogue in some new, unpredictable way, this will break. On the other hand, I understand the need for doing this post-block-layout. (Unfortunately, I missed the original email, only found it now.)

I've been thinking, though - if we're ok with assuming that the sp only changes using a specific set of instructions, why not emit all cfa adjustment post-layout, instead of during frame lowering? And if we're not ok with that, then perhaps we need to solve the epilogue problem in a different way as well?


================
Comment at: lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:577
@@ -573,1 +576,3 @@
       }
+      case MCCFIInstruction::OpDefCfa: {
+        break;
----------------
Have you checked this with the Darwin people?
I don't know enough (=anything) about compact encoding, so can't really say whether these changes make sense or not.

================
Comment at: lib/Target/X86/X86CFIInstrInserter.cpp:102
@@ +101,3 @@
+
+  if (!MF.getMMI().hasDebugInfo()) {
+    return false;
----------------
The LLVM convention is to generally frown upon braces for single-line blocks, except when needed for readability. This isn't a hard-and-fast rule (and clang-format does not enforce it), but to be consistent with the rest of the code base, I'd suggest removing them.

================
Comment at: lib/Target/X86/X86CFIInstrInserter.cpp:102
@@ +101,3 @@
+
+  if (!MF.getMMI().hasDebugInfo()) {
+    return false;
----------------
mkuper wrote:
> The LLVM convention is to generally frown upon braces for single-line blocks, except when needed for readability. This isn't a hard-and-fast rule (and clang-format does not enforce it), but to be consistent with the rest of the code base, I'd suggest removing them.
What if we don't have debug info, but have EH?
On the one hand, I'm not sure whether we care about CFA in the epilogue being correct for EH. On the other hand, I think we've already decided that we don't want -g being passed to cause LLVM to modify .eh_frame. And since we currently can't generate different .eh_frame and .debug_frame, this will happen.

================
Comment at: lib/Target/X86/X86CFIInstrInserter.cpp:108
@@ +107,3 @@
+  RI = ST->getRegisterInfo();
+  StackPtr = ST->is64Bit() ? X86::RSP : X86::ESP;
+  TFL = ST->getFrameLowering();
----------------
Why not TRI->getStackRegister(MF)?

================
Comment at: lib/Target/X86/X86CFIInstrInserter.cpp:113
@@ +112,3 @@
+  unsigned FrameReg = TRI->getFrameRegister(MF);
+  FramePtr = Is64BitILP32 ? getX86SubSuperRegister(FrameReg, 64) : FrameReg;
+  MRI = MF.getMMI().getContext().getRegisterInfo();
----------------
I think TRI->getPtrSizedFrameRegister(MF) already does what you want.

================
Comment at: lib/Target/X86/X86CFIInstrInserter.cpp:123
@@ +122,3 @@
+
+  MBBInfoList = new struct MBBInfo[MF.size()];
+  BlocksToAnalyze = new std::queue<int>();
----------------
Why are these two on the heap?

================
Comment at: lib/Target/X86/X86CFIInstrInserter.cpp:124
@@ +123,3 @@
+  MBBInfoList = new struct MBBInfo[MF.size()];
+  BlocksToAnalyze = new std::queue<int>();
+
----------------
Actually, why is this a member? Isn't it local to AnalyzeMBB()?

================
Comment at: lib/Target/X86/X86CFIInstrInserter.cpp:128
@@ +127,3 @@
+  // debug info insertion
+  for (auto I = MF.begin(); I != MF.end(); ++I) {
+    // Number of the MBB is the index in the array
----------------
Can you use a range for?

================
Comment at: lib/Target/X86/X86CFIInstrInserter.cpp:165
@@ +164,3 @@
+    int AdjustOffset = 0;
+    DebugLoc DL = NewMBBInfo->MBB->findDebugLoc(NewMBBInfo->MBB->begin());
+    auto CFIInstructions = MF.getMMI().getFrameInstructions();
----------------
This doesn't look it's used anywhere.

================
Comment at: lib/Target/X86/X86CFIInstrInserter.cpp:181
@@ +180,3 @@
+          CFIIndex = MBBI->getOperand(0).getCFIIndex();
+          if (CFIInstructions[CFIIndex].getOperation() ==
+              MCCFIInstruction::OpType::OpAdjustCfaOffset) {
----------------
Maybe have CFIInstructions[CFIIndex].getOperation() as a temp variable?

================
Comment at: lib/Target/X86/X86CFIInstrInserter.cpp:219
@@ +218,3 @@
+                                MBBI->getOpcode() == X86::POP64r)) {
+          FoundEpilogue = true;
+        }
----------------
This doesn't look right. Why would the first block that has pops be the epilogue?

More generally, will it make sense to mark the epilogue block while it's emitted? Sure, it can move it around, but do we expect it to be split? If we do, and can't mark it ahead of time, then I think we need a better way to recognize it.

================
Comment at: test/CodeGen/X86/epilogue-cfi-fp.ll:1
@@ +1,2 @@
+; RUN: llc < %s | FileCheck %s
+
----------------
Any chance the tests can be made smaller?
E.g. even if we need debug info to be available, we don't actually need the debug info, only the flag, right?


Repository:
  rL LLVM

http://reviews.llvm.org/D18046