[PATCH] D120169: [Debuginfo][LSR} Add support for salvaging variadic dbg.value intrinsics [2/2]

[Stephen Tozer via Phabricator via llvm-commits]

StephenTozer added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6154
+  // Location index 0.
+  void appendToVectors(SmallVectorImpl<uint64_t> &DestExpr,
+                       SmallVectorImpl<Value *> &DestLocations,
----------------
Could this method be implemented as a more general merge of locations, so that every location in `LocationOps` that already appears in `DestLocations` is reused, rather than just the IV?


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6192
+  SmallVector<const llvm::SCEV *, 2> SCEVs;
+  DenseMap<uint64_t, SCEVDbgValueBuilder *> RecoveryExprs;
+
----------------
This would be better as a SmallVector pre-allocated with nullptrs imo - besides the vector having faster lookup, it also would use much less space (DenseMap automatically allocates space for 64 key+value pairs, which is 4 times as many as a DIArgList should ever actually contain).


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6252
 
-  SCEVDbgValueBuilder SalvageExpr;
+  DenseMap<uint64_t, uint64_t> LocationOpIndexMap;
+  SmallVector<Value *, 2> NewLocationOps = {LSRInductionVar};
----------------
Same as the above comment for `RecoveryExprs`, this would probably be more efficient as a SmallVector with some placeholder value for "empty" (`_UI64_MAX` or something to that effect).


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6264
+    // DW_OP_LLVM_arg arguments as the expression is updated.
+    if (!isa<UndefValue>(VH)) {
+      NewLocationOps.push_back(VH);
----------------
Since `VH` is only used in this if-block, the above check could be folded in here as `if (VH && !isa<UndefValue>(VH)) {`


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6277
+      LLVM_DEBUG(dbgs() << "scev-salvage: SCEV for value at index: " << i
+                        << " now refers to an undef. Salvage abaondoned.\n");
+      return false;
----------------



================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6284
+
+    SCEVDbgValueBuilder *SalvageExpr = new SCEVDbgValueBuilder();
+    DVIRec.RecoveryExprs[i] = SalvageExpr;
----------------
This needs to be freed at some point, could be either in the DVIRecoveryRec destructor, or this could be a unique_ptr in DVIRecoveryRec instead (which is probably safer).


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

https://reviews.llvm.org/D120169