[llvm] r228556 - InstCombine: propagate nonNull through assume

[Chandler Carruth]

On Sun, Feb 8, 2015 at 5:13 PM, Ramkumar Ramachandra <artagnon at gmail.com>
wrote:

> Author: artagnon
> Date: Sun Feb  8 19:13:13 2015
> New Revision: 228556
>
> URL: http://llvm.org/viewvc/llvm-project?rev=228556&view=rev
> Log:
> InstCombine: propagate nonNull through assume
>
> Make assume (load (call|invoke) != null) set nonNull return attribute
> for the call and invoke. Also include tests.
>
> Differential Revision: http://reviews.llvm.org/D7107
>
> Modified:
>     llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp
>     llvm/trunk/test/Transforms/InstCombine/assume.ll
>
> Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp?rev=228556&r1=228555&r2=228556&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp (original)
> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp Sun Feb  8
> 19:13:13 2015
> @@ -1081,12 +1081,19 @@ Instruction *InstCombiner::visitCallInst
>            cast<Constant>(RHS)->isNullValue()) {
>          LoadInst* LI = cast<LoadInst>(LHS);
>          if (isValidAssumeForContext(II, LI, DL, DT)) {
> +          // assume( load (call|invoke) != null ) -> add 'nonnull' return
> +          // attribute
> +          Value *LIOperand = LI->getOperand(0);
> +          if (CallInst *I = dyn_cast<CallInst>(LIOperand))
> +            I->addAttribute(AttributeSet::ReturnIndex,
> Attribute::NonNull);
> +          else if (InvokeInst *I = dyn_cast<InvokeInst>(LIOperand))
> +            I->addAttribute(AttributeSet::ReturnIndex,
> Attribute::NonNull);
> +
>

This seems really wrong. Consider this test case:

% cat x.ll
target datalayout = "e-m:e-i64:64-i128:128-n32:64-S128"

declare void @llvm.assume(i1 %cond)

declare i8** @g()

define i1 @f(i1 %c) {
entry:
  %ptr1 = call i8** @g()
  br i1 %c, label %left, label %right

left:
  %ptr2 = load i8** %ptr1
  %assume.cond = icmp ne i8* %ptr2, null
  call void @llvm.assume(i1 %assume.cond)
  br label %end

right:
  %cond = icmp ne i8** %ptr1, null
  br label %end

end:
  %ret = phi i1 [ true, %left ], [ %cond, %right ]
  ret i1 %ret
}

We now optimize this to 'ret i1 true', but I don't see any reason why that
should hold.

Fundamentally, you're updating the *load* operand to have the nonnull
attribute, and I don't understand that at all. It seems the wrong thing to
do in two ways:

1) The 'isValidAssumeForContext' call uses the load as the context, not the
call. That means that, as in my example, the load may be control dependent
along with the assume but the call may be somewhere the assumption doesn't
hold at all. So it isn't right to even think about the call at this point.

2) The assume is saying that the *result* of the load is non-null, not
anything about the operand of the load. I mean, clearly, an operand of a
load must dynamically be non-null, but that's not what the assume is about
at all here.

I think to resolve the TODO you seemed to be working on, you need to add a
branch similar to the load path, but which looks specifically for an assume
of an icmp whose operand is a *call* (no load involved at all) and where
the assumption is valid in the context of the call. Then you can apply this
transformation.

I'm reverting this change in the interim as it is quite likely miscompiling
code, and there are multiple miscompiles in the tree right now so we need
to get things back to green. The test case above should be enough for you
to make progress here.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150209/a65c7e18/attachment.html>