<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">Hi Chandler,<div><br></div><div>Thanks for the proposed modified patch. It looks great.</div><div><br></div><div>By the way, thanks for the pattern matcher style code, I did not know llvm has such a thing.</div><div><br></div><div>To answer your question, it is useful to perform the transformation even if the icmp has more than one use.</div><div>Indeed, it will allow to remove two branch instructions (the conditional AND the unconditional one) and one basic block.</div><div><br></div><div>Should I commit your modified patch? (When possible :))</div><div><br></div><div>-Quentin</div><div><br></div><div><div><div>On Jan 3, 2013, at 3:56 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com">chandlerc@google.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div dir="ltr"><div class="gmail_extra">On Thu, Jan 3, 2013 at 11:04 AM, Quentin Colombet <span dir="ltr"><<a href="mailto:qcolombet@apple.com" target="_blank" class="cremed">qcolombet@apple.com</a>></span> wrote:<br>
<div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word">
Hi Chandler,<div><br></div><div>Here is the new patch that takes advantage of canonicalization previously done.</div><div>Note, however, that we cannot simplify the EQ, NE test, since the canonicalization does not occur if the result of the comparison is used more than once.</div>
</div></blockquote><div><br></div><div style="">Hrm, annoying, but yea I understand. I wonder whether this is the right call, or whether we should canonicalize to EQ, insert a logical not for the other users.</div><div style="">
<br></div><div style="">Actually, backing up a step, is this a useful transform if the icmp has more than one use? We won't actually remove it in that case, and are likely only removing a single instruction - the conditional branch. That starts to sound much more dubious to me.</div>
<div style=""><br></div><div style=""><br></div><div style="">Either way, I kept thinking that this could be simplified by using pattern matcher style code. I took a stab at it, and it looks something like the patch I attached. What do you think?</div>
</div></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Jan 3, 2013 at 11:04 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">Hi Chandler,<div><br></div><div>Here is the new patch that takes advantage of canonicalization previously done.</div>
<div>Note, however, that we cannot simplify the EQ, NE test, since the canonicalization does not occur if the result of the comparison is used more than once.</div><div><br></div><div>Thank you,</div><div><br></div><div>-Quentin</div>
<div></div></div>
<br><div style="word-wrap:break-word"><div></div><div><br><div><div>On Jan 2, 2013, at 5:52 PM, Quentin Colombet <<a href="mailto:qcolombet@apple.com" target="_blank">qcolombet@apple.com</a>> wrote:</div><br><blockquote type="cite">
<div style="word-wrap:break-word">Hi Chandler,<br><div>
</div>
<br><div><div>On Jan 2, 2013, at 4:59 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">+// Move the call to free before a NULL test.<br></div><div class="gmail_default"><div class="gmail_default">
<br></div><div class="gmail_default">Make this comment block use the doxygen format?</div></div></div></div></blockquote><br>I thought .cpp files were not proceeded in LLVM for the doxygen documentation.</div><div><br></div>
<div><blockquote type="cite"><div style="font-family:arial,helvetica,sans-serif;font-size:10pt"><div dir="ltr"><div class="gmail_default"><div class="gmail_default"><br></div><div class="gmail_default"><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">This is still a nested condition. I think you can simplify it by inverting and returning early.</div></div></div></div></div></blockquote><div>Sure.</div>
<br><blockquote type="cite"><div style="font-family:arial,helvetica,sans-serif;font-size:10pt"><div dir="ltr"><div class="gmail_default"><div class="gmail_default"><div class="gmail_default"> 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></div></div></div></blockquote>I didn't know that, thanks.<br><blockquote type="cite"><div style="font-family:arial,helvetica,sans-serif;font-size:10pt"><div dir="ltr"><div class="gmail_default"><div class="gmail_default">
<div class="gmail_default"><br></div><div class="gmail_default">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></blockquote>I'll have a closer look to that.</div><div><br></div><div>Thanks again for the review.</div><div><br></div><div>-Quentin<br><blockquote type="cite"><div style="font-family:arial,helvetica,sans-serif;font-size:10pt">
<div dir="ltr"><div class="gmail_default"><div class="gmail_default">
</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 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><br><div style="word-wrap:break-word"><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>
<br></blockquote></div><br></div></div>
</blockquote></div><br></div>
_______________________________________________<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></div>
<br></blockquote></div><br></div>
<span><pattern-match-free-hoist.patch></span></blockquote></div><br></div></body></html>