[PATCH] D75338: [Coroutines] Use dbg.declare for frame variables

[Jeremy Morse via Phabricator via llvm-commits]

jmorse accepted this revision.
jmorse added a comment.
This revision is now accepted and ready to land.

LGTM with the remaining inline comments addressed, cheers.



================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:793-796
+    SmallVector<DbgVariableIntrinsic *, 2> DbgUsers;
+    findDbgUsers(DbgUsers, A);
+    for (auto *DI : DbgUsers)
+      DI->eraseFromParent();
----------------
This should now be redundant and removed, as replaceDbgUsesWithUndef should have undef'd all debug users.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:747
+      TinyPtrVector<DbgVariableIntrinsic *> DI = FindDbgAddrUses(CurrentValue);
+      if (!DI.empty())
+        DIBuilder(*CurrentBlock->getParent()->getParent(),
----------------
jmorse wrote:
> This might be splitting hairs, but FindDbgAddrUses will also return dbg.addr instructions, and promoting those to be dbg.declares would mess with the validity of other variable locations. Could you std::remove_if DbgAddrIntrinsic's first?
> 
> (I don't think this would happen with C++, but there are other IR producers).
I'd recommend putting findDbgDeclareUses in Local.h/Local.cpp, we already have utilities there for finding {all-dbg-users, debug-values, debug-addresses}, we may as well make this available to other developers.


================
Comment at: llvm/test/Transforms/Coroutines/coro-debug.ll:29-30
 sw.bb:                                            ; preds = %entry
+  call void @llvm.dbg.declare(metadata i32* %x.addr, metadata !12, metadata !13), !dbg !14
+  call void @llvm.dbg.declare(metadata i8** %coro_hdl, metadata !15, metadata !13), !dbg !16
   br label %sw.epilog, !dbg !18
----------------
modocache wrote:
> jmorse wrote:
> > Why are these dbg.declares moving down -- is it to check that they're moved into the entry block later on?
> Right, sorry for the confusion. I mention in the commit message:
> 
> > The existing coro-debug.ll test case is also modified to reflect the locations at which Clang actually places calls to 'dbg.declare', and additional checks are added to ensure this patch works as intended in that example as well.
> 
> This patch adds the CHECK below that the `llvm.dbg.declare` will correctly appear in the coroutine funclet `f.resume`, which makes it possible for debuggers to "find" the variable `x` when stopped in a resumed coroutine function.
> 
> But, the logic in the patch works on the assumption that the original `llvm.dbg.declare` describing the variable `x` appears not in the function's entry block (where Clang places it in a non-coroutine function), but instead in the basic block that represents the coroutine's resumption after its implicit "initial suspend" point (where Clang places it in a coroutine function, since Clang first codegen's the `br` instructions for the implicit suspend point, and then codegen's the function's body).
> 
> These `dbg.declare` don't need to be moved like this, but if they aren't, then the added CHECK below won't succeed -- the patch ensures that `dbg.declare` are migrated to coroutine funclets, but specifically only when dealing with IR in the form that Clang generates it.
Thanks for the explanation!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75338