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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

Logic looks about right to me.

When I tested the original patch, there was a significant impact on compile-time: http://llvm-compile-time-tracker.com/compare.php?from=68a534e9bf69e7e5f081a515e05f1d3cb4c21761&to=8f3c56e720e64e569f930190b246e4af61be2323&stat=instructions:u But I'm not sure if it's avoidable :(



================
Comment at: llvm/include/llvm/Analysis/ScalarEvolution.h:961
+  ///
+  /// If S is a multiple of 0, than any integer is a multiple of S. We return 0
+  /// in this case.
----------------
than -> then


================
Comment at: llvm/include/llvm/Analysis/ScalarEvolution.h:965
+
+  APInt getMaxNonZeroConstantMultiple(const SCEV *S);
+
----------------
Missing doc comment.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6307
+  uint64_t BitWidth = getTypeSizeInBits(S->getType());
+  auto GetPowerOfTwo = [&BitWidth](uint32_t TrailingZeros) {
+    return TrailingZeros >= BitWidth
----------------
Unnecessary reference


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6345-6347
+      APInt Res = APInt(BitWidth, 1);
+      for (unsigned I = 0, E = M->getNumOperands(); I != E; ++I)
+        Res = Res * getMaxConstantMultiple(M->getOperand(I));
----------------



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6354
+    uint32_t TZ = 0;
+    for (const SCEV *Operand : M->operands()) {
+      TZ += getMinTrailingZeros(Operand);
----------------
Unnecessary braces


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6371
+      APInt Res = getMaxConstantMultiple(N->getOperand(0));
+      for (const SCEV *Operand : N->operands())
+        Res = APIntOps::GreatestCommonDivisor(Res,
----------------



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6380
+    uint32_t TZ = getMinTrailingZeros(N->getOperand(0));
+    for (int I = 0, E = N->getNumOperands(); I != E && TZ; ++I)
+      TZ = std::min(TZ, getMinTrailingZeros(N->getOperand(I)));
----------------
Or use the same `operands().drop_front()` style as above.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:8269
+    return *getMaxNonZeroConstantMultiple(applyLoopGuards(TCExpr, L))
+                .getRawData();
+  }
----------------
So, is getRawData() here supposed to be an implicit truncate? Let's not do that...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141823