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

[Michael Kuperstein via llvm-commits]

mkuper added a comment.

Comments inline.

Regardless of those comments, I'm still not sure I'm comfortable with the approach.
Reid, I think you originally supported this on the mailing list. Can you please take a look and see whether this is what you had in mind?


================
Comment at: lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:577
@@ -573,1 +576,3 @@
       }
+      case MCCFIInstruction::OpDefCfa: {
+        break;
----------------
May also be worthwhile to add someone relevant to this review.

================
Comment at: lib/Target/X86/X86CFIInstrInserter.cpp:103
@@ +102,3 @@
+  ST = &MF.getSubtarget<X86Subtarget>();
+  RI = ST->getRegisterInfo();
+  TFL = ST->getFrameLowering();
----------------
I don't think there are any plans to do that right now, unfortunately.
Is there anything preventing you from generating this for .eh_frame?

================
Comment at: lib/Target/X86/X86CFIInstrInserter.cpp:124
@@ +123,3 @@
+    unsigned Number = I.getNumber();
+    MBBInfoList[Number].MBB = &I;
+    // Set offset and register to initial values for now
----------------
Or an std::vector - I don't think SmallVector is too appropriate here, there's no reason for MF.size() to be small.

================
Comment at: lib/Target/X86/X86CFIInstrInserter.cpp:202
@@ +201,3 @@
+      if (!ShouldUpdateCFI) {
+        if (TFL->hasFP(MF) && (MBBI->getOpcode() == X86::POP32r ||
+                               MBBI->getOpcode() == X86::POP64r)) {
----------------
Can we maybe rip this whole if out into a separate function that returns ShouldUpdateCFI?

================
Comment at: lib/Target/X86/X86CFIInstrInserter.cpp:220
@@ +219,3 @@
+        }
+        if (!TFL->hasFP(MF) && (MBBI->getOpcode() == X86::LEA32r ||
+                                MBBI->getOpcode() == X86::LEA64r)) {
----------------
Can you also update the comments/names, as well as the comment in the class definition that explains what InsertCFIInEpilogue() does?

================
Comment at: lib/Target/X86/X86CFIInstrInserter.cpp:238
@@ +237,3 @@
+      // Set offset that is set by prologue
+      InsertCFIInEpilogue(NewMBBInfo, NewMBBInfo->EndOffset);
+    }
----------------
This still seems somewhat odd.

The logic in InsertCFIInEpilogue made sense to me, when it was meant specifically for epilogues. But now, this will affect every block - including non-epilogue blocks that happen to pop / change esp.
 
If I understand what's going on correctly, this will add a .cfi_def_cfa_offset at the end of each block that modifies the cfa offset. But doesn't CorrectCFA already do this, regardless? Or am I getting something wrong here? In any case, perhaps it will be easier to understand once the names and comments are updated.


================
Comment at: lib/Target/X86/X86CFIInstrInserter.cpp:355
@@ +354,3 @@
+          ShouldInsertCFI = true;
+        }
+      }
----------------
It may be so, but this has nothing to do with the initial offset - this is explicitly the slot size.

================
Comment at: test/CodeGen/X86/epilogue-cfi-fp.ll:2
@@ +1,3 @@
+; RUN: llc < %s | FileCheck %s
+
+; ModuleID = 'epilogue-cfi-fp.c'
----------------
This is a good start, but the tests still look like they contain much more than needed to check the specific things they check. Both in terms of debug info and attributes, and, for the EH test, a lot of code that seems to me to be redundant.


Repository:
  rL LLVM

http://reviews.llvm.org/D18046