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

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

Thanks for pushing through and finding out how to improve co-routine variable locations Brian; I have a few comments inline. Alas, I can't help with the lldb tests.

We do have Dexter too [0] for running integration tests, although I've no idea whether it'll play nice with coroutines.

[0] https://github.com/llvm/llvm-project/tree/master/debuginfo-tests/dexter



================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:747
+      TinyPtrVector<DbgVariableIntrinsic *> DI = FindDbgAddrUses(CurrentValue);
+      if (!DI.empty())
+        DIBuilder(*CurrentBlock->getParent()->getParent(),
----------------
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).


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:768
+  // we also delete the original dbg.declare.
+  // Note: We cannot do replace the alloca with GEP instructions
+  // indiscriminately, as some of the uses may not be dominated by CoroBegin.
----------------
"We cannot replace"?


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:776-779
+    SmallVector<DbgVariableIntrinsic *, 2> DbgUsers;
+    findDbgUsers(DbgUsers, A);
+    for (auto *DI : DbgUsers)
+      DI->eraseFromParent();
----------------
This will also erase dbg.value intrinsics, which I think will correspond to pointer variables that point(ed) at an alloca losing a location. Those dbg.values should be set to "undef" instead of being deleted, so that they terminate any earlier variable locations. I'd suggest using FindDbgAddrUses to get dbg.declares and delete those; then calling replaceDbgUsesWithUndef on the alloca. Or alternately, delete the dbg.declare's when they're migrated, and just call replaceDbgUsesWithUndef on the alloca.


================
Comment at: llvm/test/Transforms/Coroutines/coro-debug-frame-variable.ll:33
+; CHECK:         [[JGEP:%.+]] = getelementptr inbounds %f.Frame, %f.Frame* %FramePtr, i32 0, i32 5
+; CHECK:         call void @llvm.dbg.declare(metadata i32* [[JGEP]], metadata ![[JVAR:[0-9]+]], metadata !DIExpression()), !dbg !13
+;
----------------
These explicitly numbered metadata nodes (!dbg !13) should be replaced with pattern matching, as you've done a few lines down.


================
Comment at: llvm/test/Transforms/Coroutines/coro-debug-frame-variable.ll:218
+
+attributes #0 = { nounwind readnone speculatable willreturn }
+attributes #1 = { argmemonly nounwind readonly }
----------------
NB, we usually delete any un-necessary attributes.


================
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
----------------
Why are these dbg.declares moving down -- is it to check that they're moved into the entry block later on?


================
Comment at: llvm/test/Transforms/Coroutines/coro-debug.ll:133
+; CHECK: entry.resume:
+; CHECK:   call void @llvm.dbg.declare(metadata i32* %x.addr.reload.addr, metadata ![[RESUME_VAR:[0-9]+]]
 ; CHECK: define internal fastcc void @f.destroy(%f.Frame* noalias nonnull %FramePtr) #0 !dbg ![[DESTROY:[0-9]+]]
----------------
This might need to be CHECK-NEXT if you want to ensure the dbg.declare is in the entry.resume block, not just after the label.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75338