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

[Michael Kuperstein via llvm-commits]

mkuper added inline comments.

================
Comment at: lib/Target/X86/X86CFIInstrInserter.cpp:322
@@ +321,3 @@
+      // If frame pointer is used and the instruction pops the frame pointer,
+      // then
+      // add new def_cfa instruction with initial offset and register values
----------------
Reformat this to make the comment look better? (I guess there was an 80-char violation there, and clang-format broke the comment up?)

================
Comment at: lib/Target/X86/X86CFIInstrInserter.cpp:332
@@ +331,3 @@
+                            nullptr, MRI->getDwarfRegNum(
+                                         Is64Bit ? X86::RSP : X86::ESP, true),
+                            InitialOffset));
----------------
Don't you have this in StackPtr already?

================
Comment at: lib/Target/X86/X86CFIInstrInserter.cpp:347
@@ +346,3 @@
+      DebugLoc DL = EpilogueInfo->MBB->findDebugLoc(MBBI);
+      // If frame pointer is not used and the instruction pops the stack
+      // pointer,
----------------
You mean just "pops", right? It's not a "pop %esp".

================
Comment at: lib/Target/X86/X86CFIInstrInserter.cpp:348
@@ +347,3 @@
+      // If frame pointer is not used and the instruction pops the stack
+      // pointer,
+      // then add new def_cfa_offset instruction with appropriate offset
----------------
Same as above re comment.

================
Comment at: lib/Target/X86/X86CFIInstrInserter.cpp:354
@@ +353,3 @@
+        ++MBBI;
+        CurrentOffset -= InitialOffset;
+        TFL->BuildCFI(
----------------
Oh, so you did mean "pop %esp"? If not, then shouldn't this be SlotSize?
If you did, then is the check for the pop argument missing? And what happens for a pop to a different register?

================
Comment at: lib/Target/X86/X86CFIInstrInserter.cpp:365
@@ +364,3 @@
+      // appropriate offset
+      if (MBBI->getOpcode() == X86::ADD32ri8 ||
+          MBBI->getOpcode() == X86::ADD64ri8 ||
----------------
Can we maybe have different code only to get the offset based on the opcode, and then use the same code to actually construct the CFI instruction? It looks like most of the inside of these 3 ifs is duplicated.


Repository:
  rL LLVM

http://reviews.llvm.org/D18046