[PATCH] D19550: [InstCombine] Determine the result of a select based on a dominating condition.

[Philip Reames via llvm-commits]

reames added a subscriber: reames.
reames requested changes to this revision.
reames added a reviewer: reames.
reames added a comment.
This revision now requires changes to proceed.

Mostly minor comments, once addressed should be ready to go in.


================
Comment at: lib/Analysis/ValueTracking.cpp:3970
@@ +3969,3 @@
+  // Types must match.
+  if (LHS->getType() != RHS->getType())
+    return None;
----------------
I'm confused by this bit.  How do we get here?  Do we have a select with a non-i1 condition?  If so, maybe the check should be in the caller and we should assert both are i1s?

================
Comment at: test/Transforms/InstCombine/pr21210.ll:34
@@ -33,3 +33,3 @@
 bb:
   %cond = select i1 %cmp, i32 %len, i32 8
   %cmp11 = icmp eq i32 %cond, 8
----------------
Add a check that shows this select disappears.

================
Comment at: test/Transforms/InstCombine/pr21210.ll:44
@@ -44,1 +43,3 @@
+; This seems perfectly fine to me?
+; CHECK: phi i32 [ %len, %bb ], [ undef, %b0 ], [ %0, %entry ]
   %1 = phi i32 [ %cond, %bb ], [ undef, %b0 ], [ %0, %entry ]
----------------
Gerolf wrote:
> cond can be different from len, so you cannot replace it.
> bb-> bb1 vs entry -> b1.
> 
> I think this test covers self-build issues I had ran into when I missed some cases in the  icmp->select->icmp optimization which removes the select in some narrow cases. 
I believe the optimization as posted is correct.  Geroff, were you trying to say it wasn't?  If so, I didn't understand your point.  Could you explain again?


http://reviews.llvm.org/D19550