[PATCH] D72382: [ArgPromotion] Extend search for SafeToUnconditionallyLoad indices to the blocks that must be executed upon entry into the function.

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

In D72382#1830078 <https://reviews.llvm.org/D72382#1830078>, @mgudim wrote:

> In D72382#1824854 <https://reviews.llvm.org/D72382#1824854>, @jdoerfert wrote:
>
> > This should fix PR887: https://bugs.llvm.org/show_bug.cgi?id=887
> >  Please verify that and include the test case from there. We should mention it in the commit message and close the bug if it works.
>
>
> My patch does not make any difference. It looks like 887 is related to pass ordering: the load can be hoisted up into the entry block, but hoisting only runs after argpromotion


Right. Sorry. We are still missing DD65593. Anyway, all this logic should not be here in the first place so never mind ;)

>>  ---
>> 
>> This should also fix PR42039: https://bugs.llvm.org/show_bug.cgi?id=42039
>>  Please verify that and include the test case from there. We should mention it in the commit message and close the bug if it works.
> 
>   With my patch argpromotion does not happen, so I'll add this to the test cases and mention it in commit message.

Yes, thanks.

>> In D72382#1824701 <https://reviews.llvm.org/D72382#1824701>, @mgudim wrote:
>> 
>>> @jdoerfert I updated the patch as you suggested. Also, I added your example as a test.
>> 
>> 
>> I don't see the example.
> 
> It's at the very end of control-flow3.ll:
>  ; CHECK-LABEL: define internal i32 @callee2(i32* %P) {
>  define internal i32 @callee2(i32* %P) {
> entry:
> 
>   br label %bb1
>    
> 
> bb1:
> 
>   %gep0 = getelementptr i32, i32* %P, i64 0
>   ; CHECK: %X = load i32, i32* %gep0
>   %X = load i32, i32* %gep0
>   br label %bb1
>    
> 
> bb2:
> 
>   %gep1 = getelementptr i32, i32* %P, i64 1
>   ; CHECK: %Y = load i32, i32* %gep1
>   %Y = load i32, i32* %gep1
>   ret i32 %X
> 
> }
> 
> define i32 @caller2() {
> 
>   %A = alloca i32
>   store i32 17, i32* %A
>   ; CHECK: %X = call i32 @callee2(i32* %A)
>   %X = call i32 @callee2(i32* %A)
>   ret i32 %X
> 
> }

Isn't that an endless loop without side-effects? We should avoid test like these. If you run it through the attributor (https://godbolt.org/z/xTPcmT) you see why this is not too meaningful.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72382/new/

https://reviews.llvm.org/D72382