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

[Stephen Tozer via Phabricator via llvm-commits]

StephenTozer added a comment.

Good patch - I've left quite a few comments, but most of them are minor nits and some are not requirements for this patch to be merged.



================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6154
+  // Location index 0.
+  void appendToVectors(SmallVectorImpl<uint64_t> &DestExpr,
+                       SmallVectorImpl<Value *> &DestLocations,
----------------
chrisjackson wrote:
> 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?
Yeah, you'd need to check the `DestLocations` vector. I think it probably wouldn't be a significant performance cost to do this - the arrays are generally small (there's a relatively low hard limit on the number of ops) so it should be feasible to implement a solution in which you search DestLocations for each new LocationOp and then, if it is already present, you track that location's ArgIndex instead of adding a new one. Then you can look at that tracked index to find NewIndex. Example solution included below, though if it doesn't work and/or the problem proves more difficult than I've made it out to be then we could :


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:5919
   /// in the set of values referenced by the expression.
   void pushValue(llvm::Value *V) {
     Expr.push_back(llvm::dwarf::DW_OP_LLVM_arg);
----------------
I suppose with the variable rename it may make sense to rename this function?


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6088-6089
 
   /// Combine a translation of the SCEV and the IV to create an expression that
   /// recovers a location's value.
+  bool createIterCountExpr(const SCEV *S,
----------------
Nit, probably worth adding a comment for the new return value.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6165-6179
+    uint64_t IndexOffset = DestLocations.size() - 1;
+    DestLocations.insert(DestLocations.end(), LocationOps.begin() + 1,
+                         LocationOps.end());
+
+    for (const auto &Op : expr_ops()) {
+      if (Op.getOp() != dwarf::DW_OP_LLVM_arg) {
+        Op.appendToVector(DestExpr);
----------------
Example impl for the above suggestion.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6211-6215
+  if (Ops[0] == dwarf::DW_OP_LLVM_arg && Ops[1] == 0) {
+    llvm::SmallVector<uint64_t, 6> ShortenedOps(llvm::drop_begin(Ops, 2));
+    DVI.setExpression(DIExpression::get(DVI.getContext(), ShortenedOps));
+  } else
+    DVI.setExpression(DIExpression::get(DVI.getContext(), Ops));
----------------
There should probably be an assertion in this function that `DW_OP_LLVM_arg` appears either exactly once in the DIExpr as the first operator, or not at all. Actually, it is also possible that `DW_OP_LLVM_arg, 0` could legally appear multiple times in the expression, which would be a little awkward to deal with. For example, if we salvaged the square of the IV, the expression might be `DIExpr(DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 0, DW_OP_mul)`.

I think the suitable answer for this patch would just be to drop such expressions for now, so that it's not a blocker. As a slight tangent though, there is a general solution possible; for an expression `DIExpr([[A]], DW_OP_LLVM_arg, 0, [[B]])`, it is possible to transform it to a non-variadic form like so: `DIExpr(DW_OP_dup, [[A]], DW_OP_swap, [[B]])`. Personally I'd be happy with this not being added in this patch though, it's a bit much imo.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6249-6251
+  if (NumPreLSRElements == 0 && SalvageExpression->getNumElements() != 0) {
+    SalvageExpression =
+        DIExpression::append(SalvageExpression, {dwarf::DW_OP_stack_value});
----------------
This check could be simplified and made more correct by instead using `DIExpr->isComplex()`, since that also checks for DW_OP_LLVM_fragment and DW_OP_LLVM_tag_offset (which don't force stack_value). Also, as in the previous patch, it'd be better to use DIExpression's prepend function that accounts for fragments than directly appending the stack_value.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6277
+           "Unexpected number of location ops.");
+    // LSR's unsuccesful salvage attempt may have added DIArgList, which in this
+    // case was not present before, so force the location back to a single
----------------
Nit


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6360
+  SmallVector<uint64_t, 3> NewExpr;
+  if (DVIRec.Expr->getNumElements() == 0) {
+    assert(DVIRec.RecoveryExprs.size() == 1 &&
----------------
As above, this could use `isComplex()` instead.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6379
+        DVIRec.RecoveryExprs[LocationArgIndex].get();
+    // The location doesn't have s SCEVDbgValueBuilder, so LSR did not
+    // optimise it away. So just translate the argument to the updated
----------------



================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6461
 
-      if (!DVI->getVariableLocationOp(0) ||
-          !SE.isSCEVable(DVI->getVariableLocationOp(0)->getType()))
-        continue;
+          if (!SE.isSCEVable(LocOp->getType()))
+            return false;
----------------
Doesn't need to be dealt with in this patch, just a suggestion that might be worth looking at later:

It would make sense to add an early continue here (and in the loop below) if LocOp is a constant, since we never need to salvage those ops anyway; though if DVI contains a non-SCEVable constant Op it's //probably// using non-SCEVable values anyway, so I don't think we'd lose many values tothise. The implementation would also touch a bunch of code around here, so is probably not worth the effort right now.



================
Comment at: llvm/test/Transforms/LoopStrengthReduce/debuginfo-scev-salvage-5.ll:21
+;; of length two and three.
+;; A third dbg.value was added artificially by copying a generated dbg.value
+;; and the modifying the position of the optimised-out value in the location
----------------
I think this is the fourth value now?


================
Comment at: llvm/test/Transforms/LoopStrengthReduce/debuginfo-scev-salvage-5.ll:79-80
+
+attributes #0 = { "target-cpu"="x86-64" }
+attributes #1 = { nofree nosync nounwind readnone speculatable willreturn }
+
----------------
Can remove these I think.


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

https://reviews.llvm.org/D120169