[PATCH] D111810: [DebugInfo][LSR] Add more stringent checks on IV selection and cached dbg.value location-op SCEVs

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando added a comment.

Hi @chrisjackson, I added a couple of nits inline. Is there anyone you can add as reviewer that are more familiar with LSR/SCEV?



================
Comment at: llvm/include/llvm/Analysis/ScalarEvolution.h:540
 
+  /// Return true if the SCEV expression contains an undef value;
+  bool containsUndefs(const SCEV *S) const;
----------------
nit: in the comment `;` -> `.`


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6262
 /// Identify and cache salvageable DVI locations and expressions along with the
 /// corresponding SCEV(s). Also ensure that the DVI is not deleted before
 static void
----------------
Could you fix this comment while you're here? (`Also ensure that the DVI is not deleted before ...`)


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6322-6323
 
-    assert(isa<PHINode>(&*IV) && "Expected PhI node.");
-    if (SE.isSCEVable((*IV).getType())) {
-      PHINode *Phi = dyn_cast<PHINode>(&*IV);
-      LLVM_DEBUG(dbgs() << "scev-salvage: IV : " << *IV
-                        << "with SCEV: " << *SE.getSCEV(Phi) << "\n");
-      return Phi;
-    }
-  }
-
-  for (PHINode &Phi : L.getHeader()->phis()) {
-    if (!SE.isSCEVable(Phi.getType()))
+    PHINode *P = dyn_cast<PHINode>(&*IV);
+    if (!P)
       continue;
----------------
Is this change necessary before IV might be `UndefValue`? If that's the case, it may be clearer to do this:
```
if (isa<UndefValue>(&*IV))
    continue;
PHINode *P = cast<PHINode>(&*IV);
```
Or am I misunderstanding?


================
Comment at: llvm/test/Transforms/LoopStrengthReduce/pr52161.ll:4
+;; Ensure that scev-based salvaging in LSR does not select an IV containing
+;; an 'undef' element
+
----------------
nit: missing full stop.


================
Comment at: llvm/test/Transforms/LoopStrengthReduce/pr52161.ll:6-7
+
+; CHECK: scev-salvage: SCEV salvaging not possible. An IV could not be identified.
+; CHECK: scev-salvage: SCEV salvaging not possible. An IV could not be identified.
+
----------------
I wonder if the `-debug` output is necessary? As far as the IR output is concerned, we just want to see an undef dbg.value is generated but don't need to know how we arrived there. What do you think?


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

https://reviews.llvm.org/D111810