<div dir="ltr">Thanks for the responses everyone.</div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Nov 11, 2014 at 4:45 PM, Hal Finkel <span dir="ltr"><<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">----- Original Message -----<br>
> From: "Sean Silva" <<a href="mailto:chisophugis@gmail.com">chisophugis@gmail.com</a>><br>
> To: "Hal Finkel" <<a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a>><br>
> Cc: "David Blaikie" <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>>, <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> Sent: Tuesday, November 11, 2014 6:35:40 PM<br>
> Subject: Re: [llvm] r221737 - Canonicalize an assume(load != null) into !nonnull metadata<br>
><br>
> On Tue, Nov 11, 2014 at 4:15 PM, Hal Finkel < <a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a> ><br>
> wrote:<br>
><br>
> ----- Original Message -----<br>
> > From: "David Blaikie" < <a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a> ><br>
> > To: "Philip Reames" < <a href="mailto:listmail@philipreames.com">listmail@philipreames.com</a> ><br>
> > Cc: <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> > Sent: Tuesday, November 11, 2014 5:47:28 PM<br>
> > Subject: Re: [llvm] r221737 - Canonicalize an assume(load != null)<br>
> > into !nonnull metadata<br>
> ><br>
> > On Tue, Nov 11, 2014 at 3:33 PM, Philip Reames <<br>
> > <a href="mailto:listmail@philipreames.com">listmail@philipreames.com</a> > wrote:<br>
> ><br>
> ><br>
> > Author: reames<br>
> > Date: Tue Nov 11 17:33:19 2014<br>
> > New Revision: 221737<br>
> ><br>
> > URL: <a href="http://llvm.org/viewvc/llvm-project?rev=221737&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=221737&view=rev</a><br>
> > Log:<br>
> > Canonicalize an assume(load != null) into !nonnull metadata<br>
> ><br>
> > We currently have two ways of informing the optimizer that the<br>
> > result<br>
> > of a load is never null: metadata and assume. This change converts<br>
> > the second in to the former. This avoids a need to implement<br>
> > optimizations using both forms.<br>
> ><br>
> > We should probably extend this basic idea to metadata of other<br>
> > forms;<br>
> > in particular, range metadata. We view is that assumes should be<br>
> > considered a "last resort" for when there isn't a more canonical<br>
> > way<br>
> > to represent something.<br>
> ><br>
> ><br>
> > (naively) seems almost a pity, really - if we have the general<br>
> > tool,<br>
> > why not drop the special cases & just use the general one? For<br>
> > things already using the special cases, they can continue to look<br>
> > for the very specific canonicalized case of the general form with<br>
> > relatively low cost, I would imagine - but I know very little about<br>
> > all of this. Just a thought.<br>
> ><br>
><br>
> I share your desire for making use of a single general-purpose tool,<br>
> but in this case, using @llvm.assume as a much higher cost compared<br>
> to the metadata (both in its representational size and in its<br>
> ability to block potentially-useful optimizations),<br>
><br>
><br>
> How does it block optimizations? That's sort of saddening.<br>
<br>
</div></div>There are essentially three ways:<br>
<br>
 1. By adding uses on the values on which assumptions are being placed (hasOneUse no longer returns true).<br>
