From e6b086bef2c0597ed14b2aefbb3f6d74b94fd49e Mon Sep 17 00:00:00 2001 From: Nathan Chancellor Date: Mon, 31 May 2021 20:11:46 -0700 Subject: [PATCH] Revert "[InstCombine] Fix miscompile on GEP+load to icmp fold (PR45210)" This reverts commit 4f2fd3818b0eb26806f366bc37369349aeedcaf9. The Linux kernel fails to build after this commit. See https://reviews.llvm.org/D99481 for a reproducer. Signed-off-by: Nathan Chancellor --- .../Transforms/InstCombine/InstCombineCompares.cpp | 22 +++--------------- llvm/test/Transforms/InstCombine/load-cmp.ll | 26 ++++++++-------------- 2 files changed, 12 insertions(+), 36 deletions(-) diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp index 5e60edd..9e4d888 100644 --- a/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp +++ b/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp @@ -269,30 +269,14 @@ InstCombinerImpl::foldCmpLoadFromIndexedGlobal(GetElementPtrInst *GEP, // order the state machines in complexity of the generated code. Value *Idx = GEP->getOperand(2); + // If the index is larger than the pointer size of the target, truncate the + // index down like the GEP would do implicitly. We don't have to do this for + // an inbounds GEP because the index can't be out of range. if (!GEP->isInBounds()) { - // If the index is larger than the pointer size of the target, truncate the - // index down like the GEP would do implicitly. We don't have to do this - // for an inbounds GEP because the index can't be out of range. Type *IntPtrTy = DL.getIntPtrType(GEP->getType()); unsigned PtrSize = IntPtrTy->getIntegerBitWidth(); if (Idx->getType()->getPrimitiveSizeInBits().getFixedSize() > PtrSize) Idx = Builder.CreateTrunc(Idx, IntPtrTy); - - unsigned ElementSize = - DL.getTypeAllocSize(Init->getType()->getArrayElementType()); - - // If inbounds keyword is not present, Idx * ElementSize can overflow. - // Let's assume that ElementSize is 2 and the wanted value is at offset 0. - // Then, there are two possible values for Idx to match offset 0: - // 0x00..00, 0x80..00. - // Emitting 'icmp eq Idx, 0' isn't correct in this case because the - // comparison is false if Idx was 0x80..00. - // We need to erase the highest countTrailingZeros(ElementSize) bits of Idx. - if (countTrailingZeros(ElementSize) != 0) { - Value *Mask = ConstantInt::getSigned(Idx->getType(), -1); - Mask = Builder.CreateLShr(Mask, countTrailingZeros(ElementSize)); - Idx = Builder.CreateAnd(Idx, Mask); - } } // If the comparison is only true for one or two elements, emit direct diff --git a/llvm/test/Transforms/InstCombine/load-cmp.ll b/llvm/test/Transforms/InstCombine/load-cmp.ll index b56cfdd..84f692d 100644 --- a/llvm/test/Transforms/InstCombine/load-cmp.ll +++ b/llvm/test/Transforms/InstCombine/load-cmp.ll @@ -33,8 +33,7 @@ define i1 @test1(i32 %X) { define i1 @test1_noinbounds(i32 %X) { ; CHECK-LABEL: @test1_noinbounds( -; CHECK-NEXT: [[TMP1:%.*]] = and i32 [[X:%.*]], 2147483647 -; CHECK-NEXT: [[R:%.*]] = icmp eq i32 [[TMP1]], 9 +; CHECK-NEXT: [[R:%.*]] = icmp eq i32 [[X:%.*]], 9 ; CHECK-NEXT: ret i1 [[R]] ; %P = getelementptr [10 x i16], [10 x i16]* @G16, i32 0, i32 %X @@ -45,8 +44,8 @@ define i1 @test1_noinbounds(i32 %X) { define i1 @test1_noinbounds_i64(i64 %X) { ; CHECK-LABEL: @test1_noinbounds_i64( -; CHECK-NEXT: [[TMP1:%.*]] = and i64 [[X:%.*]], 2147483647 -; CHECK-NEXT: [[R:%.*]] = icmp eq i64 [[TMP1]], 9 +; CHECK-NEXT: [[TMP1:%.*]] = trunc i64 [[X:%.*]] to i32 +; CHECK-NEXT: [[R:%.*]] = icmp eq i32 [[TMP1]], 9 ; CHECK-NEXT: ret i1 [[R]] ; %P = getelementptr [10 x i16], [10 x i16]* @G16, i64 0, i64 %X @@ -55,14 +54,10 @@ define i1 @test1_noinbounds_i64(i64 %X) { ret i1 %R } -; When x is 0x8009, x * 2(ElementSize) overflows and it becomes 0x0012. -; It is the same location as when x is 0x0009, So we need to return -; true if x AND 0x7FFF is 0x0009. - define i1 @test1_noinbounds_as1(i32 %x) { ; CHECK-LABEL: @test1_noinbounds_as1( -; CHECK-NEXT: [[TMP1:%.*]] = and i32 [[X:%.*]], 32767 -; CHECK-NEXT: [[R:%.*]] = icmp eq i32 [[TMP1]], 9 +; CHECK-NEXT: [[TMP1:%.*]] = trunc i32 [[X:%.*]] to i16 +; CHECK-NEXT: [[R:%.*]] = icmp eq i16 [[TMP1]], 9 ; CHECK-NEXT: ret i1 [[R]] ; %p = getelementptr [10 x i16], [10 x i16] addrspace(1)* @G16_as1, i16 0, i32 %x @@ -265,8 +260,7 @@ define i1 @test10_struct_arr(i32 %x) { define i1 @test10_struct_arr_noinbounds(i32 %x) { ; CHECK-LABEL: @test10_struct_arr_noinbounds( -; CHECK-NEXT: [[TMP1:%.*]] = and i32 [[X:%.*]], 268435455 -; CHECK-NEXT: [[R:%.*]] = icmp ne i32 [[TMP1]], 1 +; CHECK-NEXT: [[R:%.*]] = icmp ne i32 [[X:%.*]], 1 ; CHECK-NEXT: ret i1 [[R]] ; %p = getelementptr [4 x %Foo], [4 x %Foo]* @GStructArr, i32 0, i32 %x, i32 2 @@ -300,9 +294,7 @@ define i1 @test10_struct_arr_i64(i64 %x) { define i1 @test10_struct_arr_noinbounds_i16(i16 %x) { ; CHECK-LABEL: @test10_struct_arr_noinbounds_i16( -; CHECK-NEXT: [[TMP1:%.*]] = sext i16 [[X:%.*]] to i32 -; CHECK-NEXT: [[TMP2:%.*]] = and i32 [[TMP1]], 268435455 -; CHECK-NEXT: [[R:%.*]] = icmp ne i32 [[TMP2]], 1 +; CHECK-NEXT: [[R:%.*]] = icmp ne i16 [[X:%.*]], 1 ; CHECK-NEXT: ret i1 [[R]] ; %p = getelementptr [4 x %Foo], [4 x %Foo]* @GStructArr, i32 0, i16 %x, i32 2 @@ -313,8 +305,8 @@ define i1 @test10_struct_arr_noinbounds_i16(i16 %x) { define i1 @test10_struct_arr_noinbounds_i64(i64 %x) { ; CHECK-LABEL: @test10_struct_arr_noinbounds_i64( -; CHECK-NEXT: [[TMP1:%.*]] = and i64 [[X:%.*]], 268435455 -; CHECK-NEXT: [[R:%.*]] = icmp ne i64 [[TMP1]], 1 +; CHECK-NEXT: [[TMP1:%.*]] = trunc i64 [[X:%.*]] to i32 +; CHECK-NEXT: [[R:%.*]] = icmp ne i32 [[TMP1]], 1 ; CHECK-NEXT: ret i1 [[R]] ; %p = getelementptr [4 x %Foo], [4 x %Foo]* @GStructArr, i32 0, i64 %x, i32 2 -- 2.7.4