[PATCH] D17184: [LVI] Extend select handling to catch min/max/clamp idioms

[Philip Reames via llvm-commits]

reames added inline comments.

================
Comment at: lib/Analysis/LazyValueInfo.cpp:972
@@ +971,3 @@
+    if (ConstantInt *CIBase = dyn_cast<ConstantInt>(ICI->getOperand(1))) {
+      auto addConstants = [](ConstantInt *A, ConstantInt *B) {
+        assert(A->getType() == B->getType());
----------------
sanjoy wrote:
> Nit: should be `AddConstants`.
Per style guide:
Function names should be verb phrases (as they represent actions), and command-like function should be imperative. The name should be camel case, and start with a lower case letter (e.g. openFile() or isFoo()).

While a lambda is technically a variable, it's really just a locally scoped function in this usage.  

================
Comment at: lib/Analysis/LazyValueInfo.cpp:979
@@ +978,3 @@
+      case ICmpInst::ICMP_EQ:
+        if (match(SI->getFalseValue(), m_Add(m_Specific(A),
+                                             m_ConstantInt(CIAdded)))) {
----------------
sanjoy wrote:
> This would be easier to follow if you summarize the transform you're doing / pattern you're matching, like
> 
> `select (A == CIBase) _ (A + CIAdded)`
See updated patch.  Is that better?

================
Comment at: lib/Analysis/LazyValueInfo.cpp:982
@@ +981,3 @@
+          auto ResNot = addConstants(CIBase, CIAdded);
+          FalseVal = intersect(FalseVal,
+                               LVILatticeVal::getNot(ResNot));
----------------
sanjoy wrote:
> How can you infer this without looking at the `SI->getTrueValue()`?  Can't the select be `select (A == CIBase) (CIBase + CIAdded) (A + CIAdded)`?  Or is there a check somewhere that I missed?
If A != CIBase, then the RHS can't contribute CIBase + CIAdded.  The LHS can, but we're only constraining the input ranges here.   


http://reviews.llvm.org/D17184