<html><head><style type='text/css'>p { margin: 0; }</style></head><body><div style='font-family: arial,helvetica,sans-serif; font-size: 10pt; color: #000000'><br><hr id="zwchr"><blockquote style="border-left: 2px solid rgb(16, 16, 255); margin-left: 5px; padding-left: 5px; color: rgb(0, 0, 0); font-weight: normal; font-style: normal; text-decoration: none; font-family: Helvetica,Arial,sans-serif; font-size: 12pt;"><b>From: </b>"Chandler Carruth" <chandlerc@google.com><br><b>To: </b>"Ramkumar Ramachandra" <artagnon@gmail.com><br><b>Cc: </b>"Commit Messages and Patches for LLVM" <llvm-commits@cs.uiuc.edu><br><b>Sent: </b>Tuesday, February 10, 2015 1:52:53 AM<br><b>Subject: </b>Re: [llvm] r228556 - InstCombine: propagate nonNull through assume<br><br><div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Feb 8, 2015 at 5:13 PM, Ramkumar Ramachandra <span dir="ltr"><<a href="mailto:artagnon@gmail.com" target="_blank">artagnon@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div id=":ajl" class="" style="overflow: hidden;">Author: artagnon<br>
Date: Sun Feb 8 19:13:13 2015<br>
New Revision: 228556<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=228556&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=228556&view=rev</a><br>
Log:<br>
InstCombine: propagate nonNull through assume<br>
<br>
Make assume (load (call|invoke) != null) set nonNull return attribute<br>
for the call and invoke. Also include tests.<br>
<br>
Differential Revision: <a href="http://reviews.llvm.org/D7107" target="_blank">http://reviews.llvm.org/D7107</a><br>
<br>
Modified:<br>
llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp<br>
llvm/trunk/test/Transforms/InstCombine/assume.ll<br>
<br>
Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp?rev=228556&r1=228555&r2=228556&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp?rev=228556&r1=228555&r2=228556&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp (original)<br>
+++ llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp Sun Feb 8 19:13:13 2015<br>
@@ -1081,12 +1081,19 @@ Instruction *InstCombiner::visitCallInst<br>
cast<Constant>(RHS)->isNullValue()) {<br>
LoadInst* LI = cast<LoadInst>(LHS);<br>
if (isValidAssumeForContext(II, LI, DL, DT)) {<br>
+ // assume( load (call|invoke) != null ) -> add 'nonnull' return<br>
+ // attribute<br>
+ Value *LIOperand = LI->getOperand(0);<br>
+ if (CallInst *I = dyn_cast<CallInst>(LIOperand))<br>
+ I->addAttribute(AttributeSet::ReturnIndex, Attribute::NonNull);<br>
+ else if (InvokeInst *I = dyn_cast<InvokeInst>(LIOperand))<br>
+ I->addAttribute(AttributeSet::ReturnIndex, Attribute::NonNull);<br>
+</div></blockquote></div><br>This seems really wrong. Consider this test case:</div><div class="gmail_extra"><br></div><div class="gmail_extra"><div class="gmail_extra">% cat x.ll</div><div class="gmail_extra">target datalayout = "e-m:e-i64:64-i128:128-n32:64-S128"</div><div class="gmail_extra"><br></div><div class="gmail_extra">declare void @llvm.assume(i1 %cond)</div><div class="gmail_extra"><br></div><div class="gmail_extra">declare i8** @g()</div><div class="gmail_extra"><br></div><div class="gmail_extra">define i1 @f(i1 %c) {</div><div class="gmail_extra">entry:</div><div class="gmail_extra"> %ptr1 = call i8** @g()</div><div class="gmail_extra"> br i1 %c, label %left, label %right</div><div class="gmail_extra"><br></div><div class="gmail_extra">left:</div><div class="gmail_extra"> %ptr2 = load i8** %ptr1</div><div class="gmail_extra"> %assume.cond = icmp ne i8* %ptr2, null</div><div class="gmail_extra"> call void @llvm.assume(i1 %assume.cond)</div><div class="gmail_extra"> br label %end</div><div class="gmail_extra"><br></div><div class="gmail_extra">right:</div><div class="gmail_extra"> %cond = icmp ne i8** %ptr1, null</div><div class="gmail_extra"> br label %end</div><div class="gmail_extra"><br></div><div class="gmail_extra">end:</div><div class="gmail_extra"> %ret = phi i1 [ true, %left ], [ %cond, %right ]</div><div class="gmail_extra"> ret i1 %ret</div><div class="gmail_extra">}</div><div class="gmail_extra"><br></div><div class="gmail_extra">We now optimize this to 'ret i1 true', but I don't see any reason why that should hold.</div><div class="gmail_extra"><br></div><div class="gmail_extra">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:</div><div class="gmail_extra"><br></div><div class="gmail_extra">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.</div><div class="gmail_extra"><br></div><div class="gmail_extra">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.</div><div class="gmail_extra"><br></div><div id="DWT5347" class="gmail_extra">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.</div></div></div></blockquote><br>That's correct. Thanks for catching this!<br><br> -Hal<br><br><blockquote style="border-left: 2px solid rgb(16, 16, 255); margin-left: 5px; padding-left: 5px; color: rgb(0, 0, 0); font-weight: normal; font-style: normal; text-decoration: none; font-family: Helvetica,Arial,sans-serif; font-size: 12pt;"><div dir="ltr"><div class="gmail_extra"><div class="gmail_extra"></div><div class="gmail_extra"><br></div><div class="gmail_extra">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.</div></div></div>
<br>_______________________________________________<br>llvm-commits mailing list<br>llvm-commits@cs.uiuc.edu<br>http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits<br></blockquote><br><br><br>-- <br><div><span name="x"></span>Hal Finkel<br>Assistant Computational Scientist<br>Leadership Computing Facility<br>Argonne National Laboratory<span name="x"></span><br></div></div></body></html>