[PATCH] D97673: [RFC] [[Coroutine] [Debug] Salvage dbg.value at O2

Jeremy Morse via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Mar 24 08:34:12 PDT 2021


jmorse added a comment.

When looping over the debug users of instructions and replacing the dbg.values operands, you should now use the "replaceVariableLocationOp" helper instead -- this avoids re-creating a dbg.value intrinsic, and should handle variadic variable locations (the patches for which are 95% landed) seamlessly.

Overall the aim of the patch makes sense to me, it's storing values used by debug intrinsics in the coroutine frame for later retrieval right? Is there a risk that some later coroutine optimisation will try to optimise the frame and overwrite the stored values-for-debug-users -- this can happen with stack slot colouring at the other end of the compiler.

I'm not sure how the rest of the community will feel about the approach; IIRC there's a "fake use" patch floating around that does this for all (non-coroutine) variables, which wasn't landed in the end.



================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:53
+    "enhance-debug-with-coroutine", cl::Hidden,
+    cl::desc("Try to salvage as many debug infomation as possible. \
+         This option may enlarge the size of coroutine frame."),
----------------
IMO: the description should explicitly refer to the fact that codegen will change as a result of -g, to avoid any users experiencing unexpected behaviour.


================
Comment at: llvm/test/Transforms/Coroutines/coro-debug-dbg.values-O2.ll:6-8
+; CHECK:       entry.resume:
+; CHECK:         call void @llvm.dbg.value(metadata %f.Frame** %FramePtr{{.*}}, metadata ![[IVAR_RESUME:[0-9]+]], metadata !DIExpression(
+; CHECK:         call void @llvm.dbg.value(metadata %f.Frame** %FramePtr{{.*}}, metadata ![[JVAR_RESUME:[0-9]+]], metadata !DIExpression(
----------------
nit: IMO you should check that the `dbg.value`s operand is a specific LLVM-IR value (i.e., the frame pointer), rather than just checking the type. That protects against some future optimisation or error producing a pointer-typed `undef` as the `dbg.value` operand.


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

https://reviews.llvm.org/D97673



More information about the llvm-commits mailing list