[PATCH] D94088: [SCEV] Assumption context for GetMinTrailingZeros

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri requested changes to this revision.
lebedev.ri added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:5611-5615
+  if (!CxtI) {
+    auto I = MinTrailingZerosCache.find(S);
+    if (I != MinTrailingZerosCache.end())
+      return I->second;
+  }
----------------
gilr wrote:
> lebedev.ri wrote:
> > Doesn't this defy the point of the cache?
> > I think `MinTrailingZerosCache`'s key should be changed to be a `std::pair<SCEV*,CxtI*>`
> > Doesn't this defy the point of the cache?
> 
> This API seems to be used extensively. Since (for now) only the unroller and vectorizer would be providing a non-null context I wasn't sure caching those calls was worth the added space/time of extending the key.
> 
> > I think MinTrailingZerosCache's key should be changed to be a std::pair<SCEV*,CxtI*>
> 
> Will do.
Thinking about this, it's not even so much that it's pessimising the cache, it's actually straight-up upsafe to do.
E.g. given
```
x = y
if(!(x & 0b1))
  f(x); // x has at least one trailing bit
```
if we first ask `GetMinTrailingZeros(X, "f(x)")`, we'll get `1`,
and if we then ask `GetMinTrailingZeros(X, "x = y")`, we'll again get `1`, which is obviously not correct.


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

https://reviews.llvm.org/D94088