[PATCH] D114545: [CodeGen] Async unwind - add a pass to fix CFI information

[Momchil Velikov via Phabricator via llvm-commits]

chill marked 6 inline comments as done.
chill added inline comments.


================
Comment at: llvm/lib/CodeGen/CFIFixup.cpp:27
+
+// In order to acommodate backends, which do not generate unwind info in
+// epilogues we compute an additional property "strong no call frame on entry",
----------------
MaskRay wrote:
> I think this comment should be moved before `StrongNoFrameOnEntry` is declared in the code below.
I'd prefer to have an overall description in one place at the start of the file.  It easy to relate the name `StrongNoFrameOnEntry`  to the description.


================
Comment at: llvm/lib/CodeGen/CFIFixup.cpp:166
+  // No point starting before the prologue block.
+  // TODO: the unwind tables will still be incorrect if an epilogue physically
+  // preceeds the prologue.
----------------
MaskRay wrote:
> Delete the TODO: I believe this is not a TODO, but an assumption. The stack based remember/restore approach just cannot handle epilogue before prologue in theory.
> 
> This is what I wanted to express but probably not in a clear way:
> "If we consider (a) a prologue, (b) an epilogue, or (c) a region delimitered by a pair of prologue/epilogue, as a vertex in a graph,
> the algorithm can handle any linear basic block layout which is a pre-order traversal.
> (This condition is sufficient but not necessary: if we require that all prologues are the same, more traversal orders can be handled)"
I've moved it to the start of the file. If an actual need appears, I think it's implementable
and the pass need not restrict itself to remember/restore state.


================
Comment at: llvm/lib/CodeGen/CFIFixup.cpp:195
+                     .addCFIIndex(CFIIndex);
+      ++InsertPt;
+      InsertMBB = &*CurrBB;
----------------
MaskRay wrote:
> It would be nice to have a test that a cfi_remember_state is inserted after a cfi_restore_state.
> Right now if I delete `++InsertPt` I don't break any test.
`cfi-fixup.mir` tests this now.


================
Comment at: llvm/test/CodeGen/AArch64/arm64-shrink-wrapping.ll:29
 ; ENABLE-NEXT:  LBB0_2: ; %false
+; ENABLE-NEXT:    .cfi_def_cfa wsp, 0
+; ENABLE-NEXT:    .cfi_same_value w18
----------------
MaskRay wrote:
> Thanks for the update to `foo`. It makes me understand the purpose of `StrongNoFrameOnEntry`. Right now there is no epilogue CFI, so BB1's `HasFrameOnExit` is (incorrectly) false. We rely on  `StrongNoFrameOnEntry` being true to generate `.cfi_def_cfa wsp, 0` and `.cfi_same_value *`.
> 
> However, using `MF.getFrameInfo().getCalleeSavedInfo();` has the problem that we may add redundant `.cfi_same_value`. As this test shows, none of w18/w30/w29 is updated in previous blocks but we add them: (a) it unnecessarily increases unwind table size (b) it may confuse a reader.
> 
> We may need to check whether the registers have actually been modified by previous CFI instructions, before emitting `.cfi_same_value`.
These (except for `x18`) are not redundant. At `LBB0_2` the unwind info says the CSR are on the stack, but they aren't there.

`x18` is because of it's potential use as a shadow call stack pointer, I've fixed the code to emit `.cfi_same_value x18` only if the function indeed uses shadow stack.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114545/new/

https://reviews.llvm.org/D114545