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

[Momchil Velikov via Phabricator via llvm-commits]

chill added inline comments.


================
Comment at: llvm/lib/CodeGen/CFIFixup.cpp:190
+      ++InsertPt;
+      InsertMBB = &*CurrBB;
+      Change = true;
----------------
jrtc27 wrote:
> jrtc27 wrote:
> > Ok but doing that here doesn't make sense, now you restore back to the prologue but other things may have happened? I was imagining updating the last MBB with a frame whenever Info.HasFrameOnExit. Then you also don't need InsertPt, just insert at the end of the that MBB. That'll keep the remember/restore tight around just the MBBs that clobber the CFI state.
> Although that doesn't work when you have multiple epilogues next to each other. You can instead keep InsertPt lying around, and updated it to CurrBB->end() whenever HasFrameOnExit is true, and in this case here set it to what you currently do just in case you don't see a non-epilogue frame before the next restore is needed.
I'm not sure I understand.

Conceptually, we do two operations: reset to initial state and reset to after-prologue state.
The pair `.cfi_restore_state`/`.cfi_remember_state` is a workaround for the lack of suitable CFI directive that sets the current state without popping the stack, in other words, these make sense to go together.

As an optimisation from this base case, as you suggested, we can omit the last `.cfi_remember_state`, saving a bit of space in the unwind tables.

Keeping directives adjacent to one another reduces the size of the unwind tables, as you don't need intervening `DW_CFA_advance_loc` instructions.

Do you suggest the instead of inserting `.cfi_remember_state` just after the  last `.cfi_restore_state` we could insert it at the end of the last block, known to have a stack frame?
That would separate the directive from the previous `.cfi_restore_state`.

Also, I think I had an earlier variant that inserted at the `->end()` of blocks, which caused failures in the verifier. Didn't investigate it further, but that would mean the directive has to go before the terminator, which would separate it from a following `.cfi_restore_state`, too.

Apologies if I misunderstood you.


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

https://reviews.llvm.org/D114545