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

[Momchil Velikov via Phabricator via llvm-commits]

chill marked 16 inline comments as done.
chill 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
----------------
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)


================
Comment at: llvm/lib/CodeGen/CFIFixup.cpp:181
+#endif
+    if (!Info.StrongNoFrameOnEntry && Info.HasFrameOnEntry && !HasFrame) {
+      // Reset to the "after prologue" state.
----------------
MaskRay wrote:
> MaskRay wrote:
> > Super nit: `if (!HasFrame && Info.HasFrameOnEntry && !Info.StrongNoFrameOnEntry) {`
> > 
> > HasFrame represents the previous state and Info.HasFrameOnEntry represents the current state. The order implies a state transition.
> > 
> > ---
> > 
> > The term "Reset" seems to imply transiting to the initial state.
> > 
> > "Transit to" may be better. To describe `!HasFrame` as well, the comment is better changed to something like `Transit from ... state to ... state`.
> Actually, I think `Info.StrongNoFrameOnEntry` can just be removed.
> 
> Info.HasFrameOnEntry implies that this is a block in "after prologue" state and `Info.StrongNoFrameOnEntry` must be false.
My mental model is that both represent the "opinion" of the unwind tables on one hand (`HasFrame`), and the "opinion" of the  CFG, on the other hand (`Info.HasFrameOnEntry`), about  the current state at entry of the block.  CFG is right (as far as we can tell form the available CFI instructions) and we fix the unwind tables.


================
Comment at: llvm/lib/CodeGen/CFIFixup.cpp:181
+#endif
+    if (!Info.StrongNoFrameOnEntry && Info.HasFrameOnEntry && !HasFrame) {
+      // Reset to the "after prologue" state.
----------------
chill wrote:
> MaskRay wrote:
> > MaskRay wrote:
> > > Super nit: `if (!HasFrame && Info.HasFrameOnEntry && !Info.StrongNoFrameOnEntry) {`
> > > 
> > > HasFrame represents the previous state and Info.HasFrameOnEntry represents the current state. The order implies a state transition.
> > > 
> > > ---
> > > 
> > > The term "Reset" seems to imply transiting to the initial state.
> > > 
> > > "Transit to" may be better. To describe `!HasFrame` as well, the comment is better changed to something like `Transit from ... state to ... state`.
> > Actually, I think `Info.StrongNoFrameOnEntry` can just be removed.
> > 
> > Info.HasFrameOnEntry implies that this is a block in "after prologue" state and `Info.StrongNoFrameOnEntry` must be false.
> My mental model is that both represent the "opinion" of the unwind tables on one hand (`HasFrame`), and the "opinion" of the  CFG, on the other hand (`Info.HasFrameOnEntry`), about  the current state at entry of the block.  CFG is right (as far as we can tell form the available CFI instructions) and we fix the unwind tables.
If there are missing unwind instructions in the epilogue, a block can have `HasFrameOnEntrySet`, even though it's reachable from function entry without passing through a prologue. In this case `StroingNoFrameOnEntry` will be true. Example of this is `llvm/test/CodeGen/AArch64/fast-isel-branch-cond-split.ll`


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

https://reviews.llvm.org/D114545