[PATCH] [InstCombine] Don't fold bitcast into store if it would need addrspacecast

[Matt Arsenault]

On 03/19/2014 06:11 AM, Richard Osborne wrote:
> Previously the code didn't check if the before and after types for the
> store were pointers to different address spaces. This resulted in
> instcombine using a bitcast to convert between pointers to different
> address spaces, causing an assertion due to the invalid cast.
>
> It is not be appropriate to use addrspacecast this case because it is
> not guaranteed to be a no-op cast. Instead bail out and do not do the
> transformation.
>
> http://llvm-reviews.chandlerc.com/D3117
>
> Files:
>    lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
>    test/Transforms/InstCombine/bitcast-store.ll
>
> Index: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
> ===================================================================
> --- lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
> +++ lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
> @@ -517,27 +517,35 @@
>         IC.getDataLayout()->getTypeSizeInBits(DestPTy))
>       return 0;
>   
> +  // Don't introduce casts between pointers to different address spaces. We
> +  // can't use the addrspacecast instruction in this case since, depending on
> +  // the target, addrspacecast may not be a no-op cast.
> +  if (SrcPTy->isPointerTy() && DestPTy->isPointerTy() &&
> +      cast<PointerType>(SrcPTy)->getAddressSpace() !=
> +      cast<PointerType>(DestPTy)->getAddressSpace())
> +    return 0;
> +
There is getPointerAddressSpace() instead of needing to use the cast, 
but I think it's cleaner to just use dyn_cast instead of checking if 
it's a pointer and then casting to pointer.

Right above here is a check leftover from when bitcast was allowed if 
the pointers were the same size. You should update that and replace it 
with this.
>     // Okay, we are casting from one integer or pointer type to another of
>     // the same size.  Instead of casting the pointer before
>     // the store, cast the value to be stored.
>     Value *NewCast;
> -  Value *SIOp0 = SI.getOperand(0);
>     Instruction::CastOps opcode = Instruction::BitCast;
> -  Type* CastSrcTy = SIOp0->getType();
> +  Type* CastSrcTy = DestPTy;
>     Type* CastDstTy = SrcPTy;
>     if (CastDstTy->isPointerTy()) {
>       if (CastSrcTy->isIntegerTy())
>         opcode = Instruction::IntToPtr;
>     } else if (CastDstTy->isIntegerTy()) {
> -    if (SIOp0->getType()->isPointerTy())
> +    if (CastSrcTy->isPointerTy())
>         opcode = Instruction::PtrToInt;
>     }
>   
>     // SIOp0 is a pointer to aggregate and this is a store to the first field,
>     // emit a GEP to index into its first field.
>     if (!NewGEPIndices.empty())
>       CastOp = IC.Builder->CreateInBoundsGEP(CastOp, NewGEPIndices);
>   
> +  Value *SIOp0 = SI.getOperand(0);
>     NewCast = IC.Builder->CreateCast(opcode, SIOp0, CastDstTy,
>                                      SIOp0->getName()+".c");
>     SI.setOperand(0, NewCast);
> Index: test/Transforms/InstCombine/bitcast-store.ll
> ===================================================================
> --- test/Transforms/InstCombine/bitcast-store.ll
> +++ test/Transforms/InstCombine/bitcast-store.ll
> @@ -3,19 +3,33 @@
>   ; Instcombine should preserve metadata and alignment while
>   ; folding a bitcast into a store.
>   
> -; CHECK: store i32 (...)** bitcast (i8** getelementptr inbounds ([5 x i8*]* @G, i64 0, i64 2) to i32 (...)**), i32 (...)*** %0, align 16, !tag !0
> -
>   target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64"
>   
>   %struct.A = type { i32 (...)** }
>   
>   @G = external constant [5 x i8*]
>   
> +; CHECK-LABEL: @foo
> +; CHECK: store i32 (...)** bitcast (i8** getelementptr inbounds ([5 x i8*]* @G, i64 0, i64 2) to i32 (...)**), i32 (...)*** %0, align 16, !tag !0
>   define void @foo(%struct.A* %a) nounwind {
>   entry:
>     %0 = bitcast %struct.A* %a to i8***
>     store i8** getelementptr inbounds ([5 x i8*]* @G, i64 0, i64 2), i8*** %0, align 16, !tag !0
>     ret void
>   }
>   
>   !0 = metadata !{metadata !"hello"}
> +
> +; Check instcombine doesn't try and fold the following bitcast into the store.
> +; This transformation would not be safe since we would need to use addrspacecast
> +; and addrspacecast is not guaranteed to be a no-op cast.
> +
> +; CHECK-LABEL: @bar
> +; CHECK: %cast = bitcast i8** %b to i8 addrspace(1)**
> +; CHECK: store i8 addrspace(1)* %a, i8 addrspace(1)** %cast
> +define void @bar(i8 addrspace(1)* %a, i8** %b) nounwind {
> +entry:
> +  %cast = bitcast i8** %b to i8 addrspace(1)**
> +  store i8 addrspace(1)* %a, i8 addrspace(1)** %cast
> +  ret void
> +}
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140319/1214523e/attachment.html>