[llvm-commits] [llvm] r168361 - in /llvm/trunk: lib/Transforms/Scalar/SROA.cpp test/Transforms/SROA/phi-and-select.ll

[Chandler Carruth]

Pawel, another simple bug fix for 3.2 I think. I'm the code owner for SROA.

On Tue, Nov 20, 2012 at 2:02 AM, Chandler Carruth <chandlerc at gmail.com> wrote:
> Author: chandlerc
> Date: Tue Nov 20 04:02:19 2012
> New Revision: 168361
>
> URL: http://llvm.org/viewvc/llvm-project?rev=168361&view=rev
> Log:
> Fix PR14132 and handle OOB loads speculated throuh PHI nodes.
>
> The issue is that we may end up with newly OOB loads when speculating
> a load into the predecessors of a PHI node, and this confuses the new
> integer splitting logic in some cases, triggering an assertion failure.
> In fact, the branch in question must be dead code as it loads from
> a too-narrow alloca. Add code to handle this gracefully and leave the
> requisite FIXMEs for both optimizing more aggressively and doing more to
> aid sanitizing invalid code which triggers these patterns.
>
> Modified:
>     llvm/trunk/lib/Transforms/Scalar/SROA.cpp
>     llvm/trunk/test/Transforms/SROA/phi-and-select.ll
>
> Modified: llvm/trunk/lib/Transforms/Scalar/SROA.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/SROA.cpp?rev=168361&r1=168360&r2=168361&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Transforms/Scalar/SROA.cpp (original)
> +++ llvm/trunk/lib/Transforms/Scalar/SROA.cpp Tue Nov 20 04:02:19 2012
> @@ -568,6 +568,10 @@
>
>      // Clamp the end offset to the end of the allocation. Note that this is
>      // formulated to handle even the case where "BeginOffset + Size" overflows.
> +    // NOTE! This may appear superficially to be something we could ignore
> +    // entirely, but that is not so! There may be PHI-node uses where some
> +    // instructions are dead but not others. We can't completely ignore the
> +    // PHI node, and so have to record at least the information here.
>      assert(AllocSize >= BeginOffset); // Established above.
>      if (Size > AllocSize - BeginOffset) {
>        DEBUG(dbgs() << "WARNING: Clamping a " << Size << " byte use @" << Offset
> @@ -2492,6 +2496,23 @@
>
>      uint64_t Size = EndOffset - BeginOffset;
>      bool IsSplitIntLoad = Size < TD.getTypeStoreSize(LI.getType());
> +
> +    // If this memory access can be shown to *statically* extend outside the
> +    // bounds of the original allocation it's behavior is undefined. Rather
> +    // than trying to transform it, just replace it with undef.
> +    // FIXME: We should do something more clever for functions being
> +    // instrumented by asan.
> +    // FIXME: Eventually, once ASan and friends can flush out bugs here, this
> +    // should be transformed to a load of null making it unreachable.
> +    uint64_t OldAllocSize = TD.getTypeAllocSize(OldAI.getAllocatedType());
> +    if (TD.getTypeStoreSize(LI.getType()) > OldAllocSize) {
> +      LI.replaceAllUsesWith(UndefValue::get(LI.getType()));
> +      Pass.DeadInsts.insert(&LI);
> +      deleteIfTriviallyDead(OldOp);
> +      DEBUG(dbgs() << "          to: undef!!\n");
> +      return true;
> +    }
> +
>      Type *TargetTy = IsSplitIntLoad ? Type::getIntNTy(LI.getContext(), Size * 8)
>                                      : LI.getType();
>      bool IsPtrAdjusted = false;
>
> Modified: llvm/trunk/test/Transforms/SROA/phi-and-select.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/SROA/phi-and-select.ll?rev=168361&r1=168360&r2=168361&view=diff
> ==============================================================================
> --- llvm/trunk/test/Transforms/SROA/phi-and-select.ll (original)
> +++ llvm/trunk/test/Transforms/SROA/phi-and-select.ll Tue Nov 20 04:02:19 2012
> @@ -390,3 +390,38 @@
>    %tmpcast.d.0 = select i1 undef, i32* %c, i32* %d.0
>    br label %for.cond
>  }
> +
> +define i64 @PR14132(i1 %flag) {
> +; CHECK: @PR14132
> +; Here we form a PHI-node by promoting the pointer alloca first, and then in
> +; order to promote the other two allocas, we speculate the load of the
> +; now-phi-node-pointer. In doing so we end up loading a 64-bit value from an i8
> +; alloca, which is completely bogus. However, we were asserting on trying to
> +; rewrite it. Now it is replaced with undef. Eventually we may replace it with
> +; unrechable and even the CFG will go away here.
> +entry:
> +  %a = alloca i64
> +  %b = alloca i8
> +  %ptr = alloca i64*
> +; CHECK-NOT: alloca
> +
> +  %ptr.cast = bitcast i64** %ptr to i8**
> +  store i64 0, i64* %a
> +  store i8 1, i8* %b
> +  store i64* %a, i64** %ptr
> +  br i1 %flag, label %if.then, label %if.end
> +
> +if.then:
> +  store i8* %b, i8** %ptr.cast
> +  br label %if.end
> +
> +if.end:
> +  %tmp = load i64** %ptr
> +  %result = load i64* %tmp
> +; CHECK-NOT: store
> +; CHECK-NOT: load
> +; CHECK: %[[result:.*]] = phi i64 [ undef, %if.then ], [ 0, %entry ]
> +
> +  ret i64 %result
> +; CHECK-NEXT: ret i64 %[[result]]
> +}
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits