[PATCH] D42290: [SCEV] Clear poison flags during expansion of SCEV

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy added inline comments.


================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1694
+/// but this is better than nothing.
+static bool SCEVLostPoisonFlags(const SCEV *S, const Value *V) {
+  if (auto *I = dyn_cast<Instruction>(V)) {
----------------
skatkov wrote:
> sanjoy wrote:
> > This is bothering me -- I agree that this mitigation is better than nothing, but what you've found shows that this `FindValueInExprValueMap` business is basically incorrect, except in perhaps very limited cases.
> > 
> > I'm tempted to rip all of this reuse logic out of SCEV/SCEVExpander.  Back when this was added in https://reviews.llvm.org/D12090 we talked about improving our CSE as an alternative to caching expressions like this; I think this bug is a sign that we should, in fact, seriously consider what `FindValueInExprValueMap` does as a CSE optimization.
> > 
> > +CC @wmi @hfinkel for discussion.
> Just a side note. This functionality is used in backend as well (in loop reduce pass). If I understand correctly your proposal is to eliminate caching and just always literally expand the SCEV and CSE later will eliminate all redundant instructions. But in this case we will need to add a CSE pass after loop reduce as well.
> 
> Ok, I keep looking into your discussion.
> But in this case we will need to add a CSE pass after loop reduce as well.

Or we can instead do some basic CSE during SCEV expansion (we already do a bit of this).

In any case, correctness trumps everything. :)


https://reviews.llvm.org/D42290