<br>
 2. By blocking code motion (I've tried to adjust for this by placing special knowledge inside of BasicAA, EarlyCSE, LoopVectorize, etc. -- but I can't say I've optimally gotten everything).<br>
<br>
 3. By keeping otherwise dead blocks alive (we could have a side of a control-flow diamond with nothing but an assumption) -- hopefully the MI-level passes will clean up properly if this happens (and they normally do), but it could still block otherwise useful IR-level optimizations.<br>
<br>
The general philosophy behind using @llvm.assume is: Use it only when you're fairly certain that the information you're providing is so important that it is worth preserving that information accounting for the fact that the preservation has priority over optimization.<br>
<span class="HOEnZb"><font color="#888888"><br>
 -Hal<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
><br>
><br>
> -- Sean Silva<br>
><br>
><br>
><br>
><br>
> so I think it makes sense to canonicalize on the metadata when<br>
> available.<br>
><br>
> -Hal<br>
><br>
><br>
><br>
> ><br>
> ><br>
> > Reviewed by: Hal<br>
> > Differential Revision: <a href="http://reviews.llvm.org/D5951" target="_blank">http://reviews.llvm.org/D5951</a><br>
> ><br>
> ><br>
> > Modified:<br>
> > llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp<br>
> > llvm/trunk/test/Transforms/InstCombine/assume.ll<br>
> ><br>
> > Modified:<br>
> > llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp<br>
> > URL:<br>
> > <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp?rev=221737&r1=221736&r2=221737&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp?rev=221737&r1=221736&r2=221737&view=diff</a><br>
> > ==============================================================================<br>
> > --- llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp<br>
> > (original)<br>
> > +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp Tue<br>
> > Nov 11 17:33:19 2014<br>
> > @@ -1102,6 +1102,26 @@ Instruction *InstCombiner::visitCallInst<br>
> > return EraseInstFromFunction(*II);<br>
> > }<br>
> ><br>
> > + // assume( (load addr) != null ) -> add 'nonnull' metadata to<br>
> > load<br>
> > + // (if assume is valid at the load)<br>
> > + if (ICmpInst* ICmp = dyn_cast<ICmpInst>(IIOperand)) {<br>
> > + Value *LHS = ICmp->getOperand(0);<br>
> > + Value *RHS = ICmp->getOperand(1);<br>
> > + if (ICmpInst::ICMP_NE == ICmp->getPredicate() &&<br>
> > + isa<LoadInst>(LHS) &&<br>
> > + isa<Constant>(RHS) &&<br>
> > + RHS->getType()->isPointerTy() &&<br>
> > + cast<Constant>(RHS)->isNullValue()) {<br>
> > + LoadInst* LI = cast<LoadInst>(LHS);<br>
> > + if (isValidAssumeForContext(II, LI, DL, DT)) {<br>
> > + MDNode* MD = MDNode::get(II->getContext(), ArrayRef<Value*>());<br>
> > + LI->setMetadata(LLVMContext::MD_nonnull, MD);<br>
> > + return EraseInstFromFunction(*II);<br>
> > + }<br>
> > + }<br>
> > + // TODO: apply nonnull return attributes to calls and invokes<br>
> > + // TODO: apply range metadata for range check patterns?<br>
> > + }<br>
> > // If there is a dominating assume with the same condition as this<br>
> > one,<br>
> > // then this one is redundant, and should be removed.<br>
> > APInt KnownZero(1, 0), KnownOne(1, 0);<br>
> ><br>
> > Modified: llvm/trunk/test/Transforms/InstCombine/assume.ll<br>
> > URL:<br>
> > <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/assume.ll?rev=221737&r1=221736&r2=221737&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/assume.ll?rev=221737&r1=221736&r2=221737&view=diff</a><br>
> > ==============================================================================<br>
> > --- llvm/trunk/test/Transforms/InstCombine/assume.ll (original)<br>
> > +++ llvm/trunk/test/Transforms/InstCombine/assume.ll Tue Nov 11<br>
> > 17:33:19 2014<br>
> > @@ -186,6 +186,80 @@ entry:<br>
> > ; CHECK: ret i32 0<br>
> > }<br>
> ><br>
> > +declare void @escape(i32* %a)<br>
> > +<br>
> > +; Do we canonicalize a nonnull assumption on a load into<br>
> > +; metadata form?<br>
> > +define i1 @nonnull1(i32** %a) {<br>
> > +entry:<br>
> > + %load = load i32** %a<br>
> > + %cmp = icmp ne i32* %load, null<br>
> > + tail call void @llvm.assume(i1 %cmp)<br>
> > + tail call void @escape(i32* %load)<br>
> > + %rval = icmp eq i32* %load, null<br>
> > + ret i1 %rval<br>
> > +<br>
> > +; CHECK-LABEL: @nonnull1<br>
> > +; CHECK: !nonnull<br>
> > +; CHECK-NOT: call void @llvm.assume<br>
> > +; CHECK: ret i1 false<br>
> > +}<br>
> > +<br>
> > +; Make sure the above canonicalization applies only<br>
> > +; to pointer types. Doing otherwise would be illegal.<br>
> > +define i1 @nonnull2(i32* %a) {<br>
> > +entry:<br>
> > + %load = load i32* %a<br>
> > + %cmp = icmp ne i32 %load, 0<br>
> > + tail call void @llvm.assume(i1 %cmp)<br>
> > + %rval = icmp eq i32 %load, 0<br>
> > + ret i1 %rval<br>
> > +<br>
> > +; CHECK-LABEL: @nonnull2<br>
> > +; CHECK-NOT: !nonnull<br>
> > +; CHECK: call void @llvm.assume<br>
> > +}<br>
> > +<br>
> > +; Make sure the above canonicalization does not trigger<br>
> > +; if the assume is control dependent on something else<br>
> > +define i1 @nonnull3(i32** %a, i1 %control) {<br>
> > +entry:<br>
> > + %load = load i32** %a<br>
> > + %cmp = icmp ne i32* %load, null<br>
> > + br i1 %control, label %taken, label %not_taken<br>
> > +taken:<br>
> > + tail call void @llvm.assume(i1 %cmp)<br>
> > + %rval = icmp eq i32* %load, null<br>
> > + ret i1 %rval<br>
> > +not_taken:<br>
> > + ret i1 true<br>
> > +<br>
> > +; CHECK-LABEL: @nonnull3<br>
> > +; CHECK-NOT: !nonnull<br>
> > +; CHECK: call void @llvm.assume<br>
> > +}<br>
> > +<br>
> > +; Make sure the above canonicalization does not trigger<br>
> > +; if the path from the load to the assume is potentially<br>
> > +; interrupted by an exception being thrown<br>
> > +define i1 @nonnull4(i32** %a) {<br>
> > +entry:<br>
> > + %load = load i32** %a<br>
> > + ;; This call may throw!<br>
> > + tail call void @escape(i32* %load)<br>
> > + %cmp = icmp ne i32* %load, null<br>
> > + tail call void @llvm.assume(i1 %cmp)<br>
> > + %rval = icmp eq i32* %load, null<br>
> > + ret i1 %rval<br>
> > +<br>
> > +; CHECK-LABEL: @nonnull4<br>
> > +; CHECK-NOT: !nonnull<br>
> > +; CHECK: call void @llvm.assume<br>
> > +}<br>
> > +<br>
> > +<br>
> > +<br>
> > +<br>
> > attributes #0 = { nounwind uwtable }<br>
> > attributes #1 = { nounwind }<br>
> ><br>
> ><br>
> ><br>
> > _______________________________________________<br>
> > llvm-commits mailing list<br>
> > <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
> ><br>
> ><br>
> > _______________________________________________<br>
> > llvm-commits mailing list<br>
> > <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
> ><br>
><br>
> --<br>
> Hal Finkel<br>
> Assistant Computational Scientist<br>
> Leadership Computing Facility<br>
> Argonne National Laboratory<br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
><br>
><br>
<br>
--<br>
Hal Finkel<br>
Assistant Computational Scientist<br>
Leadership Computing Facility<br>
Argonne National Laboratory<br>
</div></div></blockquote></div><br></div>