[PATCH] D98644: [DebugInfo] Fix incorrect handling of variadic debug values

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

I like the direction of the SelectionDAG change; reducing duplication of information is a worthy goal. My knee-jerk reaction is that we need a test for it, but I imagine it's hard to hit something that deep into DAG Combining. If you've got a reproducer to hand, it'd be good to add that as a test, if not maybe we can skip it. IMO it's also worth breaking this off from the LSR modification.

For the LSR part: this definitely wants a test, it's a fairly fragile scenario (recovering a dbg.value that's been salvaged to death), and it seems likely that future patches could miss this scenario. I've put some minor nits inline, but otherwise this all LGTM (with a test).



================
Comment at: llvm/lib/CodeGen/SelectionDAG/SDNodeDbgValue.h:141
   LocOpVector LocationOps;
-  SDNodeVector SDNodes;
+  SDNodeVector AdditionalDependencies;
   DIVariable *Var;
----------------
Probably wants a comment explaining what they're in addition _to_.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:5833-5834
 using EqualValues = SmallVector<std::tuple<WeakVH, int64_t>, 4>;
 using EqualValuesMap =
-    DenseMap<std::pair<DbgValueInst *, unsigned>, EqualValues>;
-using ExpressionMap = DenseMap<DbgValueInst *, DIExpression *>;
+    DenseMap<DbgValueInst *, SmallVector<std::pair<unsigned, EqualValues>>>;
+using LocationMap =
----------------
To confirm my understanding: this swapping of key/value is effectively NFC, right?


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:5868-5871
+            DVI->getExpression(),
+            DVI->hasArgList() ? DVI->getRawLocation()
+                              : ValueAsMetadata::get(UndefValue::get(
+                                    DVI->getVariableLocationOp(0)->getType()))};
----------------
A comment pointing out that this is to preserve the syntactic structure of variadic variable locations would help the reader. Producing the `Metadata*` value as a separately named variable would let you describe its intention in the variables name too.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:5884-5885
       continue;
-    for (auto EV : A.second) {
-      auto EVHandle = std::get<WeakVH>(EV);
-      if (!EVHandle)
-        continue;
-      // The dbg.value may have had its value changed by LSR; refresh it from
-      // the map, but continue to update the mapped expression as it may be
-      // updated multiple times in this function.
-      auto DbgDIExpr = DbgValueToExpression[DVI];
-      auto Offset = std::get<int64_t>(EV);
-      DVI->replaceVariableLocationOp(Idx, EVHandle);
-      if (Offset) {
-        SmallVector<uint64_t, 8> Ops;
-        DIExpression::appendOffset(Ops, Offset);
-        DbgDIExpr = DIExpression::appendOpsToArg(DbgDIExpr, Ops, Idx, true);
+    // The dbg.value may have had its value or expression changed by LSR;
+    // refresh them from the map.
+    auto DbgDIExpr = DbgValueToLocation[DVI].first;
----------------
IMO: this should explicitly say "in a failed salvage attempt", to avoid giving the impression LSR does any salvaging itself right now. I'd also say "reset" rather than refresh, as the latter implies it's making it valid (to me at least, YMMV).


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:5890
+    for (auto IdxEV : A.second) {
+      auto Idx = IdxEV.first;
+      for (auto EV : IdxEV.second) {
----------------
Opinion: just declare `Idx` as unsigned? Avoids un-necessary uncertainty.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:5906
     }
   }
 }
----------------
Am I right in thinking that when we reset the operands / expression, and we fail to find an equal Value to replace an operand with, we'll still be left with an undef dbg.value? IMO it'd be good to encode this as an assertion somehow -- it would communicate to the reader our expectation is that an equal expr is found, OR the dbg.value exits the loop as undef. Not finding an equal expr and still being valid should be illegal (AFAIUI).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98644