[llvm] r282612 - Don't look through addrspacecast in GetPointerBaseWithConstantOffset

Thu Sep 29 10:38:06 PDT 2016

On Thu, Sep 29, 2016 at 05:07:02PM +0000, Artur Pilipenko wrote:
> Hi,
> 
> In my fix I don't look through addrspace casts which change pointer size. It’s just of one of a possible conservative fixes for the assert in GetPointerBaseWithConstantOffset. Since the semantics of different addrspaces is not defined, I guess we can look through them even if the pointer size is changed. If you have a scenario when this kind of optimization matters, you can proceed with your solution.
> 

OK, I will commit my patch then, it does make a difference for us in
some cases.

> Note, that we have similar logic in Value::stripAndAccumulateInBoundsConstantOffsets and ValueTracking::isDereferenceableAndAlignedPointer. I have a patch for review which unifies their behavior. Can’t send a link right now because reviews.llvm.org<http://reviews.llvm.org> seems to be down. If you decide to look through addrspace cast even if it changes pointer size it will be nice to fix these two as well.
> 

OK, I think I also have a patch which fixes one of these other issues.
Can you add me as a reviewer to your patches once phabricator is back
up?

Thanks,
Tom

> Artur
> 
> On 29 Sep 2016, at 00:12, Tom Stellard <tom at stellard.net<mailto:tom at stellard.net>> wrote:
> 
> On Wed, Sep 28, 2016 at 05:57:19PM -0000, Artur Pilipenko via llvm-commits wrote:
> Author: apilipenko
> Date: Wed Sep 28 12:57:16 2016
> New Revision: 282612
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=282612&view=rev
> Log:
> Don't look through addrspacecast in GetPointerBaseWithConstantOffset
> 
> Pointers in different addrspaces can have different sizes, so it's not valid to look through addrspace cast calculating base and offset for a value.
> 
> 
> Hi,
> 
> Why is not valid to look through addrspacecasts?  I had proposed an alternative fix
> for this problem here: https://reviews.llvm.org/D24772 which still allows this.
> 
> -Tom
> 
> This is similar to D13008.
> 
> Reviewed By: reames
> 
> Differential Revision: https://reviews.llvm.org/D24729
> 
> Added:
>    llvm/trunk/test/Transforms/GVN/PRE/rle-addrspace-cast.ll
>    llvm/trunk/test/Transforms/GVN/addrspace-cast.ll
> Modified:
>    llvm/trunk/lib/Analysis/ValueTracking.cpp
>    llvm/trunk/test/Transforms/GVN/PRE/rle.ll
> 
> Modified: llvm/trunk/lib/Analysis/ValueTracking.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/ValueTracking.cpp?rev=282612&r1=282611&r2=282612&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Analysis/ValueTracking.cpp (original)
> +++ llvm/trunk/lib/Analysis/ValueTracking.cpp Wed Sep 28 12:57:16 2016
> @@ -2848,9 +2848,14 @@ Value *llvm::GetPointerBaseWithConstantO
>       ByteOffset += GEPOffset;
> 
>       Ptr = GEP->getPointerOperand();
> -    } else if (Operator::getOpcode(Ptr) == Instruction::BitCast ||
> -               Operator::getOpcode(Ptr) == Instruction::AddrSpaceCast) {
> +    } else if (Operator::getOpcode(Ptr) == Instruction::BitCast) {
>       Ptr = cast<Operator>(Ptr)->getOperand(0);
> +    } else if (AddrSpaceCastInst *ASCI = dyn_cast<AddrSpaceCastInst>(Ptr)) {
> +      Value *SourcePtr = ASCI->getPointerOperand();
> +      // Don't look through addrspace cast which changes pointer size
> +      if (BitWidth != DL.getPointerTypeSizeInBits(SourcePtr->getType()))
> +        break;
> +      Ptr = SourcePtr;
>     } else if (GlobalAlias *GA = dyn_cast<GlobalAlias>(Ptr)) {
>       if (GA->isInterposable())
>         break;
> 
> Added: llvm/trunk/test/Transforms/GVN/PRE/rle-addrspace-cast.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/GVN/PRE/rle-addrspace-cast.ll?rev=282612&view=auto
> ==============================================================================
> --- llvm/trunk/test/Transforms/GVN/PRE/rle-addrspace-cast.ll (added)
> +++ llvm/trunk/test/Transforms/GVN/PRE/rle-addrspace-cast.ll Wed Sep 28 12:57:16 2016
> @@ -0,0 +1,14 @@
> +; RUN: opt < %s -default-data-layout="e-p:32:32:32-p1:32:32:32-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:32:64-f32:32:32-f64:32:64-n8:16:32" -basicaa -gvn -S -die | FileCheck %s
> +
> +define i8 @coerce_offset0_addrspacecast(i32 %V, i32* %P) {
> +  store i32 %V, i32* %P
> +
> +  %P2 = addrspacecast i32* %P to i8 addrspace(1)*
> +  %P3 = getelementptr i8, i8 addrspace(1)* %P2, i32 2
> +
> +  %A = load i8, i8 addrspace(1)* %P3
> +  ret i8 %A
> +; CHECK-LABEL: @coerce_offset0_addrspacecast(
> +; CHECK-NOT: load
> +; CHECK: ret i8
> +}
> 
> Modified: llvm/trunk/test/Transforms/GVN/PRE/rle.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/GVN/PRE/rle.ll?rev=282612&r1=282611&r2=282612&view=diff
> ==============================================================================
> --- llvm/trunk/test/Transforms/GVN/PRE/rle.ll (original)
> +++ llvm/trunk/test/Transforms/GVN/PRE/rle.ll Wed Sep 28 12:57:16 2016
> @@ -318,19 +318,6 @@ define i8 @coerce_offset0(i32 %V, i32* %
> ; CHECK: ret i8
> }
> 
> -define i8 @coerce_offset0_addrspacecast(i32 %V, i32* %P) {
> -  store i32 %V, i32* %P
> -
> -  %P2 = addrspacecast i32* %P to i8 addrspace(1)*
> -  %P3 = getelementptr i8, i8 addrspace(1)* %P2, i32 2
> -
> -  %A = load i8, i8 addrspace(1)* %P3
> -  ret i8 %A
> -; CHECK-LABEL: @coerce_offset0_addrspacecast(
> -; CHECK-NOT: load
> -; CHECK: ret i8
> -}
> -
> ;; non-local i32/float -> i8 load forwarding.
> define i8 @coerce_offset_nonlocal0(i32* %P, i1 %cond) {
>   %P2 = bitcast i32* %P to float*
> 
> Added: llvm/trunk/test/Transforms/GVN/addrspace-cast.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/GVN/addrspace-cast.ll?rev=282612&view=auto
> ==============================================================================
> --- llvm/trunk/test/Transforms/GVN/addrspace-cast.ll (added)
> +++ llvm/trunk/test/Transforms/GVN/addrspace-cast.ll Wed Sep 28 12:57:16 2016
> @@ -0,0 +1,21 @@
> +; RUN: opt < %s -gvn -S | FileCheck %s
> +target datalayout = "e-m:e-p:16:16-p1:32:16-i32:16-i64:16-n8:16"
> +
> +; In cases where two address spaces do not have the same size pointer, the
> +; input for the addrspacecast should not be used as a substitute for itself
> +; when manipulating the pointer.
> +
> +; Check that we don't hit the assert in this scenario
> +define i8 @test(i32 %V, i32* %P) {
> +; CHECK-LABEL: @test(
> +; CHECK: load
> +  %P1 = getelementptr inbounds i32, i32* %P, i16 16
> +
> +  store i32 %V, i32* %P1
> +
> +  %P2 = addrspacecast i32* %P1 to i8 addrspace(1)*
> +  %P3 = getelementptr i8, i8 addrspace(1)* %P2, i32 2
> +
> +  %A = load i8, i8 addrspace(1)* %P3
> +  ret i8 %A
> +}
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org<mailto:llvm-commits at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> 