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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added a comment.

Whenever a new cache is introduced, it is highly recommended to add logic to ScalarEvoltion::verify for it to make sure it is sane. Can you please add that?



================
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.
----------------
`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>
```
?


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6332
+      for (unsigned I = 1, E = N->getNumOperands(); I < E && Res != 1; ++I)
+        Res = APIntOps::GreatestCommonDivisor(
+            Res, getConstantMultiple(N->getOperand(I)));
----------------
This might be overly conservative for `mul`. You can just take constant multiple of any operand, or even their product from all operands. I'm OK if it's not in this patch, but maybe you should consider this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141823