[llvm] 84b285d - [GVN] Set phi entries of unreachable predecessors to poison instead of undef
Philip Reames via llvm-commits
llvm-commits at lists.llvm.org
Sun Jan 2 09:52:02 PST 2022
On 1/2/22 9:37 AM, Nuno Lopes wrote:
> This commit does 2 changes:
> 1) Replaces 'store undef, null' with 'store poison, null'. This is
> purely cosmetic as both instructions are UB.
>
> 2) Replaces phi([undef, unreachable-pred]) with phi([poison,
> unreachable-pred])
> This is also correct as the phi will never yield the value of an
> unreachable predecessor. So you can change the value to whatever you
> want.
>
> This change is related with another change I want to do, which is to
> fix the instsimplify transformation phi(undef, x) -> x, which is
> wrong. But to fix that (without perf regressions), we need to move all
> creators of phi(undef) -> phi(poison).
Thanks for this, the context helps a lot, and addresses all of my
concern with the original commit.
>
> The remaining correctness issues of NewGVN are being addressed ATM by
> me & Alina. I think we have a good understanding of what's left.
I suspect you do. You just need to explain that to others as you go.
Review :)
>
> Nuno
>
>
> -----Original Message-----
> From: Philip Reames
> Sent: Sunday, January 2, 2022 5:14 PM
> Subject: Re: [llvm] 84b285d - [GVN] Set phi entries of unreachable
> predecessors to poison instead of undef
>
> Nuno,
>
> No issue with the patch per se, but process wise, two points. 1) This
> probably warranted review. 2) The explanation here is highly lacking.
> In particular, NewGVN is not yet enabled due to correctness issues, so
> matching it's behavior is not an obvious justification. Can you respond
> to this with a more complete explanation of a) why this is correct, and
> b) why this is safe to do now? On (b) I'm suspecting there's some
> interaction with other poison related changes, and I'd really like to
> see the context explained.
>
> Philip
>
> On 12/30/21 6:47 AM, Nuno Lopes via llvm-commits wrote:
>> Author: Nuno Lopes
>> Date: 2021-12-30T14:47:24Z
>> New Revision: 84b285d6eb9d52f467fa710f2c9f490a0584c0b2
>>
>> URL:
>> https://github.com/llvm/llvm-project/commit/84b285d6eb9d52f467fa710f2c9f490a0584c0b2
>> DIFF:
>> https://github.com/llvm/llvm-project/commit/84b285d6eb9d52f467fa710f2c9f490a0584c0b2.diff
>>
>> LOG: [GVN] Set phi entries of unreachable predecessors to poison
>> instead of undef
>> This matches NewGVN's behavior.
>>
>> Added:
>>
>> Modified:
>> llvm/lib/Transforms/Scalar/GVN.cpp
>> llvm/test/Transforms/GVN/assume-equal.ll
>> llvm/test/Transforms/GVN/calls-nonlocal.ll
>> llvm/test/Transforms/GVN/condprop.ll
>> llvm/test/Transforms/GVN/equality-assume.ll
>> llvm/test/Transforms/GVN/preserve-memoryssa.ll
>>
>> Removed:
>>
>>
>> ################################################################################
>>
>> diff --git a/llvm/lib/Transforms/Scalar/GVN.cpp
>> b/llvm/lib/Transforms/Scalar/GVN.cpp
>> index 00506fb860069..ee7a2e4aed253 100644
>> --- a/llvm/lib/Transforms/Scalar/GVN.cpp
>> +++ b/llvm/lib/Transforms/Scalar/GVN.cpp
>> @@ -1769,7 +1769,7 @@ bool GVNPass::processAssumeIntrinsic(AssumeInst
>> *IntrinsicI) {
>> // Insert a new store to null instruction before the load to
>> indicate that
>> // this code is not reachable. FIXME: We could insert
>> unreachable
>> // instruction directly because we can modify the CFG.
>> - auto *NewS = new StoreInst(UndefValue::get(Int8Ty),
>> + auto *NewS = new StoreInst(PoisonValue::get(Int8Ty),
>>
>> Constant::getNullValue(Int8Ty->getPointerTo()),
>> IntrinsicI);
>> if (MSSAU) {
>> @@ -2991,12 +2991,12 @@ void GVNPass::addDeadBlock(BasicBlock *BB) {
>> }
>> }
>> - // Now undef the incoming values from the dead predecessors.
>> + // Now poison the incoming values from the dead predecessors.
>> for (BasicBlock *P : predecessors(B)) {
>> if (!DeadBlocks.count(P))
>> continue;
>> for (PHINode &Phi : B->phis()) {
>> - Phi.setIncomingValueForBlock(P,
>> UndefValue::get(Phi.getType()));
>> + Phi.setIncomingValueForBlock(P,
>> PoisonValue::get(Phi.getType()));
>> if (MD)
>> MD->invalidateCachedPointerInfo(&Phi);
>> }
>>
>> diff --git a/llvm/test/Transforms/GVN/assume-equal.ll
>> b/llvm/test/Transforms/GVN/assume-equal.ll
>> index 941f14ce402cd..b51fded5bd20f 100644
>> --- a/llvm/test/Transforms/GVN/assume-equal.ll
>> +++ b/llvm/test/Transforms/GVN/assume-equal.ll
>> @@ -217,7 +217,7 @@ entry:
>> bb2:
>> ; CHECK-NOT: %cmp3 =
>> %cmp3 = icmp eq i32 %p, 43
>> - ; CHECK: store i8 undef, i8* null
>> + ; CHECK: store i8 poison, i8* null
>> call void @llvm.assume(i1 %cmp3)
>> ret i32 15
>> bb3:
>>
>> diff --git a/llvm/test/Transforms/GVN/calls-nonlocal.ll
>> b/llvm/test/Transforms/GVN/calls-nonlocal.ll
>> index 81057dd6cc99e..952f6748ef6f0 100644
>> --- a/llvm/test/Transforms/GVN/calls-nonlocal.ll
>> +++ b/llvm/test/Transforms/GVN/calls-nonlocal.ll
>> @@ -68,7 +68,7 @@ return: ; preds = %bb27
>> ; CHECK: bb26:
>> ; CHECK: br label %bb27
>> ; CHECK: bb27:
>> -; CHECK: %tmp.0 = phi i32 [ 11, %bb26 ], [ undef, %bb24 ], [
>> undef, %bb14 ], [ %g, %bb ]
>> +; CHECK: %tmp.0 = phi i32 [ 11, %bb26 ], [ poison, %bb24 ], [
>> poison, %bb14 ], [ %g, %bb ]
>> ; CHECK: ret i32 %tmp.0
>> ; CHECK: }
>>
>> diff --git a/llvm/test/Transforms/GVN/condprop.ll
>> b/llvm/test/Transforms/GVN/condprop.ll
>> index 04ecb3c5592c4..8a730aef847dc 100644
>> --- a/llvm/test/Transforms/GVN/condprop.ll
>> +++ b/llvm/test/Transforms/GVN/condprop.ll
>> @@ -31,7 +31,7 @@ define i32 @test1() nounwind {
>> ; CHECK: bb7:
>> ; CHECK-NEXT: br label [[BB8]]
>> ; CHECK: bb8:
>> -; CHECK-NEXT: [[DOT0:%.*]] = phi i32 [ [[TMP0]], [[BB7]] ], [
>> undef, [[BB6]] ], [ undef, [[BB4]] ], [ 4, [[BB2]] ], [ 5, [[BB]] ]
>> +; CHECK-NEXT: [[DOT0:%.*]] = phi i32 [ [[TMP0]], [[BB7]] ], [
>> poison, [[BB6]] ], [ poison, [[BB4]] ], [ 4, [[BB2]] ], [ 5, [[BB]] ]
>> ; CHECK-NEXT: ret i32 [[DOT0]]
>> ;
>> entry:
>>
>> diff --git a/llvm/test/Transforms/GVN/equality-assume.ll
>> b/llvm/test/Transforms/GVN/equality-assume.ll
>> index ee2cb06c158df..ee55d5d463f45 100644
>> --- a/llvm/test/Transforms/GVN/equality-assume.ll
>> +++ b/llvm/test/Transforms/GVN/equality-assume.ll
>> @@ -149,7 +149,7 @@ merge:
>> define i32 @conflicting_constants(i32* %p) {
>> ; CHECK-LABEL: @conflicting_constants(
>> -; CHECK-NEXT: store i8 undef, i8* null
>> +; CHECK-NEXT: store i8 poison, i8* null
>> ; CHECK-NEXT: br i1 undef, label [[TAKEN:%.*]], label [[MERGE:%.*]]
>> ; CHECK: taken:
>> ; CHECK-NEXT: br label [[MERGE]]
>>
>> diff --git a/llvm/test/Transforms/GVN/preserve-memoryssa.ll
>> b/llvm/test/Transforms/GVN/preserve-memoryssa.ll
>> index b78aba2238e4b..282dbc0a8b28a 100644
>> --- a/llvm/test/Transforms/GVN/preserve-memoryssa.ll
>> +++ b/llvm/test/Transforms/GVN/preserve-memoryssa.ll
>> @@ -97,7 +97,7 @@ for.body.i22:
>> define void @test_assume_false_to_store_undef_1(i32* %ptr) {
>> ; CHECK-LABEL: @test_assume_false_to_store_undef_1(
>> ; CHECK-NEXT: store i32 10, i32* [[PTR:%.*]], align 4
>> -; CHECK-NEXT: store i8 undef, i8* null, align 1
>> +; CHECK-NEXT: store i8 poison, i8* null, align 1
>> ; CHECK-NEXT: call void @f()
>> ; CHECK-NEXT: ret void
>> ;
>> @@ -113,7 +113,7 @@ define i32
>> @test_assume_false_to_store_undef_2(i32* %ptr, i32* %ptr.2) {
>> ; CHECK-LABEL: @test_assume_false_to_store_undef_2(
>> ; CHECK-NEXT: store i32 10, i32* [[PTR:%.*]], align 4
>> ; CHECK-NEXT: [[LV:%.*]] = load i32, i32* [[PTR_2:%.*]], align 4
>> -; CHECK-NEXT: store i8 undef, i8* null, align 1
>> +; CHECK-NEXT: store i8 poison, i8* null, align 1
>> ; CHECK-NEXT: call void @f()
>> ; CHECK-NEXT: ret i32 [[LV]]
>> ;
>> @@ -130,7 +130,7 @@ define i32
>> @test_assume_false_to_store_undef_3(i32* %ptr, i32* %ptr.2) {
>> ; CHECK-LABEL: @test_assume_false_to_store_undef_3(
>> ; CHECK-NEXT: store i32 10, i32* [[PTR:%.*]], align 4
>> ; CHECK-NEXT: [[LV:%.*]] = load i32, i32* [[PTR_2:%.*]], align 4
>> -; CHECK-NEXT: store i8 undef, i8* null, align 1
>> +; CHECK-NEXT: store i8 poison, i8* null, align 1
>> ; CHECK-NEXT: ret i32 [[LV]]
>> ;
>> store i32 10, i32* %ptr
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
More information about the llvm-commits
mailing list