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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/lib/CodeGen/CFIFixup.cpp:28
+// In order to acommodate backends, which do not generate unwind info in
+// epilogues we compute an additional property "strong no call frame", which
+// is set for the entry point of the function and for every block reachable
----------------
chill wrote:
> MaskRay wrote:
> > MaskRay wrote:
> > > "strong no call frame on entry"
> > > 
> > > The "on entry" seems important because a block with a prologue may have the StrongNoFrameOnEntry flag set.
> > Does the Info.StrongNoFrameOnEntry semantic drift from the description here? In addition, removing `!Info.StrongNoFrameOnEntry && ` from the .cfi_remember_state code path's condition below doesn't break any test, which indicates missing coverage.
> There was a bug in the conditions, that I fixed and it triggers in a few of the tests (those with `.cfi_same_value` appearing)
Thanks:) I think I fully understand it now.


================
Comment at: llvm/lib/CodeGen/CFIFixup.cpp:9
+//
+// This pass inserts the necessary .cfi_remember_state and
+// .cfi_restore_state CFI instructions to adjust for the inconsistency of
----------------
"This pass inserts the necessary .cfi_remember_state/.cfi_remember_state and calls resetCFIToInitialState"

`resetCFIToInitialState` may insert CFI instructions other than  .cfi_remember_state/.cfi_remember_state



================
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",
----------------
I think this comment should be moved before `StrongNoFrameOnEntry` is declared in the code below.


================
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.
----------------
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)"


================
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
----------------
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`.


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

https://reviews.llvm.org/D114545