[PATCH] Teach ScalarEvolution to exploit min and max expressions.

[Sanjoy Das]

================
Comment at: lib/Analysis/ScalarEvolution.cpp:6954
@@ +6953,3 @@
+/// return false.
+static bool MatchNotExpr(const SCEV *Expr, const SCEV *&NotOf) {
+  const SCEVAddExpr *Add = dyn_cast<SCEVAddExpr>(Expr);
----------------
atrick wrote:
> Not a big deal, but why not return NotOf and nullptr for a mismatch?
> 
> Did you implement the pattern matching to avoid the overhead of just comparing the result of getNotExpr if you can bail-out of the match early?
> Not a big deal, but why not return NotOf and nullptr for a mismatch?

Will do, that seems obviously better.

> Did you implement the pattern matching to avoid the overhead of just comparing the result of getNotExpr if you can bail-out of the match early?

Yes.  I have to admit though that I don't have a good intuition on what kinds of operations in ScalarEvolution are fast enough that they're not worth optimizing like this.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:6983
@@ +6982,3 @@
+  for (const SCEV *Op : MaxExpr->operands())
+    if (SE.getMinusSCEV(Candidate, Op)->isZero())
+      return true;
----------------
atrick wrote:
> Can you explain why the subtraction is necessary. What unique expressions have zero difference?
> 
> Are you assuming that MaybeExpr and Candidate have the same bitwidth? What happens if they don't?
> Can you explain why the subtraction is necessary. What unique expressions have zero difference?

I was initially thinking of things like `%a OP %b` vs. `%b OP %a` but depending on how scev canonicalizes these it may not actually be an issue.  I'll change it to use pointer equality for now -- that seems to pass the test with this patch.

> Are you assuming that MaybeExpr and Candidate have the same bitwidth? What happens if they don't?

I don't think that should be an issue once I change this to use pointer equality.  That said, is `isImpliedCondOperands(P, A, B, C, D)` ever called  with either `A Pred B` or `C Pred D` not well-typed?

================
Comment at: lib/Analysis/ScalarEvolution.cpp:7034
@@ +7033,3 @@
+
+  llvm_unreachable("covered switch fell through?!");
+}
----------------
atrick wrote:
> Out of curiosity, do you need the unreachable? There shouldn't be a warning for a covered switch, should be a warning otherwise.
Without the `llvm_unreachable` I don't see a warning with clang, but wasn't sure if one of the buildbots might.

http://reviews.llvm.org/D6635

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/