From 79c7fa31f3aafba77862325cdc70a3ece7f71950 Mon Sep 17 00:00:00 2001 From: Sanjay Patel Date: Sat, 21 Dec 2019 10:31:21 -0500 Subject: [PATCH] [InstCombine] check alloc size in bitcast of geps fold (PR44321) We missed a constraint in D44833 when folding a bitcast into a GEP with vector/array types. If the alloc sizes specified by the datalayout don't match, this could miscompile as shown in: https://bugs.llvm.org/show_bug.cgi?id=44321 Differential Revision: https://reviews.llvm.org/D71771 --- .../InstCombine/InstructionCombining.cpp | 10 +++--- llvm/test/Transforms/InstCombine/gep-vector.ll | 36 +++++++++++++++++----- 2 files changed, 34 insertions(+), 12 deletions(-) diff --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp index d423c38..9037dd7 100644 --- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp +++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp @@ -2191,15 +2191,17 @@ Instruction *InstCombiner::visitGetElementPtrInst(GetElementPtrInst &GEP) { // of a bitcasted pointer to vector or array of the same dimensions: // gep (bitcast * X to [c x ty]*), Y, Z --> gep X, Y, Z // gep (bitcast [c x ty]* X to *), Y, Z --> gep X, Y, Z - auto areMatchingArrayAndVecTypes = [](Type *ArrTy, Type *VecTy) { + auto areMatchingArrayAndVecTypes = [](Type *ArrTy, Type *VecTy, + const DataLayout &DL) { return ArrTy->getArrayElementType() == VecTy->getVectorElementType() && - ArrTy->getArrayNumElements() == VecTy->getVectorNumElements(); + ArrTy->getArrayNumElements() == VecTy->getVectorNumElements() && + DL.getTypeAllocSize(ArrTy) == DL.getTypeAllocSize(VecTy); }; if (GEP.getNumOperands() == 3 && ((GEPEltType->isArrayTy() && SrcEltType->isVectorTy() && - areMatchingArrayAndVecTypes(GEPEltType, SrcEltType)) || + areMatchingArrayAndVecTypes(GEPEltType, SrcEltType, DL)) || (GEPEltType->isVectorTy() && SrcEltType->isArrayTy() && - areMatchingArrayAndVecTypes(SrcEltType, GEPEltType)))) { + areMatchingArrayAndVecTypes(SrcEltType, GEPEltType, DL)))) { // Create a new GEP here, as using `setOperand()` followed by // `setSourceElementType()` won't actually update the type of the diff --git a/llvm/test/Transforms/InstCombine/gep-vector.ll b/llvm/test/Transforms/InstCombine/gep-vector.ll index 64ee29e..432da0a 100644 --- a/llvm/test/Transforms/InstCombine/gep-vector.ll +++ b/llvm/test/Transforms/InstCombine/gep-vector.ll @@ -27,9 +27,12 @@ define <2 x i8*> @vectorindex3() { ret <2 x i8*> %1 } +; Negative test - datalayout's alloc size for the 2 types must match. + define i32* @bitcast_vec_to_array_gep(<7 x i32>* %x, i64 %y, i64 %z) { ; CHECK-LABEL: @bitcast_vec_to_array_gep( -; CHECK-NEXT: [[GEP:%.*]] = getelementptr <7 x i32>, <7 x i32>* [[X:%.*]], i64 [[Y:%.*]], i64 [[Z:%.*]] +; CHECK-NEXT: [[ARR_PTR:%.*]] = bitcast <7 x i32>* [[X:%.*]] to [7 x i32]* +; CHECK-NEXT: [[GEP:%.*]] = getelementptr [7 x i32], [7 x i32]* [[ARR_PTR]], i64 [[Y:%.*]], i64 [[Z:%.*]] ; CHECK-NEXT: ret i32* [[GEP]] ; %arr_ptr = bitcast <7 x i32>* %x to [7 x i32]* @@ -37,9 +40,12 @@ define i32* @bitcast_vec_to_array_gep(<7 x i32>* %x, i64 %y, i64 %z) { ret i32* %gep } +; Negative test - datalayout's alloc size for the 2 types must match. + define i32* @bitcast_array_to_vec_gep([3 x i32]* %x, i64 %y, i64 %z) { ; CHECK-LABEL: @bitcast_array_to_vec_gep( -; CHECK-NEXT: [[GEP:%.*]] = getelementptr inbounds [3 x i32], [3 x i32]* [[X:%.*]], i64 [[Y:%.*]], i64 [[Z:%.*]] +; CHECK-NEXT: [[VEC_PTR:%.*]] = bitcast [3 x i32]* [[X:%.*]] to <3 x i32>* +; CHECK-NEXT: [[GEP:%.*]] = getelementptr inbounds <3 x i32>, <3 x i32>* [[VEC_PTR]], i64 [[Y:%.*]], i64 [[Z:%.*]] ; CHECK-NEXT: ret i32* [[GEP]] ; %vec_ptr = bitcast [3 x i32]* %x to <3 x i32>* @@ -47,6 +53,8 @@ define i32* @bitcast_array_to_vec_gep([3 x i32]* %x, i64 %y, i64 %z) { ret i32* %gep } +; Sizes and types match - safe to remove bitcast. + define i32* @bitcast_vec_to_array_gep_matching_alloc_size(<4 x i32>* %x, i64 %y, i64 %z) { ; CHECK-LABEL: @bitcast_vec_to_array_gep_matching_alloc_size( ; CHECK-NEXT: [[GEP:%.*]] = getelementptr <4 x i32>, <4 x i32>* [[X:%.*]], i64 [[Y:%.*]], i64 [[Z:%.*]] @@ -57,6 +65,8 @@ define i32* @bitcast_vec_to_array_gep_matching_alloc_size(<4 x i32>* %x, i64 %y, ret i32* %gep } +; Sizes and types match - safe to remove bitcast. + define i32* @bitcast_array_to_vec_gep_matching_alloc_size([4 x i32]* %x, i64 %y, i64 %z) { ; CHECK-LABEL: @bitcast_array_to_vec_gep_matching_alloc_size( ; CHECK-NEXT: [[GEP:%.*]] = getelementptr inbounds [4 x i32], [4 x i32]* [[X:%.*]], i64 [[Y:%.*]], i64 [[Z:%.*]] @@ -67,11 +77,14 @@ define i32* @bitcast_array_to_vec_gep_matching_alloc_size([4 x i32]* %x, i64 %y, ret i32* %gep } +; Negative test - datalayout's alloc size for the 2 types must match. + define i32 addrspace(3)* @bitcast_vec_to_array_addrspace(<7 x i32>* %x, i64 %y, i64 %z) { ; CHECK-LABEL: @bitcast_vec_to_array_addrspace( -; CHECK-NEXT: [[GEP:%.*]] = getelementptr <7 x i32>, <7 x i32>* [[X:%.*]], i64 [[Y:%.*]], i64 [[Z:%.*]] -; CHECK-NEXT: [[TMP1:%.*]] = addrspacecast i32* [[GEP]] to i32 addrspace(3)* -; CHECK-NEXT: ret i32 addrspace(3)* [[TMP1]] +; CHECK-NEXT: [[ARR_PTR:%.*]] = bitcast <7 x i32>* [[X:%.*]] to [7 x i32]* +; CHECK-NEXT: [[ASC:%.*]] = addrspacecast [7 x i32]* [[ARR_PTR]] to [7 x i32] addrspace(3)* +; CHECK-NEXT: [[GEP:%.*]] = getelementptr [7 x i32], [7 x i32] addrspace(3)* [[ASC]], i64 [[Y:%.*]], i64 [[Z:%.*]] +; CHECK-NEXT: ret i32 addrspace(3)* [[GEP]] ; %arr_ptr = bitcast <7 x i32>* %x to [7 x i32]* %asc = addrspacecast [7 x i32]* %arr_ptr to [7 x i32] addrspace(3)* @@ -79,11 +92,14 @@ define i32 addrspace(3)* @bitcast_vec_to_array_addrspace(<7 x i32>* %x, i64 %y, ret i32 addrspace(3)* %gep } +; Negative test - datalayout's alloc size for the 2 types must match. + define i32 addrspace(3)* @inbounds_bitcast_vec_to_array_addrspace(<7 x i32>* %x, i64 %y, i64 %z) { ; CHECK-LABEL: @inbounds_bitcast_vec_to_array_addrspace( -; CHECK-NEXT: [[GEP:%.*]] = getelementptr inbounds <7 x i32>, <7 x i32>* [[X:%.*]], i64 [[Y:%.*]], i64 [[Z:%.*]] -; CHECK-NEXT: [[TMP1:%.*]] = addrspacecast i32* [[GEP]] to i32 addrspace(3)* -; CHECK-NEXT: ret i32 addrspace(3)* [[TMP1]] +; CHECK-NEXT: [[ARR_PTR:%.*]] = bitcast <7 x i32>* [[X:%.*]] to [7 x i32]* +; CHECK-NEXT: [[ASC:%.*]] = addrspacecast [7 x i32]* [[ARR_PTR]] to [7 x i32] addrspace(3)* +; CHECK-NEXT: [[GEP:%.*]] = getelementptr inbounds [7 x i32], [7 x i32] addrspace(3)* [[ASC]], i64 [[Y:%.*]], i64 [[Z:%.*]] +; CHECK-NEXT: ret i32 addrspace(3)* [[GEP]] ; %arr_ptr = bitcast <7 x i32>* %x to [7 x i32]* %asc = addrspacecast [7 x i32]* %arr_ptr to [7 x i32] addrspace(3)* @@ -91,6 +107,8 @@ define i32 addrspace(3)* @inbounds_bitcast_vec_to_array_addrspace(<7 x i32>* %x, ret i32 addrspace(3)* %gep } +; Sizes and types match - safe to remove bitcast. + define i32 addrspace(3)* @bitcast_vec_to_array_addrspace_matching_alloc_size(<4 x i32>* %x, i64 %y, i64 %z) { ; CHECK-LABEL: @bitcast_vec_to_array_addrspace_matching_alloc_size( ; CHECK-NEXT: [[GEP:%.*]] = getelementptr <4 x i32>, <4 x i32>* [[X:%.*]], i64 [[Y:%.*]], i64 [[Z:%.*]] @@ -103,6 +121,8 @@ define i32 addrspace(3)* @bitcast_vec_to_array_addrspace_matching_alloc_size(<4 ret i32 addrspace(3)* %gep } +; Sizes and types match - safe to remove bitcast. + define i32 addrspace(3)* @inbounds_bitcast_vec_to_array_addrspace_matching_alloc_size(<4 x i32>* %x, i64 %y, i64 %z) { ; CHECK-LABEL: @inbounds_bitcast_vec_to_array_addrspace_matching_alloc_size( ; CHECK-NEXT: [[GEP:%.*]] = getelementptr inbounds <4 x i32>, <4 x i32>* [[X:%.*]], i64 [[Y:%.*]], i64 [[Z:%.*]] -- 2.7.4