[PATCH] D141823: [SCEV] More precise trip multiples

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6372
 
-uint32_t ScalarEvolution::getMinTrailingZeros(const SCEV *S) {
-  auto I = MinTrailingZerosCache.find(S);
-  if (I != MinTrailingZerosCache.end())
+APInt ScalarEvolution::getConstantMultiple(const SCEV *S) {
+  auto I = ConstantMultipleCache.find(S);
----------------
This should probably return `const APInt &`?


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:8276
+    APInt Multiple = getNonZeroConstantMultiple(TCExpr);
+    return Multiple.getActiveBits() > 32 ? 1 : *Multiple.getRawData();
+  }
----------------
Should be getZExtValue() instead of getRawData().


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:14341
+  for (auto [S, Multiple] : ConstantMultipleCache) {
+    APInt RecomputedMultiple = SE2.getConstantMultipleImpl(S);
+    if (Multiple != RecomputedMultiple) {
----------------
Hm, can this end up modifying the map we're iterating?


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6328
+    const SCEVNAryExpr *N = cast<SCEVNAryExpr>(S);
+    if (N->hasNoUnsignedWrap()) {
+      // The result is GCD of all operands results.
----------------
mkazantsev wrote:
> `hasNoUnsignedWrap` doesn't make sense for min/max. How about:
> ```
>   case scAddExpr:
>   case scAddRecExpr:
>   case scUMaxExpr:
>     <handling for nuw>
>     // fallthrough
>   case scUMaxExpr:
>   case scSMaxExpr:
>   case scUMinExpr:
>   case scSMinExpr:
>   case scSequentialUMinExpr:
>     <common part>
> ```
> ?
min/max are implicitly nuw/nsw. With the new code structure we will use only the trailing zeros for min/max, while using the GCD would be legal.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141823