[llvm] r219462 - SimplifyCFG: Don't convert phis into selects if we could remove undef behavior

[Philip Reames]

On 10/10/2014 01:35 PM, Arnold Schwaighofer wrote:
>
>> On Oct 10, 2014, at 12:14 PM, Philip Reames 
>> <listmail at philipreames.com <mailto:listmail at philipreames.com>> wrote:
>>
>>
>> On 10/10/2014 12:05 PM, Arnold Schwaighofer wrote:
>>> Right, however the code is already there and also handles the more 
>>> general:
>>>
>>>   define void @test6(i1 %cond, i8* %ptr) {
>>>   entry:
>>>     br i1 %cond, label %bb1, label %bb2
>>>
>>>   bb1:
>>>     call void foobar()
>>>     br label %bb2
>>>
>>>   bb2:
>>>     %ptr.2 = phi i8* [ %ptr, %entry ], [ null, %bb1 ]
>>>     store i8 2, i8* %ptr.2, align 8
>>>     ret void
>>>   }
>>>
>>> =>
>>>
>>>   define void @test6(i1 %cond, i8* %ptr) {
>>>   entry:
>>>     br i1 %cond, label %bb1, label %bb2
>>>
>>>   bb1:
>>>     call void foobar()
>>>     unreachable
>>>
>>>   bb2:
>>>     store i8 2, i8* %ptr, align 8
>>>     ret void
>>>   }
>>>
>>> There is an argument to be made to do both I guess ...
>> This would be my preference.  I dislike the coupling between 
>> optimizations created by your patch.  On first look, it seems like 
>> your patch would be unnecessary if we handled the select case.  Is 
>> this correct, or are their other cases you're aware of?
>
> I think, the more general thing is to remove the control flow (not to 
> create the select that we later remove - leaving instructions that we 
> would have otherwise removed). This handles both:
>
> define void @test6(i1 %cond, i8* %ptr) {
> entry:
>   br i1 %cond, label %bb2, label %bb1
>
> bb1:
>   br label %bb2
>
> bb2:
>   %ptr.2 = phi i8* [ %ptr, %entry ], [ null, %bb1 ]
>   store i8 2, i8* %ptr.2, align 8
>   ret void
> }
>
> and this
>
> define void @test6(i1 %cond, i8* %ptr) {
> entry:
>   br i1 %cond, label %bb2, label %bb1
>
> bb1:
>   call @foobar()
>   br label %bb2
>
> bb2:
>   %ptr.2 = phi i8* [ %ptr, %entry ], [ null, %bb1 ]
>   store i8 2, i8* %ptr.2, align 8
>   ret void
> }
I see your point here, but we should be able to trigger the select 
optimization without loosing information.  If we do, that's concerning.

p.s. I want to be clear here.  I'm talking about ideals, not 
practicality.  Your change is fine as is, I'm just debating what the 
'best' approach would be.
>
> Also, we would have to teach whatever optimization to fix up all 
> select users of a undef select condition decision (not hard but still 
> redoing work we are already doing in simplifycfg), instcombine is 
> typically not doing this sort of optimization.
I was viewing this a bit differently.  I was picturing a pattern which 
started from the store, and looked to see if it's pointer input was a 
select with a null input.  If so, it would replace the select with the 
other input.  Potentially, it could also exploit the implied condition 
(in a path sensitive manner)
>
>
> define void @testcase(i1 %cond, i8* %ptr, i32 * %ptrv, i32 %v) {
> entry:
>   br i1 %cond, label %bb1, label %bb2
>
> bb1:
>  %v2 = add i32 %v, 10
>   br label %bb2
>
>
> bb2:
>   %ptr.2 = phi i8* [ %ptr, %entry], [ null, %bb1 ]
>   %v.3 = phi i32 [ %v, %entry ],   [ %v2, %bb1 ]
>   store i8 2, i8* %ptr.2
>   store i32 %v.3 , i32* %ptrv
>   ret void
>
> }
>
> opt -simplifycfg (before my patch):
>
> define void @testcase(i1 %cond, i8* %ptr, i32* %ptrv, i32 %v) {
> entry:
>   %v2 = add i32 %v, 10
>   %.ptr = select i1 %cond, i8* null, i8* %ptr
>   %v2.v = select i1 %cond, i32 %v2, i32 %v
>   store i8 2, i8* %.ptr
>   store i32 %v2.v, i32* %ptrv
>   ret void
> }
>
> SimplifyCFG should not be creating the select to begin with in my 
> opinion. I don’t see a coupling here. I see it the other way round if 
> you generated the select in simplifycfg, someone (instcombine) has to 
> clean it up.
What if a frontend creates the select?  We should handle that case 
regardless of what the optimizer creates.  Given we have to handle it, I 
don't see the value in avoiding creating it elsewhere in the optimizer.
>
> To me, the argument for also handling selects is that some else might 
> generate a select %cond, null ...
I think we're in agreement here.
>
>
>
>

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