[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 inline comments.


================
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;
----------------
chrisjackson wrote:
> Orlando wrote:
> > chrisjackson wrote:
> > > Orlando wrote:
> > > > 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?
> > > Not exactly. The IV's SCEV may contain an undef. I don't think that is the same as the IV value being undef,
> > Ah ok, then - sorry if this is a silly question - why have you weakened the assert `assert(isa<PHINode>(&*IV) && "Expected PhI node.");` to a dyn_cast-check?
> This was an oversight, Thanks for the correction.
I think you could simplify this (sorry for the continued nits), from:
```
    assert(isa<PHINode>(&*IV) && "Expected PhI node.");
    PHINode *P = dyn_cast<PHINode>(&*IV);
    if (!P)
      continue;
```
to

```
    PHINode *P = cast<PHINode>(&*IV);
```


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

https://reviews.llvm.org/D111810