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

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added subscribers: fhahn, reames.
jmorse added a comment.

Looks good in principle; I've added some nits, and there are a couple of things that need to be revised,

- I think the DWARF stack operands go on in the wrong order for casts, plus it needs test coverage,
- Fail gracefully with unrecognised casts rather than asserting.
- InsertedIVs needs to be clear()'able from SCEVExpander

CC @reames and @fhahn who appear to frequently touch SCEV -- this patch exports a vector of induction variables from SCEVExpander, which we assume is OK as SCEVExpander already exports inserted instructions and similar. The induction variables are used to salvage variable locations when a Value is optimised out of a loop during strength reduction. CCing to make you aware of the API change.



================
Comment at: llvm/include/llvm/Transforms/Utils/ScalarEvolutionExpander.h:193-195
+  // THe IVs generated by the expander e.g. "%lsr.iv".
+  SmallVector<WeakVH, 2> InsertedIVs;
+
----------------
The style in this class appears to be that data fields are private and at the top of the class declaration; public accessors near the bottom. IMO, to fit in with the style, this vector should move towards where InsertedValues is declared, with a public accessor that returns a const reference.


================
Comment at: llvm/include/llvm/Transforms/Utils/ScalarEvolutionExpander.h:204
     ReusedValues.clear();
     ChainedPhis.clear();
   }
----------------
You're going to need to clear  InsertedIVs here.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:5582
+  // Select IV as a candidate if it has been inserted.
+  for (WeakVH IV : Rewriter.InsertedIVs)
+    if (IV && dyn_cast<Instruction>(&*IV)->getParent())
----------------
Performance-nit: could be a const reference to WeakVH instead of constructing a new one each time around the loop.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:5910
+        pushOperator(DwarfOp);
+      EmitOperator++;
+    }
----------------
Mega nit, I think the style convention is to pre-increment


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:5923-5924
+                                   : llvm::dwarf::DW_ATE_unsigned};
+    for (const auto &Op : CastOps)
+      pushOperator(Op);
+
----------------
Doesn't the DWARF stack work by operating on things already pushed onto the stack; and thus shouldn't the Inner value be pushed onto the stack before the convert operation is? (Like you do with "div" below).

Possibly I've missed something; but there should also definitely be a test that covers such casting, and it doesn't look like the tests cover anything that produces DW_OP_LLVM_convert.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:5927-5930
+    if (const llvm::SCEVUnknown *U = dyn_cast<SCEVUnknown>(Inner)) {
+      pushValue(cast<SCEVUnknown>(U)->getValue());
+    } else if (const SCEVConstant *C = dyn_cast<SCEVConstant>(Inner)) {
+      pushUInt(C->getAPInt().trunc(ToWidth).getZExtValue());
----------------
Can we not use pushSCEV for Inner in all circumstances and not expand the if/else cases?


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:5942-5943
+      Success &= pushCastImpl(Cast, (isa<SCEVSignExtendExpr>(Cast)));
+    else
+      llvm_unreachable("Unrecognised SCEVCastExpr type.");
+
----------------
This probably wants to return false, rather than assert, better to drop debug-info than stop compiling.

(If the four casts listed cover all cast types, even better would be putting that in a call to assert, rather than using a conditional).


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:5949
+  // TODO: MinMax - although these haven't been encountered in the test suite.
+  bool pushSCEV(const llvm::SCEV *S) {
+    bool Success = true;
----------------
clang-format? I suspect the empty line doesn't meet the style guidelines.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6115
+                                     ScalarEvolution &SE) {
+  // LSR may add locations to previous;y single location-op DVIs which
+  // are currently not supported.
----------------



================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6120-6121
+
+  assert(CachedDVI.DVI->getNumVariableLocationOps() == 1 &&
+         "Expected only one location op.");
+
----------------
Redundant given the above conditional?


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6162-6163
+  bool Changed = false;
+  if (const SCEVAddRecExpr *IVAddRec =
+          dyn_cast<SCEVAddRecExpr>(SCEVInductionVar)) {
+
----------------
clang-format


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6211
+/// Ideally pick the PHi IV inserted by ScalarEvolutionExpander. As a fallback
+/// any PHi from the loop header is usable, but may have less chance of
+/// surviving subsequent transforms.
----------------



================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6219
+  for (WeakVH IV : LSR.ScalarEvolutionIVs) {
+    if (IV) {
+      assert(isa<PHINode>(&*IV) && "Expected Phi node.");
----------------
`if (!IV) continue;`? Avoid extra indentation, same with the extra condition below.


================
Comment at: llvm/test/Transforms/LoopStrengthReduce/debuginfo-scev-variadic-value-0.ll:62
+
+attributes #0 = { nofree nosync nounwind readnone speculatable willreturn }
+
----------------
NB, we usually strip un-necessary attributes to avoid needless maintenance; see also @djtodoro 's meta-burn tool on the mailing list. (Probably not necessary for these tests as they're fairly small already).


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

https://reviews.llvm.org/D105207