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

[Kyle Butt via Phabricator via llvm-commits]

iteratee added a comment.

In https://reviews.llvm.org/D18046#715352, @violetav wrote:

> > They currently affect code generation. This is my biggest complaint. I don't see why they should affect these passes.
>
> This patch modifies those passes with changes that prevent CFI instructions from affecting code generation, so, with these changes we will avoid affecting code generation.


Those kinds of modifications are exactly what I don't want to see.

Here's what I'd like to see:
Change CFI Instructions so that the following 3 things are no longer true:

1. CFI Instructions are marked as not duplicable.
2. CFI Instructions don't compare as equal (and so can't be merged)
3. CFI Instructions count as instructions when counting for tail-duplicating or tail-merging.

The first 2 shouldn't require any changes to existing passes, and the third should be minor, and definitely not platform specific.
These will require other work as well, but basically what I described above.

As a second best, special casing these instructions in a platform-independent way for platforms that can handle them later would work.
That means adding something to Target Info to indicate if a platform can handle these instructions, defaulting it to false, and then setting it to true for X86.
That would be sufficient for tail-duplication and tail merging.



================
Comment at: lib/CodeGen/BranchFolding.cpp:289
+  MachineFunction &MF = *MBB1->getParent();
+  bool IsX86 = MF.getTarget().getTargetTriple().getArch() == Triple::x86_64 ||
+               MF.getTarget().getTargetTriple().getArch() == Triple::x86;
----------------
violetav wrote:
> rnk wrote:
> > Please don't do these types of triple checks in target independent CodeGen code. Use TII.
> > 
> > What are you trying to do here? Avoid tail merging epilogues, or do more tail merging, or...?
> How can I use TII to get this information?
> 
> To do more tail merging (CFI instructions have different CFI indices, so isIdenticalTo() returns false, and prevents tail merging that would have happened otherwise).
CFI Instructions should be more like the debug instructions that are skipped below.
It shouldn't be platform specific. CFI isn't platform specific.


================
Comment at: lib/CodeGen/TailDuplicator.cpp:583
     if (MI.isNotDuplicable())
-      return false;
+      if (!IsX86 || (IsX86 && !MI.isCFIInstruction()))
+        return false;
----------------
This tells me that marking CFI Instructions as not duplicable is wrong for all platforms.


================
Comment at: lib/CodeGen/TailDuplicator.cpp:604
     if (!MI.isPHI() && !MI.isDebugValue())
-      InstrCount += 1;
+       if (!IsX86 || (IsX86 && !MI.isCFIInstruction()))
+         InstrCount += 1;
----------------
And this tells me that either it needs to be marked as a debug instruction, or we need a broader category for assembler directives that are not instructions.


================
Comment at: lib/CodeGen/TargetInstrInfo.cpp:396
+
+  if (!(IsX86 && Orig.isCFIInstruction()))
+    assert(!Orig.isNotDuplicable() && "Instruction cannot be duplicated");
----------------
I read the DWARF spec on CFI. I didn't see where it said this only worked on X86.


Repository:
  rL LLVM

https://reviews.llvm.org/D18046