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

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy added subscribers: wmi, hfinkel.
sanjoy added inline comments.


================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1692
+/// Check whether value has nuw/nsw/exact set but SCEV does not.
+/// TODO: In reality it is better to check the poison recursevely
+/// but this is better than nothing.
----------------
s/recursevely/recursively/


================
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)) {
----------------
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.


https://reviews.llvm.org/D42290