[PATCH] D144848: [SCEV] Skip instrs with non-scevable types in forget[Loop,Value].

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:8467
+    if (!isSCEVable(I->getType()))
+      continue;
+    assert(ValueExprMap.count(I) == 0 &&
----------------
fhahn wrote:
> nikic wrote:
> > fhahn wrote:
> > > mkazantsev wrote:
> > > > mkazantsev wrote:
> > > > > nikic wrote:
> > > > > > Hm, isn't the verification supposed to omit this check?
> > > > > I guess we can skip it. We can't have them in `ValueExprMap` anyways.
> > > > Maybe even assert on that.
> > > The issue is that non-SCEVable instructions can be operands for SCEVable instructions (e.g. a floating point instruction used by an fcmp). I think in all those cases, the SCEV-able instruction needs to be a SCEVUnknown and the SCEV def-use chain is broken and we should be able to stop invalidation for this instruction.
> > > 
> > > I updated the condition to more explicitly check for this.
> > I think the general change here is right, but I'm not sure this verification is actually verifying anything useful. We aren't going to invalidate the same set of values (by design), so there doesn't really seem anything to verify.
> > 
> > My understanding of this change is that we only want to visit users for which the value may contribute towards the user's SCEV expression. Non-scevable types are one case that can never contribute. The other are instructions that always produce SCEVUnknown, e.g. it doesn't really make sense to follow the use chain across a `load` regardless of which types it works on (this is probably more common than non-scevable types).
> > 
> > The caveat here is that while they cannot contribute towards SCEVs, it's still theoretically possible to contribute towards cached analysis results, e.g. a SCEVUnknown could be caching no longer valid ValueTracking results.
> The current verification here is not super useful for the change here itself, the main use case where it would become more useful should be D144849. I can also just remove it from the current patch.
In that case I'd drop it from this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144848