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

[Violeta Vukobrat via Phabricator via llvm-commits]

violetav added inline comments.


================
Comment at: lib/CodeGen/BranchFolding.cpp:329
     --I2;
     while (I2->isDebugValue()) {
       if (I2 == MBB2->begin())
----------------
iteratee wrote:
> should this be isDirective?
I don't think that this logic can be applied to CFI instructions. It seems that this code aims to set the iterators to include consecutive DBG_VALUE instructions above last common instruction in one block (when those DBG instructions do not exist in the other block). I don't think that that's the desired behaviour for CFI instructions.


================
Comment at: lib/CodeGen/BranchFolding.cpp:346
+
+  // Cover the case of having I1 or I2 point to a CFI_INSTRUCTION. This can
+  // happen if I1 and I2 are non-identical when compared and then one or both of
----------------
iteratee wrote:
> Is this block still necessary if the tests above are changed to isDirective? I'm having trouble making sense of them
It is. The tests above make sure that CFI instructions are not compared with isIdenticalTo(), and are not included when calculating TailLen.
This code ensures that the iterators do not point to CFI instructions (so CFI instructions do not represent the starting point of the common tail in a BB, as they do not exactly 'count' as common instructions of two blocks).
For example:
BB1:
...
INSTRUCTION_A
ADD32ri8
CFI_INSTRUCTION
POP32r
CFI_INSTRUCTION
POP32r
CFI_INSTRUCTION
RET

BB2:
...
INSTRUCTION_B
CFI_INSTRUCTION
ADD32ri8
CFI_INSTRUCTION
POP32r
CFI_INSTRUCTION
POP32r
CFI_INSTRUCTION
RET

In this example, BB1 and BB2 will have 4 common instructions (RET, POP, POP and ADD). When INSTRUCTION_A and INSTRUCTION_B are compared as not equal, after incrementing the iterators (++I1; ++I2;), I1 will point to ADD (the last common instruction, as it should), however I2 will point to the CFI instruction. This will, later on, result in BB2 being 'hacked off' (in ReplaceTailWithBranchTo()) at the wrong place (starting from this CFI instruction, instead of ADD below it), so this CFI instruction will be lost.


================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:1443
+    case MCCFIInstruction::OpDefCfaOffset:
+      setOutgoingCFAOffset(-CFI.getOffset() + AdjustAmount);
+      setAdjustCFAOffset(AdjustAmount);
----------------
iteratee wrote:
> Why is this offset negative? Can you explain it to me?
A short answer would be: because the value of cfa offset that is passed when creating a CFI instruction with createDefCfa() and createDefCfaOffset() is negated. That is the value that will end up as the actual offset in cfi directives. I was calculating in/out cfa offset with this non-negated value (that is passed when creating these instructions) so I had to negate the offset when reading the value with getOffset().

I am not sure why the correct value is not passed in the first place (e.g. in createAdjustCfaOffset(), the passed value is not negated).
I realised there was a bug with calculating in/out cfa offset - for DefCfa and DefCfaOffset values used for calculating in/out cfa offset were negative of the ones that end up in cfi directives, but that was not the case for AdjustCfaOffset. I changed it so that the values stored as in/out cfa offset are the values of actual offsets set by cfi directives (and not their negative values).


Repository:
  rL LLVM

https://reviews.llvm.org/D18046