[PATCH] Fix Bug in SROA transformation [PR18615]

[Chandler Carruth]

Doh, really sorry.

Some one pointed me at the bug, and I didn't realize this review thread was
the *same* PR. I ended up fixing this in r202224 by completely removing the
check rather than fixing my buggy boolean logic. I also committed a
somewhat further reduced test case.

Very sorry for stealing your bug, and thanks for digging through all of the
layers here! I know SROA can be challenging to hack on.


On Mon, Feb 24, 2014 at 10:20 PM, Karthik Bhat <kv.bhat at samsung.com> wrote:

>   Hi Chandler,
>   Could you please have a look at the patch for final approval? Please let
> me know your inputs on the patch and if this is good to commit? This fixes
> PR18615.
>   Thanks to Rafael once again for reviewing the patch. Updated the test
> case as per Rafael's suggesion.
>
>   Regards
>   Karthik Bhat
>
> Hi chandlerc, rafael,
>
> http://llvm-reviews.chandlerc.com/D2759
>
> CHANGE SINCE LAST DIFF
>   http://llvm-reviews.chandlerc.com/D2759?vs=7303&id=7340#toc
>
> Files:
>   test/Transforms/SROA/pr18615.ll
>   lib/Transforms/Scalar/SROA.cpp
>
> Index: test/Transforms/SROA/pr18615.ll
> ===================================================================
> --- test/Transforms/SROA/pr18615.ll
> +++ test/Transforms/SROA/pr18615.ll
> @@ -0,0 +1,23 @@
> +; In this test case we are just checking that sroa transformation
> +; doesn't result in crash as reported in PR18615 hence disabling output.
> +
> +; RUN: opt < %s -early-cse -instcombine -sroa -disable-output -S
> +; RUN: opt < %s -early-cse -sroa -disable-output -S
> +
> +target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
> +
> +%struct.S0 = type { i32, i32, i32 }
> +
> +define void @fn1() {
> +entry:
> +  %b = alloca i32, align 4
> +  %f = alloca [1 x %struct.S0], align 4
> +  %arrayidx = getelementptr inbounds [1 x %struct.S0]* %f, i32 0, i64 0
> +  %arrayidx1 = getelementptr inbounds [1 x %struct.S0]* %f, i32 0, i64 -1
> +  %tmp1 = bitcast %struct.S0* %arrayidx to i8*
> +  %tmp2 = bitcast %struct.S0* %arrayidx1 to i8*
> +  call void @llvm.memcpy.p0i8.p0i8.i64(i8* %tmp1, i8* %tmp2, i64 12, i32
> 4, i1 false)
> +  ret void
> +}
> +
> +declare void @llvm.memcpy.p0i8.p0i8.i64(i8* nocapture, i8* nocapture
> readonly, i64, i32, i1)
> Index: lib/Transforms/Scalar/SROA.cpp
> ===================================================================
> --- lib/Transforms/Scalar/SROA.cpp
> +++ lib/Transforms/Scalar/SROA.cpp
> @@ -473,12 +473,13 @@
>      if (!IsOffsetKnown)
>        return PI.setAborted(&II);
>
> -    // This side of the transfer is completely out-of-bounds, and so we
> can
> +    // This side of the transfer is completely out-of-bounds
> +    // (i.e. Offest < 0 or Offset >= AllocSize), and so we can
>      // nuke the entire transfer. However, we also need to nuke the other
> side
>      // if already added to our partitions.
>      // FIXME: Yet another place we really should bypass this when
>      // instrumenting for ASan.
> -    if (!Offset.isNegative() && Offset.uge(AllocSize)) {
> +    if (Offset.isNegative() || Offset.uge(AllocSize)) {
>        SmallDenseMap<Instruction *, unsigned>::iterator MTPI =
> MemTransferSliceMap.find(&II);
>        if (MTPI != MemTransferSliceMap.end())
>          S.Slices[MTPI->second].kill();
>
> _______________________________________________
> 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/20140225/a83978ee/attachment.html>