[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
----------------
"strong no call frame on entry"

The "on entry" seems important because a block with a prologue may have the StrongNoFrameOnEntry flag set.


================
Comment at: llvm/lib/CodeGen/CFIFixup.cpp:87
+static bool containsEpilogue(const MachineBasicBlock &MBB) {
+  return llvm::any_of(make_range(MBB.rbegin(), MBB.rend()), [](const auto &MI) {
+    return MI.getOpcode() == TargetOpcode::CFI_INSTRUCTION &&
----------------
`llvm::reverse(MBB)`

Though using reverse iteration may not improve much.


================
Comment at: llvm/lib/CodeGen/CFIFixup.cpp:113
+  ReversePostOrderTraversal<MachineBasicBlock *> RPOT(&*MF.begin());
+  for (auto *MBB : RPOT) {
+    auto &Info = BlockInfo[MBB->getNumber()];
----------------



================
Comment at: llvm/lib/CodeGen/CFIFixup.cpp:114
+  for (auto *MBB : RPOT) {
+    auto &Info = BlockInfo[MBB->getNumber()];
+
----------------



================
Comment at: llvm/lib/CodeGen/CFIFixup.cpp:114
+  for (auto *MBB : RPOT) {
+    auto &Info = BlockInfo[MBB->getNumber()];
+
----------------
MaskRay wrote:
> 



================
Comment at: llvm/lib/CodeGen/CFIFixup.cpp:136
+    for (auto *Succ : MBB->successors()) {
+      auto &SuccInfo = BlockInfo[Succ->getNumber()];
+      SuccInfo.StrongNoFrameOnEntry |=
----------------



================
Comment at: llvm/lib/CodeGen/CFIFixup.cpp:138
+      SuccInfo.StrongNoFrameOnEntry |=
+          Info.StrongNoFrameOnEntry && !HasPrologue && !HasEpilogue;
+      SuccInfo.HasFrameOnEntry = Info.HasFrameOnExit;
----------------
` && !HasEpilogue` seems unneeded.


================
Comment at: llvm/lib/CodeGen/CFIFixup.cpp:159
+  MachineBasicBlock::iterator InsertPt = PrologueBlock->begin();
+  for (MachineInstr &MI : PrologueBlock->instrs()) {
+    if (isPrologueCFIInstruction(MI))
----------------
delete braces

`for (MachineInstr &MI : *PrologueBlock)` is more common.



================
Comment at: llvm/lib/CodeGen/CFIFixup.cpp:145
+      unsigned CFIIndex =
+          MF.addFrameInst(MCCFIInstruction::createRememberState(nullptr));
+      BuildMI(MBB, MBB.begin(), DebugLoc(),
----------------
chill wrote:
> jrtc27 wrote:
> > Having both even in the case when you don't need it is a bit weird. You should be able to just keep track of the last MBB seen that had a frame and insert the remember at the end of it when you discover you need a restore in the current MBB. That also removes the need to special-case the prologue.
> > 
> > (This code is also confusing due to using MBB.begin() twice rather than an interator that gets advanced, as the insertion order ends up being backwards so the code looks the wrong way round at first glance)
> Indeed, thanks for the suggestion!
... Every block inherits the frame state (as recorded in the unwind tables) of the previous block. If the intended frame state is different, insert compensating CFI instructions.


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

https://reviews.llvm.org/D114545