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

[Gil Rapaport via Phabricator via llvm-commits]

gilr 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;
+  }
----------------
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?


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

https://reviews.llvm.org/D94088