<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Apr 26, 2016, at 7:20 PM, Gerolf Hoflehner <<a href="mailto:ghoflehner@apple.com" class="">ghoflehner@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><meta http-equiv="Content-Type" content="text/html charset=us-ascii" class=""><meta http-equiv="Content-Type" content="text/html charset=us-ascii" class=""><meta http-equiv="Content-Type" content="text/html charset=us-ascii" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div class=""><blockquote type="cite" class=""><div class="">On Apr 26, 2016, at 6:33 PM, Philip Reames <<a href="mailto:listmail@philipreames.com" class="">listmail@philipreames.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div class="">reames added a subscriber: reames.<br class="">reames requested changes to this revision.<br class="">reames added a reviewer: reames.<br class="">reames added a comment.<br class="">This revision now requires changes to proceed.<br class=""><br class="">Mostly minor comments, once addressed should be ready to go in.<br class=""><br class=""><br class="">================<br class="">Comment at: lib/Analysis/ValueTracking.cpp:3970<br class="">@@ +3969,3 @@<br class="">+ // Types must match.<br class="">+ if (LHS->getType() != RHS->getType())<br class="">+ return None;<br class="">----------------<br class="">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?<br class=""><br class="">================<br class="">Comment at: test/Transforms/InstCombine/pr21210.ll:34<br class="">@@ -33,3 +33,3 @@<br class=""> bb:<br class=""> %cond = select i1 %cmp, i32 %len, i32 8<br class=""> %cmp11 = icmp eq i32 %cond, 8<br class="">----------------<br class="">Add a check that shows this select disappears.<br class=""><br class="">================<br class="">Comment at: test/Transforms/InstCombine/pr21210.ll:44<br class="">@@ -44,1 +43,3 @@<br class="">+; This seems perfectly fine to me?<br class="">+; CHECK: phi i32 [ %len, %bb ], [ undef, %b0 ], [ %0, %entry ]<br class=""> %1 = phi i32 [ %cond, %bb ], [ undef, %b0 ], [ %0, %entry ]<br class="">----------------<br class="">Gerolf wrote:<br class=""><blockquote type="cite" class="">cond can be different from len, so you cannot replace it.<br class="">bb-> bb1 vs entry -> b1.<br class=""><br class="">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. <br class=""></blockquote>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?<br class=""></div></div></blockquote></div></div></div></blockquote><div><br class=""></div>Ah, yes, you guys are right. In the current test ‘cond’ can be replaced by ‘len’. The test case needs to be sharpened eg. by another cmp2 so that this scenario becomes a bug, which was the intent.<br class=""><blockquote type="cite" class=""><div class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class=""><div class=""><br class=""></div><div class="">I think the crux was the the value of len was used for cond on the path entry to b1. Maybe the test case does not model the original failure scenario accurately. Would your patch work if we replace %0 by %len in b1? </div><div class=""><br class=""></div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo;" class="">; don't replace 'cond' by 'len' in a block ('b1') that dominates all uses</div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo;" class="">; of the select outside the home block ('bb'), but can be reached from the home</div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo;" class="">; block on another path ('bb -> b0 -> b1')</div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo;" class="">define void @test2(i32 %len) <span style="font-variant-ligatures: no-common-ligatures; background-color: #00e6e5" class="">{</span></div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo;" class="">entry:</div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo;" class=""> %0 = call i32 @bar(i32 %len);</div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo;" class=""> %cmp = icmp ult i32 %len, 4 </div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo;" class=""> br i1 %cmp, label %bb, label %b1</div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo;" class="">bb:</div></div></div></div></blockquote> %cmp2 = icmp ult i32 %len, 6<br class=""><blockquote type="cite" class=""><div class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class=""><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo;" class=""> %cond = select i1 %cmp2, i32 %len, i32 8 </div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo;" class=""> %cmp3 = icmp eq i32 %cond, 8</div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo;" class=""> br i1 %cmp3, label %b0, label %b1</div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo; min-height: 13px;" class=""><br class=""></div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo;" class="">b0:</div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo;" class=""> call void @foo(i32 %len)</div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo;" class=""> br label %b1</div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo; min-height: 13px;" class=""><br class=""></div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo;" class="">b1:</div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo;" class="">; CHECK: phi i32 [ %cond, %bb ], [ undef, %b0 ], [ %0, %entry ] </div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo;" class=""> %1 = phi i32 [ %cond, %bb ], [ undef, %b0 ], [ %0, %entry ]</div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo;" class=""> br label %ret</div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo; min-height: 13px;" class=""><br class=""></div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo;" class="">ret:</div><div class=""><br class=""></div><blockquote type="cite" class=""><div class=""><div class=""><br class=""><br class=""><a href="http://reviews.llvm.org/D19550" class="">http://reviews.llvm.org/D19550</a><br class=""><br class=""><br class=""><br class=""></div></div></blockquote></div><br class=""></div></div></blockquote></div><br class=""></body></html>