[PATCH] SROA: Prevent a cross address space bitcast

[Chandler Carruth]

On Mon, Oct 14, 2013 at 6:58 PM, Tom Stellard <tom at stellard.net> wrote:

> From: Tom Stellard <thomas.stellard at amd.com>
>
> Patch by: Michael Ferguson
>
> This is a modified version of Michael's patch posted here:
> http://www.llvm.org/bugs/show_bug.cgi?id=15907#c1 I removed the bitcast
> wrapper and left only what was necessary to fix the bug.
>

Thanks for cleaning this up and taking the time to mail it out to the list.
A couple of comments:

Can you use Phabricator? Or at least mail as an attachment?


> http://www.llvm.org/bugs/show_bug.cgi?id=15907
> ---
>  lib/Transforms/Scalar/SROA.cpp             |  9 ++++--
>  test/Transforms/SROA/sroa-addrspace-bug.ll | 52
> ++++++++++++++++++++++++++++++
>  2 files changed, 59 insertions(+), 2 deletions(-)
>  create mode 100644 test/Transforms/SROA/sroa-addrspace-bug.ll
>
> diff --git a/lib/Transforms/Scalar/SROA.cpp
> b/lib/Transforms/Scalar/SROA.cpp
> index a0be2c6..3990942 100644
> --- a/lib/Transforms/Scalar/SROA.cpp
> +++ b/lib/Transforms/Scalar/SROA.cpp
> @@ -1416,8 +1416,9 @@ static Value *getAdjustedPtr(IRBuilderTy &IRB, const
> DataLayout &DL,
>
>    if (!OffsetPtr) {
>      if (!Int8Ptr) {
> -      Int8Ptr = IRB.CreateBitCast(Ptr, IRB.getInt8PtrTy(),
> -                                  "raw_cast");
> +      unsigned PtrAS = Ptr->getType()->getPointerAddressSpace();
>

Why only pull this out here?


> +      Int8Ptr = IRB.CreateBitCast(Ptr, IRB.getInt8PtrTy(PtrAS),
> +                                 "raw_cast");
>        Int8PtrOffset = Offset;
>      }
>
> @@ -2485,6 +2486,7 @@ private:
>      // Strip all inbounds GEPs and pointer casts to try to dig out any
> root
>      // alloca that should be re-examined after rewriting this instruction.
>      Value *OtherPtr = IsDest ? II.getRawSource() : II.getRawDest();
> +    unsigned OtherAS = OtherPtr->getType()->getPointerAddressSpace();
>      if (AllocaInst *AI
>            = dyn_cast<AllocaInst>(OtherPtr->stripInBoundsOffsets()))
>        Pass.Worklist.insert(AI);
> @@ -2538,6 +2540,9 @@ private:
>        OtherPtrTy = SubIntTy->getPointerTo();
>      }
>
> +    // Fix OtherPtrTy to be in OtherAS address space.
> +    OtherPtrTy =
> OtherPtrTy->getPointerElementType()->getPointerTo(OtherAS);
> +
>

This seems like a bit of a hack. I feel like this isn't really doing a good
job of fundamentally thinking about the cases where address spaces are
relevant and reasoning about them in a methodical sense... Maybe I'm
missing it. I think I have a suggestion below that will help though.


>      Value *SrcPtr = getAdjustedPtr(IRB, DL, OtherPtr, RelOffset,
> OtherPtrTy);
>      Value *DstPtr = &NewAI;
>      if (!IsDest)
> diff --git a/test/Transforms/SROA/sroa-addrspace-bug.ll
> b/test/Transforms/SROA/sroa-addrspace-bug.ll
> new file mode 100644
> index 0000000..34e8d82
> --- /dev/null
> +++ b/test/Transforms/SROA/sroa-addrspace-bug.ll
> @@ -0,0 +1,52 @@
> +; RUN: opt < %s -sroa -S | FileCheck %s
> +
> +target datalayout =
> "e-p:32:32:32-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-v16:16:16-v24:32:32-v32:32:32-v48:64:64-v64:64:64-v96:128:128-v128:128:128-v192:256:256-v256:256:256-v512:512:512-v1024:1024:1024-v2048:2048:2048-n32:64"
> +target triple = "r600"
> +
> +%struct.Block = type { i32, i32, i32 }
> +
> +; CHECK-NOT: bitcast i8 addrspace(1)* {{%[a-zA-z0-9._]+}} to i32*
>

This is a *really* isolated test case. Have you considered trying to take
some of the existing tests and re-write them in ways that are address space
sensitive to get more comprehensive coverage?

At the very least, I would like to see specific positive tests combined
with a negative test regarding address spaces so it is clear what is
supposed to work, and *where* something is supposed to not happen. As-is,
this test will provide *no* help to a future maintainer trying to
understand why their change regressed it.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131014/9506109d/attachment.html>