[PATCH] D97122: [SCEV] Improve handling of pointer compares involving subtractions (WIP).

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 2 inline comments as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:1840
+                       getZeroExtendExpr(SM->getOperand(1), Ty));
+  }
+
----------------
reames wrote:
> Couple of comments on this transform:
> 1) It's not clear why (or if) this is a profitable canonicalization.  We're replacing one zext with two.  I'd want an explanation of why this was the right idea, and in particular, why not to canonicalize the other direction.
> 2) umax can have more than two operands.
> 3) this should be a separate change once you're ready for actual review.
I will split this off in a separate review.

Initially I was planning on only restricting it to a umax with at least one constant operand, so the number of `zexts` stays the same. We already shift `zexts` inwards for other expressions above, so I think most code would expect them as 'inwards' as possible (but I might be wrong).


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:3117
+  //
+  // (-C1 + X) /u C2 can be transformed to (C1 /u C3) + (X /u C2), if
+  //   * C1 % C2 == 0
----------------
mkazantsev wrote:
> What `C3` stands for? Is it actually `C2`?
It should be `C2`, thanks!


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:3121
+  //   * X s>= C1
+  //   * C1 s>= 0
+  //
----------------
reames wrote:
> Another way to consider approaching this:
> (-C1 + X) /u C2
> == -C1/C2 + (-C1 % C2 + X) /u C2
> 
> (i.e. extract out the whole integer divisions and leave only the possible fractional part)
Thanks, I'll have to think a bit more about that and see if there are any expressions that cannot be handled well by either.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:10979
+  auto *Add = dyn_cast<SCEVAddExpr>(LHS);
+  if (Add && Pred == CmpInst::ICMP_ULE) {
+    auto *X = Add->getOperand(0);
----------------
reames wrote:
> You can also handle ULT without much work.  isKnownNonNegative becomes isKnownPositive in that case.
Done, but I still need to construct a test case.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:10982
+    auto *Y = Add->getOperand(1);
+    if (Y == RHS && isKnownNonPositive(X) && isKnownNonNegative(Y) &&
+        isKnownPredicateViaConstantRanges(CmpInst::ICMP_UGE, Y,
----------------
reames wrote:
> Do we canonicalize x + (-Y) to (-Y) + X somewhere?  If not, you're missing some pattern matching cases here.  
It looks like we don't do that, we only sort the operands by their complexity which should group the `-1 * X` parts together, but there could be other 'less complex' expression before them. I adjusted to also check for the swapped version.

 I still need to construct a test case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97122