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

[Philip Reames]

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?

Philip
>
>> On Oct 10, 2014, at 11:53 AM, Philip Reames <listmail at philipreames.com> wrote:
>>
>> Why is this pattern any worse for recognizing undefined behaviour?
>>
>>   define void @test6(i1 %cond, i8* %ptr) {
>>     %ptr.2 = select i1 %cond, i8* null, i8* %ptr
>>     store i8 2, i8* %ptr.2, align 8
>>     ret void
>>   }
>>
>> We can infer that %cond can't be true, or undefined behaviour would result.  This might not be implemented currently, but it certainly seems easy to do.
>>
>> That transformation would seem to get us the same result:
>>
>>   define void @test6(i1 %cond, i8* %ptr) {
>>     store i8 2, i8* %ptr.2, align 8
>>     ret void
>>   }
>>
>> Philip
>>
>> On 10/09/2014 06:27 PM, Arnold Schwaighofer wrote:
>>> Author: arnolds
>>> Date: Thu Oct  9 20:27:02 2014
>>> New Revision: 219462
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=219462&view=rev
>>> Log:
>>> SimplifyCFG: Don't convert phis into selects if we could remove undef behavior
>>> instead
>>>
>>> We used to transform this:
>>>
>>>    define void @test6(i1 %cond, i8* %ptr) {
>>>    entry:
>>>      br i1 %cond, label %bb1, label %bb2
>>>
>>>    bb1:
>>>      br label %bb2
>>>
>>>    bb2:
>>>      %ptr.2 = phi i8* [ %ptr, %entry ], [ null, %bb1 ]
>>>      store i8 2, i8* %ptr.2, align 8
>>>      ret void
>>>    }
>>>
>>> into this:
>>>
>>>    define void @test6(i1 %cond, i8* %ptr) {
>>>      %ptr.2 = select i1 %cond, i8* null, i8* %ptr
>>>      store i8 2, i8* %ptr.2, align 8
>>>      ret void
>>>    }
>>>
>>> because the simplifycfg transformation into selects would happen to happen
>>> before the simplifycfg transformation that removes unreachable control flow
>>> (We have 'unreachable control flow' due to the store to null which is undefined
>>> behavior).
>>>
>>> The existing transformation that removes unreachable control flow in simplifycfg
>>> is:
>>>
>>>    /// If BB has an incoming value that will always trigger undefined behavior
>>>    /// (eg. null pointer dereference), remove the branch leading here.
>>>    static bool removeUndefIntroducingPredecessor(BasicBlock *BB)
>>>
>>> Now we generate:
>>>
>>>    define void @test6(i1 %cond, i8* %ptr) {
>>>      store i8 2, i8* %ptr.2, align 8
>>>      ret void
>>>    }
>>>
>>> I did not see any impact on the test-suite + externals.
>>>
>>> rdar://18596215
>>>
>>> Modified:
>>>      llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp
>>>      llvm/trunk/test/Transforms/SimplifyCFG/UnreachableEliminate.ll
>>>
>>> Modified: llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp?rev=219462&r1=219461&r2=219462&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp (original)
>>> +++ llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp Thu Oct  9 20:27:02 2014
>>> @@ -1010,6 +1010,8 @@ static bool isSafeToHoistInvoke(BasicBlo
>>>     return true;
>>>   }
>>>   +static bool passingValueIsAlwaysUndefined(Value *V, Instruction *I);
>>> +
>>>   /// HoistThenElseCodeToIf - Given a conditional branch that goes to BB1 and
>>>   /// BB2, hoist any common code in the two blocks up into the branch block.  The
>>>   /// caller of this function guarantees that BI's block dominates BB1 and BB2.
>>> @@ -1094,6 +1096,12 @@ HoistTerminator:
>>>         if (BB1V == BB2V)
>>>           continue;
>>>   +      // Check for passingValueIsAlwaysUndefined here because we would rather
>>> +      // eliminate undefined control flow then converting it to a select.
>>> +      if (passingValueIsAlwaysUndefined(BB1V, PN) ||
>>> +          passingValueIsAlwaysUndefined(BB2V, PN))
>>> +       return Changed;
>>> +
>>>         if (isa<ConstantExpr>(BB1V) && !isSafeToSpeculativelyExecute(BB1V, DL))
>>>           return Changed;
>>>         if (isa<ConstantExpr>(BB2V) && !isSafeToSpeculativelyExecute(BB2V, DL))
>>> @@ -1508,6 +1516,11 @@ static bool SpeculativelyExecuteBB(Branc
>>>       if (ThenV == OrigV)
>>>         continue;
>>>   +    // Don't convert to selects if we could remove undefined behavior instead.
>>> +    if (passingValueIsAlwaysUndefined(OrigV, PN) ||
>>> +        passingValueIsAlwaysUndefined(ThenV, PN))
>>> +      return false;
>>> +
>>>       HaveRewritablePHIs = true;
>>>       ConstantExpr *OrigCE = dyn_cast<ConstantExpr>(OrigV);
>>>       ConstantExpr *ThenCE = dyn_cast<ConstantExpr>(ThenV);
>>>
>>> Modified: llvm/trunk/test/Transforms/SimplifyCFG/UnreachableEliminate.ll
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/SimplifyCFG/UnreachableEliminate.ll?rev=219462&r1=219461&r2=219462&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/test/Transforms/SimplifyCFG/UnreachableEliminate.ll (original)
>>> +++ llvm/trunk/test/Transforms/SimplifyCFG/UnreachableEliminate.ll Thu Oct  9 20:27:02 2014
>>> @@ -71,3 +71,50 @@ U:
>>>   T:
>>>           ret i32 2
>>>   }
>>> +
>>> +
>>> +;; We can either convert the following control-flow to a select or remove the
>>> +;; unreachable control flow because of the undef store of null. Make sure we do
>>> +;; the latter.
>>> +
>>> +define void @test5(i1 %cond, i8* %ptr) {
>>> +
>>> +; CHECK-LABEL: test5
>>> +; CHECK: entry:
>>> +; CHECK-NOT: select
>>> +; CHECK:  store i8 2, i8* %ptr
>>> +; CHECK:  ret
>>> +
>>> +entry:
>>> +  br i1 %cond, label %bb1, label %bb3
>>> +
>>> +bb3:
>>> + br label %bb2
>>> +
>>> +bb1:
>>> + br label %bb2
>>> +
>>> +bb2:
>>> +  %ptr.2 = phi i8* [ %ptr, %bb3 ], [ null, %bb1 ]
>>> +  store i8 2, i8* %ptr.2, align 8
>>> +  ret void
>>> +}
>>> +
>>> +; CHECK-LABEL: test6
>>> +; CHECK: entry:
>>> +; CHECK-NOT: select
>>> +; CHECK:  store i8 2, i8* %ptr
>>> +; CHECK:  ret
>>> +
>>> +define void @test6(i1 %cond, i8* %ptr) {
>>> +entry:
>>> +  br i1 %cond, label %bb1, label %bb2
>>> +
>>> +bb1:
>>> +  br label %bb2
>>> +
>>> +bb2:
>>> +  %ptr.2 = phi i8* [ %ptr, %entry ], [ null, %bb1 ]
>>> +  store i8 2, i8* %ptr.2, align 8
>>> +  ret void
>>> +}
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits