[PATCH] D64595: [Debuginfo][SROA] Need to handle dbg.value in SROA pass.

[Vedant Kumar via Phabricator via llvm-commits]

vsk added a reviewer: debug-info.
vsk added a comment.

Thanks for the patch!

I think the deleteDeadInstructions change looks good, but have some reservations about the way new dbg.declares are inserted. More inline --



================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:4250
+
+void findDeclarations(SmallVectorImpl<DbgVariableIntrinsic *> &Declarations,
+                      Value *V) {
----------------
Why not filter the result from llvm::findDbgUsers?


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:4376
+  // for dbg.value also.
+  SmallVector<DbgVariableIntrinsic *, 0x10> Declarations;
+  findDeclarations(Declarations, &AI);
----------------
Typically, container sizes are written in decimal, not hex.


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:4379
+
+  for (auto Decl : Declarations) {
+
----------------
Iterating over a DenseSet will give non-deterministic results. You need a vector-like container (or perhaps a SetVector) here.


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:4380
+  for (auto Decl : Declarations) {
+
+    auto *Var = Decl->getVariable();
----------------
Please avoid inserting vertical whitespace after open curly braces, and/or whitespace changes in code that is otherwise not being modified.


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:4392
       // expression if there is only one partition.
       auto *FragmentExpr = Expr;
+
----------------
I'm not sure this fragment expr is valid for, say, dbg.values with OP_deref attached. I think this would result in a dbg.declare with an OP_deref, which doesn't seem to make sense. Currently you're uniquing debug intrinsics by variable name, so you might not hit the issue right away, but eventually it'll pop up.

Perhaps it'd help to start with simple expressions, i.e. empty expressions, and then incrementally build on that + add tests.




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

https://reviews.llvm.org/D64595