[llvm] 85eeba4 - [ConstantHoisting] Add back ptr->ptr bitcast to avoid assertion failure
Björn Pettersson A via llvm-commits
llvm-commits at lists.llvm.org
Mon Aug 14 04:21:05 PDT 2023
Thanks for the heads up!
I actually did notice it, and tried to fixed it here: https://github.com/llvm/llvm-project/commit/e217f8803435f5ae4cae66db97a84a140fab8c8e
(I don't have a windows env, so I could not verify it myself.)
/Björn
> -----Original Message-----
> From: Yung, Douglas <douglas.yung at sony.com>
> Sent: Monday, 14 August 2023 12:48
> To: Björn Pettersson A <bjorn.a.pettersson at ericsson.com>; Bjorn Pettersson
> <llvmlistbot at llvm.org>; llvm-commits at lists.llvm.org
> Cc: Tom.Weaver at sony.com
> Subject: RE: [llvm] 85eeba4 - [ConstantHoisting] Add back ptr->ptr bitcast to
> avoid assertion failure
>
> Hi Bjorn,
>
> The test you added seems to be hitting an assertion failure on Windows build
> bots:
> (https://lab.llvm.org/buildbot/#/builders/216/builds/25550)
>
> Assertion failed: UsesNum == (ReBasesNum + NotRebasedNum) && "Not all
> uses are rebased", file Z:\b\llvm-clang-x86_64-sie-win\llvm-
> project\llvm\lib\Transforms\Scalar\ConstantHoisting.cpp, line 919
>
> The failure is also appears in another build bot
> https://lab.llvm.org/buildbot/#/builders/233/builds/2011.
>
> Is this assertion failure expected? Can you take a look?
>
> Douglas Yung
>
> -----Original Message-----
> From: llvm-commits <llvm-commits-bounces at lists.llvm.org> On Behalf Of Bjorn
> Pettersson via llvm-commits
> Sent: Monday, August 14, 2023 2:14
> To: llvm-commits at lists.llvm.org
> Subject: [llvm] 85eeba4 - [ConstantHoisting] Add back ptr->ptr bitcast to avoid
> assertion failure
>
>
> Author: Bjorn Pettersson
> Date: 2023-08-14T11:13:13+02:00
> New Revision: 85eeba48a0fefcdf6d12cf422d2060592f689823
>
> URL: https://protect2.fireeye.com/v1/url?k=31323334-501d5122-313273af-
> 454445555731-714986eb964f5fce&q=1&e=ac08994d-6683-4a94-9a5e-
> 4c37b689050a&u=https%3A%2F%2Fgithub.com%2Fllvm%2Fllvm-
> project%2Fcommit%2F85eeba48a0fefcdf6d12cf422d2060592f689823
> DIFF: https://protect2.fireeye.com/v1/url?k=31323334-501d5122-313273af-
> 454445555731-2b88ad498bcdb76a&q=1&e=ac08994d-6683-4a94-9a5e-
> 4c37b689050a&u=https%3A%2F%2Fgithub.com%2Fllvm%2Fllvm-
> project%2Fcommit%2F85eeba48a0fefcdf6d12cf422d2060592f689823.diff
>
> LOG: [ConstantHoisting] Add back ptr->ptr bitcast to avoid assertion failure
>
> In commit a7ee80fab213fe7a a ptr->ptr bitcast was removed. But that seem to
> cause "Expected an cast instruction!" assertions later in that pass. This patch
> will add back the bitcast again.
>
> This was a bit unexpected since there is no bitcast added after creating the Add
> instruction in the else clause, but I guess there is something special with the
> GetElementPtr scenario which makes this bitcast needed to avoid such asserts.
>
> This patch is also adding a reproducer for
> https://protect2.fireeye.com/v1/url?k=31323334-501d5122-313273af-
> 454445555731-37342d51e28efd86&q=1&e=ac08994d-6683-4a94-9a5e-
> 4c37b689050a&u=https%3A%2F%2Fgithub.com%2Fllvm%2Fllvm-
> project%2Fissues%2F52689
> that started to fail due to hitting the above mentioned assert. Now it should
> end up hitting the assertion failure from #52689 again.
>
> Added:
> llvm/test/Transforms/ConstantHoisting/X86/pr52689-not-all-uses-rebased.ll
>
> Modified:
> llvm/lib/Transforms/Scalar/ConstantHoisting.cpp
> llvm/test/Transforms/ConstantHoisting/AArch64/const-hoist-gep.ll
> llvm/test/Transforms/ConstantHoisting/ARM/const-hoist-gep-overindexing.ll
> llvm/test/Transforms/ConstantHoisting/ARM/const-hoist-gep.ll
>
> Removed:
>
>
>
> ###################################################################
> #############
> diff --git a/llvm/lib/Transforms/Scalar/ConstantHoisting.cpp
> b/llvm/lib/Transforms/Scalar/ConstantHoisting.cpp
> index 0c9dea343a25e7..3e5d979f11cc50 100644
> --- a/llvm/lib/Transforms/Scalar/ConstantHoisting.cpp
> +++ b/llvm/lib/Transforms/Scalar/ConstantHoisting.cpp
> @@ -763,6 +763,8 @@ void
> ConstantHoistingPass::emitBaseConstants(Instruction *Base,
> // Constant being rebased is a ConstantExpr.
> Mat = GetElementPtrInst::Create(Type::getInt8Ty(*Ctx), Base, Adj->Offset,
> "mat_gep", Adj->MatInsertPt);
> + // Hide it behind a bitcast.
> + Mat = new BitCastInst(Mat, Adj->Ty, "mat_bitcast",
> + Adj->MatInsertPt);
> } else
> // Constant being rebased is a ConstantInt.
> Mat = BinaryOperator::Create(Instruction::Add, Base, Adj->Offset,
>
> diff --git a/llvm/test/Transforms/ConstantHoisting/AArch64/const-hoist-gep.ll
> b/llvm/test/Transforms/ConstantHoisting/AArch64/const-hoist-gep.ll
> index e540b146f560f4..da0b2e68313cd5 100644
> --- a/llvm/test/Transforms/ConstantHoisting/AArch64/const-hoist-gep.ll
> +++ b/llvm/test/Transforms/ConstantHoisting/AArch64/const-hoist-gep.ll
> @@ -22,11 +22,14 @@ define dso_local void @zot() {
> ; CHECK-NEXT: [[CONST:%.*]] = bitcast ptr getelementptr inbounds
> ([[TMP0:%.*]], ptr @global, i32 0, i32 4, i32 0, i32 0) to ptr
> ; CHECK-NEXT: store i32 undef, ptr [[CONST]], align 4
> ; CHECK-NEXT: [[MAT_GEP:%.*]] = getelementptr i8, ptr [[CONST]], i32 4
> -; CHECK-NEXT: store i32 undef, ptr [[MAT_GEP]], align 4
> +; CHECK-NEXT: [[MAT_BITCAST:%.*]] = bitcast ptr [[MAT_GEP]] to ptr
> +; CHECK-NEXT: store i32 undef, ptr [[MAT_BITCAST]], align 4
> ; CHECK-NEXT: [[MAT_GEP1:%.*]] = getelementptr i8, ptr [[CONST]], i32 160
> -; CHECK-NEXT: store i32 undef, ptr [[MAT_GEP1]], align 4
> -; CHECK-NEXT: [[MAT_GEP2:%.*]] = getelementptr i8, ptr [[CONST]], i32 164
> -; CHECK-NEXT: store i32 undef, ptr [[MAT_GEP2]], align 4
> +; CHECK-NEXT: [[MAT_BITCAST2:%.*]] = bitcast ptr [[MAT_GEP1]] to ptr
> +; CHECK-NEXT: store i32 undef, ptr [[MAT_BITCAST2]], align 4
> +; CHECK-NEXT: [[MAT_GEP3:%.*]] = getelementptr i8, ptr [[CONST]], i32 164
> +; CHECK-NEXT: [[MAT_BITCAST4:%.*]] = bitcast ptr [[MAT_GEP3]] to ptr
> +; CHECK-NEXT: store i32 undef, ptr [[MAT_BITCAST4]], align 4
> ; CHECK-NEXT: ret void
> ;
> bb:
>
> diff --git a/llvm/test/Transforms/ConstantHoisting/ARM/const-hoist-gep-
> overindexing.ll b/llvm/test/Transforms/ConstantHoisting/ARM/const-hoist-gep-
> overindexing.ll
> index 36ef7045fd05ee..08ce51cefadcfe 100644
> --- a/llvm/test/Transforms/ConstantHoisting/ARM/const-hoist-gep-
> overindexing.ll
> +++ b/llvm/test/Transforms/ConstantHoisting/ARM/const-hoist-gep-overinde
> +++ xing.ll
> @@ -14,9 +14,11 @@ define void @test_inbounds() {
> ; CHECK-NEXT: [[CONST:%.*]] = bitcast ptr getelementptr inbounds
> ([[TMP0:%.*]], ptr @global, i32 0, i32 1, i32 0) to ptr
> ; CHECK-NEXT: store i16 undef, ptr [[CONST]], align 2
> ; CHECK-NEXT: [[MAT_GEP:%.*]] = getelementptr i8, ptr [[CONST]], i32 2
> -; CHECK-NEXT: store i16 undef, ptr [[MAT_GEP]], align 2
> +; CHECK-NEXT: [[MAT_BITCAST:%.*]] = bitcast ptr [[MAT_GEP]] to ptr
> +; CHECK-NEXT: store i16 undef, ptr [[MAT_BITCAST]], align 2
> ; CHECK-NEXT: [[MAT_GEP1:%.*]] = getelementptr i8, ptr [[CONST]], i32 20
> -; CHECK-NEXT: store i16 undef, ptr [[MAT_GEP1]], align 2
> +; CHECK-NEXT: [[MAT_BITCAST2:%.*]] = bitcast ptr [[MAT_GEP1]] to ptr
> +; CHECK-NEXT: store i16 undef, ptr [[MAT_BITCAST2]], align 2
> ; CHECK-NEXT: ret void
> ;
> bb:
>
> diff --git a/llvm/test/Transforms/ConstantHoisting/ARM/const-hoist-gep.ll
> b/llvm/test/Transforms/ConstantHoisting/ARM/const-hoist-gep.ll
> index b5c58aaa367098..2a11a7a15b858a 100644
> --- a/llvm/test/Transforms/ConstantHoisting/ARM/const-hoist-gep.ll
> +++ b/llvm/test/Transforms/ConstantHoisting/ARM/const-hoist-gep.ll
> @@ -24,10 +24,12 @@ define dso_local void @zot() {
> ; CHECK-NEXT: [[CONST:%.*]] = bitcast ptr getelementptr inbounds
> ([[TMP0]], ptr @global, i32 0, i32 4, i32 0, i32 0) to ptr
> ; CHECK-NEXT: store i32 undef, ptr [[CONST]], align 4
> ; CHECK-NEXT: [[MAT_GEP:%.*]] = getelementptr i8, ptr [[CONST]], i32 4
> -; CHECK-NEXT: store i32 undef, ptr [[MAT_GEP]], align 4
> +; CHECK-NEXT: [[MAT_BITCAST:%.*]] = bitcast ptr [[MAT_GEP]] to ptr
> +; CHECK-NEXT: store i32 undef, ptr [[MAT_BITCAST]], align 4
> ; CHECK-NEXT: store i32 undef, ptr [[CONST1]], align 4
> ; CHECK-NEXT: [[MAT_GEP2:%.*]] = getelementptr i8, ptr [[CONST1]], i32 4
> -; CHECK-NEXT: store i32 undef, ptr [[MAT_GEP2]], align 4
> +; CHECK-NEXT: [[MAT_BITCAST3:%.*]] = bitcast ptr [[MAT_GEP2]] to ptr
> +; CHECK-NEXT: store i32 undef, ptr [[MAT_BITCAST3]], align 4
> ; CHECK-NEXT: ret void
> ;
> bb:
>
> diff --git a/llvm/test/Transforms/ConstantHoisting/X86/pr52689-not-all-uses-
> rebased.ll b/llvm/test/Transforms/ConstantHoisting/X86/pr52689-not-all-uses-
> rebased.ll
> new file mode 100644
> index 00000000000000..00d77f9963e983
> --- /dev/null
> +++ b/llvm/test/Transforms/ConstantHoisting/X86/pr52689-not-all-uses-reb
> +++ ased.ll
> @@ -0,0 +1,39 @@
> +; RUN: not --crash opt -passes='consthoist' -S -o - -consthoist-gep=1
> +-mtriple=x86_64-unknown-linux-gnu < %s 2>&1 | FileCheck %s
> +
> +; REQUIRES: asserts
> +
> +; This is a reproducer for
> +INVALID URI REMOVED
> +/52689__;!!JmoZiZGBv3RvKRSx!5bL5AqmIJya5Tdog9D8MSk0ZnO2rhq0bIpQ7
> mYlCD6v
> +Dt4xg3j8j-fhMZZCGsDd-zhsK-uG3b4Y_rb0Q812GnqeKXnm3$
> +;
> +; opt: ../lib/Transforms/Scalar/ConstantHoisting.cpp:919: bool
> llvm::ConstantHoistingPass::emitBaseConstants(llvm::GlobalVariable *):
> Assertion `UsesNum == (ReBasesNum + NotRebasedNum) && "Not all uses are
> rebased"' failed.
> +
> +; CHECK: Assertion `UsesNum == (ReBasesNum + NotRebasedNum) && "Not
> all uses are rebased"' failed.
> +
> +
> + at g_77 = external global [5 x i32]
> +
> +define internal ptr @func_29(i1 %p1, i1 %p2, ptr %p3) {
> +entry:
> + br i1 %p1, label %crit_edge, label %if.else3089
> +
> +crit_edge: ; preds = %entry
> + br label %for.cond1063
> +
> +for.cond1063: ; preds = %cleanup1660, %crit_edge
> + %l_323.sroa.0.0 = phi ptr [ getelementptr inbounds ([5 x i32], ptr
> + at g_77, i32 0, i32 3), %cleanup1660 ], [ null, %crit_edge ]
> + %l_323.sroa.2.0 = phi ptr [ getelementptr inbounds ([5 x i32], ptr
> + at g_77, i32 0, i32 3), %cleanup1660 ], [ null, %crit_edge ]
> + br i1 %p2, label %cleanup1660.thread, label %cleanup1674
> +
> +cleanup1660.thread: ; preds = %for.cond1063
> + br label %cleanup1674
> +
> +cleanup1660: ; No predecessors!
> + br label %for.cond1063
> +
> +cleanup1674: ; preds = %cleanup1660.thread,
> %for.cond1063
> + store ptr getelementptr inbounds ([5 x i32], ptr @g_77, i32 0, i32
> +1), ptr %p3, align 1
> + ret ptr null
> +
> +if.else3089: ; preds = %entry
> + store ptr getelementptr inbounds ([5 x i32], ptr @g_77, i32 0, i32
> +1), ptr %p3, align 1
> + ret ptr null
> +}
>
>
>
> _______________________________________________
> 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