From fa120cbdbc66935dbe41e4fcc76030d50a2692d4 Mon Sep 17 00:00:00 2001 From: Ewan Crawford Date: Mon, 8 Oct 2018 08:40:45 +0000 Subject: [PATCH] [InstCombine] Fix incongruous GEP type addrspace Currently running the @insertelem_after_gep function below through the InstCombine pass with opt produces invalid IR. Input: ``` define void @insertelem_after_gep(<16 x i32>* %t0) { %t1 = bitcast <16 x i32>* %t0 to [16 x i32]* %t2 = addrspacecast [16 x i32]* %t1 to [16 x i32] addrspace(3)* %t3 = getelementptr inbounds [16 x i32], [16 x i32] addrspace(3)* %t2, i64 0, i64 0 %t4 = insertelement <16 x i32 addrspace(3)*> undef, i32 addrspace(3)* %t3, i32 0 call void @extern_vec_pointers_func(<16 x i32 addrspace(3)*> %t4) ret void } ``` Output: ``` define void @insertelem_after_gep(<16 x i32>* %t0) { %t3 = getelementptr inbounds <16 x i32>, <16 x i32>* %t0, i64 0, i64 0 %t4 = insertelement <16 x i32 addrspace(3)*> undef, i32 addrspace(3)* %t3, i32 0 call void @my_extern_func(<16 x i32 addrspace(3)*> %t4) ret void } ``` Which although causes no complaints when produced, isn't valid IR as the insertelement use of the %t3 GEP expects an address space. ``` opt: /tmp/bad.ll:52:73: error: '%t3' defined with type 'i32*' but expected 'i32 addrspace(3)*' %t4 = insertelement <16 x i32 addrspace(3)*> undef, i32 addrspace(3)* %t3, i32 0 ``` I've fixed this by adding an addrspacecast after the GEP in the InstCombine pass, and including a check for this type mismatch to the verifier. Reviewers: spatel, lebedev.ri Subscribers: llvm-commits Differential Revision: https://reviews.llvm.org/D52294 llvm-svn: 343956 --- llvm/lib/IR/Verifier.cpp | 6 ++++++ .../InstCombine/InstructionCombining.cpp | 19 +++++++++++++++--- llvm/test/Transforms/InstCombine/gep-vector.ll | 23 ++++++++++++++++++++++ 3 files changed, 45 insertions(+), 3 deletions(-) diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp index 84e9c6f..4b954c7 100644 --- a/llvm/lib/IR/Verifier.cpp +++ b/llvm/lib/IR/Verifier.cpp @@ -3142,6 +3142,12 @@ void Verifier::visitGetElementPtrInst(GetElementPtrInst &GEP) { "All GEP indices should be of integer type"); } } + + if (auto *PTy = dyn_cast(GEP.getType())) { + Assert(GEP.getAddressSpace() == PTy->getAddressSpace(), + "GEP address space doesn't match type", &GEP); + } + visitInstruction(GEP); } diff --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp index 70fea85..00ffe9e 100644 --- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp +++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp @@ -2052,9 +2052,22 @@ Instruction *InstCombiner::visitGetElementPtrInst(GetElementPtrInst &GEP) { areMatchingArrayAndVecTypes(GEPEltType, SrcEltType)) || (GEPEltType->isVectorTy() && SrcEltType->isArrayTy() && areMatchingArrayAndVecTypes(SrcEltType, GEPEltType)))) { - GEP.setOperand(0, SrcOp); - GEP.setSourceElementType(SrcEltType); - return &GEP; + + // Create a new GEP here, as using `setOperand()` followed by + // `setSourceElementType()` won't actually update the type of the + // existing GEP Value. Causing issues if this Value is accessed when + // constructing an AddrSpaceCastInst + Value *NGEP = + GEP.isInBounds() + ? Builder.CreateInBoundsGEP(nullptr, SrcOp, {Ops[1], Ops[2]}) + : Builder.CreateGEP(nullptr, SrcOp, {Ops[1], Ops[2]}); + NGEP->takeName(&GEP); + + // Preserve GEP address space to satisfy users + if (NGEP->getType()->getPointerAddressSpace() != GEP.getAddressSpace()) + return new AddrSpaceCastInst(NGEP, GEPType); + + return replaceInstUsesWith(GEP, NGEP); } // See if we can simplify: diff --git a/llvm/test/Transforms/InstCombine/gep-vector.ll b/llvm/test/Transforms/InstCombine/gep-vector.ll index a8247db..c0db01e 100644 --- a/llvm/test/Transforms/InstCombine/gep-vector.ll +++ b/llvm/test/Transforms/InstCombine/gep-vector.ll @@ -47,3 +47,26 @@ define i32* @bitcast_array_to_vec_gep([3 x i32]* %x, i64 %y, i64 %z) { ret i32* %gep } +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]] +; + %arr_ptr = bitcast <7 x i32>* %x to [7 x i32]* + %asc = addrspacecast [7 x i32]* %arr_ptr to [7 x i32] addrspace(3)* + %gep = getelementptr [7 x i32], [7 x i32] addrspace(3)* %asc, i64 %y, i64 %z + ret i32 addrspace(3)* %gep +} + +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]] +; + %arr_ptr = bitcast <7 x i32>* %x to [7 x i32]* + %asc = addrspacecast [7 x i32]* %arr_ptr to [7 x i32] addrspace(3)* + %gep = getelementptr inbounds [7 x i32], [7 x i32] addrspace(3)* %asc, i64 %y, i64 %z + ret i32 addrspace(3)* %gep +} -- 2.7.4