[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:14:40 PST 2022
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