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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
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:
> > 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.
> Not sure I follow. If a context is provided the cache is neither queried nor updated, so the call `GetMinTrailingZeros(X, "f(x)")` shouldn't affect a later `GetMinTrailingZeros(X, "x = y")` call (or a later `GetMinTrailingZeros(X)` call). Am I missing something?
Err, nope.


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

https://reviews.llvm.org/D94088