[llvm] r228556 - InstCombine: propagate nonNull through assume

[Hal Finkel]

----- Original Message -----

> From: "Chandler Carruth" <chandlerc at google.com>
> To: "Ramkumar Ramachandra" <artagnon at gmail.com>
> Cc: "Commit Messages and Patches for LLVM" <llvm-commits at cs.uiuc.edu>
> Sent: Tuesday, February 10, 2015 1:52:53 AM
> Subject: Re: [llvm] r228556 - InstCombine: propagate nonNull through
> assume

> 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.
That's correct. Thanks for catching this! 

-Hal 

> 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.
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-- 

Hal Finkel 
Assistant Computational Scientist 
Leadership Computing Facility 
Argonne National Laboratory 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150210/7d85c92c/attachment.html>