[PATCH] D50167: RFC: [SCEV] Add explicit representations of umin/smin

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy accepted this revision.
sanjoy added a comment.
This revision is now accepted and ready to land.

This change in isolation LGTM (modulo some minor comments inline) -- at the very least having separate `umin` / `smin` nodes makes min expressions more readable (as demonstrated by the updates to the test cases).

However, I'm not sure if this is a sufficient solution of your original problem involving non-integral pointers.  Fundamentally to do trip counts right with non-integral pointers, we will at least need to teach SCEV the difference between ni pointers and integers so that it does not create SCEV expressions that cannot be lowered.  I think we will also need a `psub` instruction or intrinsic so that we can compute trip counts for loops like:

  ptr0 = ... ; ni pointer
  ptr1 = ... ; ni pointer at some offset from ptr0
  
  for (i = ptr0; i != ptr1; i++)
    ...

Of course all of these may be "working" in practice today because of various reasons. :)



================
Comment at: lib/Analysis/ScalarEvolution.cpp:3573
     assert(Idx < Ops.size());
+    auto &FoldOp =
+        Kind == scSMaxExpr
----------------
Let's avoid a nested ternary here.  Maybe you could instead use an immediately executed lambda.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:3589
 
-    // If we are left with a constant minimum-int, strip it off.
-    if (cast<SCEVConstant>(Ops[0])->getValue()->isMinValue(true)) {
-      Ops.erase(Ops.begin());
-      --Idx;
-    } else if (cast<SCEVConstant>(Ops[0])->getValue()->isMaxValue(true)) {
-      // If we have an smax with a constant maximum-int, it will always be
-      // maximum-int.
-      return Ops[0];
+    if (IsMax) {
+      // If we are left with a constant minimum-int, strip it off.
----------------
Probably this can just be one loop if you create two `APInts`, `Top` and `Bottom`, or two lambdas `IsTop` and `IsBottom` if you're worried about creating large `APInt`s being expensive.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:10137
+        // min(A, ...) <= A
+        IsMinMaxConsistingOfByNegation<SCEVSMaxExpr>(SE, LHS, RHS) ||
+        IsMinMaxConsistingOf<SCEVSMinExpr>(LHS, RHS) ||
----------------
Does this case even fire anymore?


================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:2165
   // the exit condition.
-  if (isa<SCEVSMaxExpr>(S) || isa<SCEVUMaxExpr>(S))
+  if (isa<SCEVSMaxExpr>(S) || isa<SCEVUMaxExpr>(S) || isa<SCEVSMinExpr>(S) || isa<SCEVUMinExpr>(S))
     return true;
----------------
Did this overflow 80 cols?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D50167