<div style="font-family: arial, helvetica, sans-serif; font-size: 10pt"><div dir="ltr"><div class="gmail_default" style>+// Move the call to free before a NULL test.<br></div><div class="gmail_default" style><div class="gmail_default">
<br></div><div class="gmail_default" style>Make this comment block use the doxygen format?</div><div class="gmail_default" style><br></div><div class="gmail_default" style><div class="gmail_default">+ // If equal, it must reach the call to free by the false branch.</div>
<div class="gmail_default">+ if ((Condition->getPredicate() == ICmpInst::ICMP_EQ && !FreeBBIsTrue) ||</div><div class="gmail_default">+ // If not equal, by the true branch</div><div class="gmail_default">
+ (Condition->getPredicate() == ICmpInst::ICMP_NE && FreeBBIsTrue)) {</div><div class="gmail_default">+ Value *FirstCondOp = Condition->getOperand(0);</div><div class="gmail_default">+ Value *SecondCondOp = Condition->getOperand(1);</div>
<div class="gmail_default">+ Value *NullPtr =</div><div class="gmail_default">+ ConstantPointerNull::get(dyn_cast<PointerType>(Op->getType()));</div><div class="gmail_default">+ // Does the condition checks NULL against the argument of Free</div>
<div class="gmail_default">+ if ((Op == FirstCondOp && NullPtr == SecondCondOp) ||</div><div class="gmail_default">+ (Op == SecondCondOp && NullPtr == FirstCondOp)) {</div><div class="gmail_default">
+ // Move the call to free in the predecessor</div><div class="gmail_default">+ FI.moveBefore(PredBBBr);</div><div class="gmail_default">+ return &FI;</div><div class="gmail_default">+ }</div><div class="gmail_default">
+ }</div><div class="gmail_default"><br></div><div class="gmail_default" style>This is still a nested condition. I think you can simplify it by inverting and returning early. However, the high-order bit is that you can simplify this sequence (and maybe some of the other code here) by relying on the canonicalization strategy and using pattern matchers.</div>
<div class="gmail_default" style><br></div><div class="gmail_default" style>For example, instcombine will canonicalize 'null' to the RHS of the icmp, so only test for it there. Also, if it doesn't already happen, you can teach instcombine to canonicalize either ICMP_EQ or ICMP_NE by reversing the branch destinations. Then this whole thing can become a single pattern match for an icmp between Op and a constant null pointer. The fact that it simplifies your combinatorial testing of patterns here is the primary motivation behind canonicalization in instcombine.</div>
</div></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Jan 2, 2013 at 3:56 PM, Quentin Colombet <span dir="ltr"><<a href="mailto:qcolombet@apple.com" target="_blank">qcolombet@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">Hi Chandler,<div><br></div><div>Thanks for the very detailed review!</div><div><br></div>
<div>I've updated the patch accordingly, except for the name of the helper function: tryToMoveFreeBeforeNullTest. The name is not better but it is more in line with what the function is actually doing (the removal of the null test is an expected side effect of that transformation).</div>
<div><br></div><div>I let you have a look to the patch to check if I miss anything.</div><div><div><div style="text-indent:0px;letter-spacing:normal;font-variant:normal;text-align:-webkit-auto;font-style:normal;font-weight:normal;line-height:normal;text-transform:none;font-size:medium;white-space:normal;font-family:Helvetica;word-wrap:break-word;word-spacing:0px">
<br></div><div style="text-indent:0px;letter-spacing:normal;font-variant:normal;text-align:-webkit-auto;font-style:normal;font-weight:normal;line-height:normal;text-transform:none;font-size:medium;white-space:normal;font-family:Helvetica;word-wrap:break-word;word-spacing:0px">
Thank you,</div><div style="text-indent:0px;letter-spacing:normal;font-variant:normal;text-align:-webkit-auto;font-style:normal;font-weight:normal;line-height:normal;text-transform:none;font-size:medium;white-space:normal;font-family:Helvetica;word-wrap:break-word;word-spacing:0px">
<br></div><div style="text-indent:0px;letter-spacing:normal;font-variant:normal;text-align:-webkit-auto;font-style:normal;font-weight:normal;line-height:normal;text-transform:none;font-size:medium;white-space:normal;font-family:Helvetica;word-wrap:break-word;word-spacing:0px">
-Quentin</div><div style="text-indent:0px;letter-spacing:normal;font-variant:normal;text-align:-webkit-auto;font-style:normal;font-weight:normal;line-height:normal;text-transform:none;font-size:medium;white-space:normal;font-family:Helvetica;word-wrap:break-word;word-spacing:0px">
<br></div><div style="text-indent:0px;letter-spacing:normal;font-variant:normal;text-align:-webkit-auto;font-style:normal;font-weight:normal;line-height:normal;text-transform:none;font-size:medium;white-space:normal;font-family:Helvetica;word-wrap:break-word;word-spacing:0px">
</div></div></div></div><br><div style="word-wrap:break-word"><div><div><div style="text-indent:0px;letter-spacing:normal;font-variant:normal;text-align:-webkit-auto;font-style:normal;font-weight:normal;line-height:normal;text-transform:none;font-size:medium;white-space:normal;font-family:Helvetica;word-wrap:break-word;word-spacing:0px">
</div>
</div>
<br><div><div>On Jan 2, 2013, at 1:16 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>> wrote:</div><br><blockquote type="cite"><div style="font-family:arial,helvetica,sans-serif;font-size:10pt">
<div dir="ltr"><div class="gmail_default">Cool, thanks.</div><div class="gmail_default"><br></div><div class="gmail_default"><div class="gmail_default">
+ // Check if this free is accessed after its argument has been test</div><div class="gmail_default">+ // against NULL (property 0).</div><div class="gmail_default">+ // If yes, it is legal to move this call in its predecessor block.</div>
<div class="gmail_default">+ // Currently, we consider that it is profitable only if the block</div><div class="gmail_default">+ // containing the call to free will be removed, i.e.:</div><div class="gmail_default">+ // 1. it has only one predecessor P, and P has two successors</div>
<div class="gmail_default">+ // 2. it contains the call and an unconditional branch</div><div class="gmail_default">+ // 3. its successor is the same as its predecessor's successor</div><div class="gmail_default">+ // FIXME: Could handle more general cases but we need a cost model</div>
<div class="gmail_default"><br></div><div class="gmail_default">I think this is no longer entirely accurate -- we have a cost model in that we assume this will slow code down and only do it for size. I would just comment that this should only be done as a space saving optimization when performance isn't important.</div>
<div class="gmail_default"><br></div><div class="gmail_default">You might also add the obvious next step if anyone wants to take it of allowing this transform if the remaining instructions are safe to speculate. It should still be the right choice for -Oz no matter how many instructions we speculate.</div>
<div class="gmail_default"><br></div><div class="gmail_default"><br></div><div class="gmail_default"><div class="gmail_default">+ // 1. Only one predecessor with 2 successors</div><div class="gmail_default">
<br></div><div class="gmail_default">It took me a moment to realize what '1.' is here... Maybe "Validate constraint #1:"?</div><div class="gmail_default"><br></div><div class="gmail_default">+ if (!PredBB ||</div>
<div class="gmail_default">+ // Check part of 0., i.e., is the terminator inst a branch?</div><div class="gmail_default">+ // If not, a fortiori, the condition cannot be a test against NULL</div><div class="gmail_default">
+ !(PredBBBr = dyn_cast<BranchInst>(PredBB->getTerminator())) ||</div><div class="gmail_default">+ PredBBBr->getNumSuccessors() != 2) </div><div class="gmail_default">+ return 0;</div><div class="gmail_default">
<br></div><div class="gmail_default">I think this wil lbe much easier to read if you break it out into more early returning conditions:</div><div class="gmail_default"><br></div><div class="gmail_default">
if (!PredBB)</div><div class="gmail_default"> return 0;</div><div class="gmail_default"><br></div><div class="gmail_default">BranchInst *PrebBBBr = dyn_cast<BranchInst>(PredBB->getTerminator());</div>
<div class="gmail_default">if (!PredBBBr || PredBBr->getNumSuccessors() != 2)</div><div class="gmail_default"> return 0;</div><div class="gmail_default"><br></div><div class="gmail_default">You can also simplify the assert code with this pattern.</div>
<div class="gmail_default"><br></div><div class="gmail_default"><div class="gmail_default">+ if (FreeInstrBB->size() == 2 &&</div><div class="gmail_default">+ (FIBBBr = dyn_cast<BranchInst>(FreeInstrBB->getTerminator())) &&</div>
<div class="gmail_default">+ FIBBBr->isUnconditional() &&</div><div class="gmail_default">+ // 3. Same successor</div><div class="gmail_default">+ FIBBBr->getSuccessor(0) == SuccBB &&</div>
<div class="gmail_default">+ // 0. Does the condition is an equal or not equal to something.</div><div class="gmail_default">+ (Condition = dyn_cast<ICmpInst>(PredBBBr->getCondition())) &&</div>
<div class="gmail_default">+ // If equal, it must get reach the call to free by the false branch.</div><div class="gmail_default">+ ((Condition->getPredicate() == ICmpInst::ICMP_EQ && !FreeBBIsTrue) ||</div>
<div class="gmail_default">+ // If not equal, by the true branch</div><div class="gmail_default">+ (Condition->getPredicate() == ICmpInst::ICMP_NE &&</div><div class="gmail_default">+ FreeBBIsTrue))) {</div>
<div class="gmail_default"><br></div><div class="gmail_default">Same as above. Please use early return instead here. In general, any time you have an assignment in a condition, you probably want an early return on the prior condition.</div>
<div class="gmail_default"><br></div><div class="gmail_default">Maybe hoist this whole thing into a helper function?</div><div class="gmail_default"><br></div><div class="gmail_default">if (MinimizeSize)</div>
<div class="gmail_default"> if (Instruction *I = tryToRemoveNullTestBeforeFree(BI))</div><div class="gmail_default"> return I;</div><div class="gmail_default"><br></div></div></div></div><div class="gmail_extra">
My name is terrible, maybe you can think of a better one.</div><div class="gmail_extra"><br></div><div class="gmail_extra">The nice thing about this is that then if it doesn't fire, and someone adds a new optimization later in visitFree, that new one still has a chance.<br>
<br><div class="gmail_quote">On Wed, Jan 2, 2013 at 11:42 AM, Quentin Colombet <span dir="ltr"><<a href="mailto:qcolombet@apple.com" target="_blank">qcolombet@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div>Hi Chandler,</div><div><br></div><div>Thanks for the comments.</div><div><br></div><div>Like you pointed out, the patch can really hurt the performance in some cases.</div><div><br>
</div>
<div>Hence, I've changed it so that the transformation is done only when Oz (i.e., MinSize function attribute) is set on the caller function.</div><div><br></div><div>New patch attached.</div><span><font color="#888888"><br>
<div>
<div style="text-indent:0px;letter-spacing:normal;font-variant:normal;text-align:-webkit-auto;font-style:normal;font-weight:normal;line-height:normal;text-transform:none;font-size:medium;white-space:normal;font-family:Helvetica;word-wrap:break-word;word-spacing:0px">
-Quentin</div>
</div>
<div><br></div><div></div></font></span></div>
<br><div style="word-wrap:break-word"><div></div><br><div><div>On Jan 2, 2013, at 11:15 AM, Chandler Carruth <<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>> wrote:</div>
<br><blockquote type="cite"><div style="font-family:arial,helvetica,sans-serif;font-size:10pt"><div dir="ltr"><div>I really do share Nick's concern.</div><div><br></div><div>
I would be fine doing this in -Oz mode, where hurting performance for code size is the goal, but outside of that I'm deeply skeptical that we have enough information to make the right decision here.</div><div>
<br></div><div>There are code patterns where the pointer is almost-always null. Imagine a sparsely populated vector of pointers. In this case, there will be a hot loop where you are turning a highly predictable branch into a function call.</div>
<div><br></div><div>We could use various "hotness" heuristics from the BlockFrequencyInfo to control this optimization, but honestly I think the only one that we won't find to pessimize real world code is "very cold" relative to the function entry block.</div>
<div><br></div><div class="gmail_extra"><div class="gmail_quote">On Wed, Jan 2, 2013 at 10:18 AM, Quentin Colombet <span dir="ltr"><<a href="mailto:qcolombet@apple.com" target="_blank">qcolombet@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">Ping!<span><font color="#888888"><br><div>
<div style="text-indent:0px;letter-spacing:normal;font-variant:normal;text-align:-webkit-auto;font-style:normal;font-weight:normal;line-height:normal;text-transform:none;font-size:medium;white-space:normal;font-family:Helvetica;word-wrap:break-word;word-spacing:0px">
-Quentin</div>
</div>
<div><br></div><div></div></font></span></div>
<br><div style="word-wrap:break-word"><div></div><br><div><div>On Dec 21, 2012, at 4:04 PM, Quentin Colombet <<a href="mailto:qcolombet@apple.com" target="_blank">qcolombet@apple.com</a>> wrote:</div>
<br><blockquote type="cite">Hi Nick,<br><br>On Dec 21, 2012, at 3:59 PM, Dmitri Gribenko <<a href="mailto:gribozavr@gmail.com" target="_blank">gribozavr@gmail.com</a>> wrote:<br><br><blockquote type="cite">
On Sat, Dec 22, 2012 at 1:53 AM, Nick Lewycky <<a href="mailto:nicholas@mxc.ca" target="_blank">nicholas@mxc.ca</a>> wrote:<br><blockquote type="cite">On 12/21/2012 03:43 PM, Quentin Colombet wrote:<br>
<blockquote type="cite"><br>Hi,<br><br>Here is a patch to help llvm turning a code like this:<br>if (foo)<br>free(foo)<br><br>into that:<br>free(foo)<br><br>It is legal and safe, since free is already checking its argument<br>
against NULL internally (I may find the reference in the Standard if<br>needed :)).<br></blockquote><br>It's legal and safe, but is it desirable? Calling a function is expensive<br>even if the function doesn't do anything.<br>
</blockquote></blockquote><br>Yes, you're right, it may not be desirable but I'm tempt to think like Dmitri.<br><br>Do you think that this optimization should be controlled by a flag?<br>At least, for code size (Oz and maybe Os), it may always be desirable.<br>
<br>I did not consider changing free(foo) into if(foo) free(foo)<br><br><blockquote type="cite"><br>That's true, but there's another viewpoint: programmers usually write<br>'if(!foo) free(foo);' not because they are manually inlining the fast<br>
path of free(), but because they think that free(NULL) is UB.<br><br>Dmitri<br><br>-- <br>main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if<br>(j){printf("%d\n",i);}}} /*Dmitri Gribenko <<a href="mailto:gribozavr@gmail.com" target="_blank">gribozavr@gmail.com</a>>*/<br>
</blockquote><br>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">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>
</blockquote></div><br></div><br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">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></blockquote></div><br></div></div></div>
</blockquote></div><br></div>
<br></blockquote></div><br></div></div></div>
</blockquote></div><br></div></div>
<br></blockquote></div><br></div></div>