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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added a comment.

I think this needs stronger test coverage. At least I want tests for all operations (either IR tests or unittests in CPP, whatever is easier) exercising corner case scenarios, such as bit width overflow with `mul`.



================
Comment at: llvm/include/llvm/Analysis/ScalarEvolution.h:971
   /// If S is guaranteed to be 0, it returns the bitwidth of S.
-  uint32_t GetMinTrailingZeros(const SCEV *S);
+  uint32_t getMinTrailingZeros(const SCEV *S);
 
----------------
Separate NFC?


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6309
+    return TrailingZeros >= BitWidth
+               ? APInt::getZero(BitWidth)
+               : APInt::getOneBitSet(BitWidth, TrailingZeros);
----------------
Why zero and not `APInt::getOneBitSet(BitWidth, BitWidth - 1)`? Zero is not even a power of 2, how to interpret that?


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6320
+    unsigned TZ =
+        computeKnownBits(U->getValue(), getDataLayout(), 0, &AC, nullptr, &DT)
+            .countMinTrailingZeros();
----------------
`/*param name/* nullptr`


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6373
+        Res = APIntOps::GreatestCommonDivisor(Res,
+                                              getMaxConstantMultiple(Operand));
+      return Res;
----------------
Early bail if GCD has become `1`? It won't get any better anyways.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141823