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

[Violeta Vukobrat via llvm-commits]

violetav added a comment.

The problem with TailDuplication is the main reason why a separate pass is needed. In shouldTailDuplicate() method, while going through instructions in the BB, the pass finds CFI instructions (that are marked as NotDuplicable in Target.td) and decides not to duplicate the given BB.

Using save/restore CFI instructions could probably simplify solving the problem of epilogue in the middle, if CFI instructions were inserted in epilogue in frame lowering.

Another problem that complicates the case when CFI instructions are inserted in epilogue during frame lowering is shrink wrapping. It can cause epilogue to split (e.g. to a BB containing 'pop' instructions and a BB consisting of 'ret' instruction). Those BBs can be reordered later, and there could exist a situation where 'ret' instruction comes before 'pop' instructions. That ret instruction should have def_cfa_offset set by the epilogue, not prologue. This problem could be solved in some way, but the solution would probably be complicated and scattered among different passes.
Also, I believe that problems with other passes could come up, but that we just haven't come across them yet. Those, and mainly the TailDuplication issue, are the reasons why I decided to implement a separate pass to solve this issue.

As for deciding what is an epilogue, I will check if just checking the FrameDestroy flag is sufficient.

Concerning the problem with Darwin, I tried setting FrameSetup tag to CFI instructions added in prologue and using that when generating compact unwind, and it works, but not for the case when it is generated from the .s file (because the information about the tag is lost). I haven't checked what GCC does yet.


================
Comment at: lib/Target/X86/X86CFIInstrInserter.cpp:112
@@ +111,3 @@
+  TRI = ST->getRegisterInfo();
+  StackPtr = RI->getStackRegister();
+  FramePtr = RI->getPtrSizedFrameRegister(MF);
----------------
loladiro wrote:
> Have you checked that this works with x32, I saw problems there in my implementation.
I didn't notice any problems with x32 so far. What problems have you come across?

================
Comment at: lib/Target/X86/X86CFIInstrInserter.cpp:377
@@ +376,3 @@
+    if (!TFL->hasFP(MF) && (MBBI.getOpcode() == X86::ADD32ri8 ||
+                            MBBI.getOpcode() == X86::ADD64ri8 ||
+                            MBBI.getOpcode() == X86::ADD32ri)) {
----------------
loladiro wrote:
> Missing `X86::ADD64ri32`. A sufficiently large alloca would probably work for a test.
Thanks for pointing it out!


Repository:
  rL LLVM

http://reviews.llvm.org/D18046