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

[Chris Jackson via Phabricator via llvm-commits]

chrisjackson added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6154
+  // Location index 0.
+  void appendToVectors(SmallVectorImpl<uint64_t> &DestExpr,
+                       SmallVectorImpl<Value *> &DestLocations,
----------------
StephenTozer wrote:
> 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?
I agree that would be the most efficient use of the location ops arguments, but at the time of writing I though this would add some additional complexity. However, it now seems not so difficult.

I had planned to to implement this in an additional patch, but I'm happy to add the functionality to this. But would it then be necessary to search the destination location vector to check if an append is necessary or location-reuse is possible?


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6192
+  SmallVector<const llvm::SCEV *, 2> SCEVs;
+  DenseMap<uint64_t, SCEVDbgValueBuilder *> RecoveryExprs;
+
----------------
StephenTozer wrote:
> 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).
Agreed, I was concerned with the potential cost of DenseMap, though it is convenient.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6252
 
-  SCEVDbgValueBuilder SalvageExpr;
+  DenseMap<uint64_t, uint64_t> LocationOpIndexMap;
+  SmallVector<Value *, 2> NewLocationOps = {LSRInductionVar};
----------------
StephenTozer wrote:
> 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).
Agreed, thanks.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6284
+
+    SCEVDbgValueBuilder *SalvageExpr = new SCEVDbgValueBuilder();
+    DVIRec.RecoveryExprs[i] = SalvageExpr;
----------------
StephenTozer wrote:
> 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).
I thought I had taken care of this with the clear() methods. Perhaps I implemented them incorrectly. Either way, I agree unique_ptr would be nicer.


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

https://reviews.llvm.org/D120169