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

[Kyle Butt via Phabricator via llvm-commits]

iteratee added a comment.

This is looking pretty good. I'm going to go back over, but most of my initial concerns have been satisfied.



================
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
----------------
violetav wrote:
> 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.
Thanks. Can you shrink the example: 
BB1:
...
INSTRUCTION_A
ADD32ri8
...

BB2:
...
INSTRUCTION_B
CFI_INSTRUCTION

and include it in the comments?


Repository:
  rL LLVM

https://reviews.llvm.org/D18046