[PATCH] D105207: [debuginfo][lsr] SCEV-based salvaging for LoopStrengthReduction

[Jeremy Morse via Phabricator via llvm-commits]

jmorse accepted this revision.
jmorse added a comment.
This revision is now accepted and ready to land.

Thanks for the additional test; ideally the "switching dbg.value(!DIArgList..." back to single-value dbg.values would be covered by a test case too, just to ensure that this doesn't change in the future. LGTM with some nits.



================
Comment at: llvm/include/llvm/Transforms/Utils/ScalarEvolutionExpander.h:86
 
+  // THe induction variables generated.
+  SmallVector<WeakVH, 2> InsertedIVs;
----------------



================
Comment at: llvm/include/llvm/Transforms/Utils/ScalarEvolutionExpander.h:209
   ScalarEvolution *getSE() { return &SE; }
+  const SmallVector<WeakVH> &getInsertedIVs() const { return InsertedIVs; }
 
----------------
IIRC, this needs to be SmallVectorImpl to abstract the size of the underlying vector from the way of accessing it.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:2091
   bool getChanged() const { return Changed; }
+  const SmallVector<WeakVH, 2>  &getScalarEvolutionIVs() const {
+    return ScalarEvolutionIVs;
----------------
Similarly wants to be SmallVectorImpl to avoid relying on the vector size detail. 


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:5964-5966
+      assert((isa<SCEVZeroExtendExpr>(Cast) || isa<SCEVTruncateExpr>(Cast) ||
+              isa<SCEVPtrToIntExpr>(Cast) || isa<SCEVSignExtendExpr>(Cast)) &&
+             "Unexpected cast type in SCEV.");
----------------
I don't quite get why we're willing to return-false if there are unrecognised operators; but assert false if there are unrecognised cast operators. Are these four classes the only derived classes from SCEVCastExpr, and the idea is to fire the assertion if someone adds another? If so, a comment to that effect would be good.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6175-6185
+      // Some DVIs that were single location-op when cached are now multi-op,
+      // due to LSR optimisations. However, multi-op salvaging is not yet
+      // supported by SCEV salvaging. But, we can attempt a salvage by restoring
+      // the pre-LSR single-op expression.
+      if (DVIRec.DVI->hasArgList()) {
+        llvm::Type *Ty = DVIRec.DVI->getVariableLocationOp(0)->getType();
+        DVIRec.DVI->setRawLocation(
----------------
Ideally would want a test.


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

https://reviews.llvm.org/D105207