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

[Gerolf Hoflehner via llvm-commits]

> On Apr 26, 2016, at 7:20 PM, Gerolf Hoflehner <ghoflehner at apple.com> wrote:
> 
> 
>> On Apr 26, 2016, at 6:33 PM, Philip Reames <listmail at philipreames.com <mailto:listmail at philipreames.com>> wrote:
>> 
>> 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?

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.
> 
> 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? 
> 
> ; don't replace 'cond' by 'len' in a block ('b1') that dominates all uses
> ; of the select outside the home block ('bb'), but can be reached from the home
> ; block on another path ('bb -> b0 -> b1')
> define void @test2(i32 %len) {
> entry:
>   %0 = call i32 @bar(i32 %len);
>   %cmp = icmp ult i32 %len, 4                         
>   br i1 %cmp, label %bb, label %b1
> bb:
     %cmp2 = icmp ult i32 %len, 6
>   %cond = select i1 %cmp2, i32 %len, i32 8          
>   %cmp3 = icmp eq i32 %cond, 8
>   br i1 %cmp3, label %b0, label %b1
> 
> b0:
>   call void @foo(i32 %len)
>   br label %b1
> 
> b1:
> ; CHECK: phi i32 [ %cond, %bb ], [ undef, %b0 ], [ %0, %entry ]  
>   %1 = phi i32 [ %cond, %bb ], [ undef, %b0 ], [ %0, %entry ]
>   br label %ret
> 
> ret:
> 
>> 
>> 
>> http://reviews.llvm.org/D19550 <http://reviews.llvm.org/D19550>
>> 
>> 
>> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160427/1f2a8f6d/attachment.html>