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

[Gil Rapaport via Phabricator via llvm-commits]

gilr added a comment.

In D94088#2519311 <https://reviews.llvm.org/D94088#2519311>, @lebedev.ri wrote:

> I would expect that the SCEV change could be testable with just the scev itselfs (`-analyze -scalar-evolution`), is it not?

Right, will add a unit test for this API.

In D94088#2519327 <https://reviews.llvm.org/D94088#2519327>, @fhahn wrote:

> I think the fix makes sense in isolation, but I am wondering if we could generalize this to automatically apply in more cases. Would it be possible to 'apply' the information from the loop guard directly to the SCEV expression? Like we do for some conditions already: https://github.com/llvm/llvm-project/blob/master/llvm/lib/Analysis/ScalarEvolution.cpp#L13241 ?

Will take a look at that. Thanks!



================
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:
> 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.


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

https://reviews.llvm.org/D94088