[llvm-commits] [PATCH] Skip NULL pointer check for free
Quentin Colombet
qcolombet at apple.com
Thu Jan 3 16:44:49 PST 2013
Hi Chandler,
Thanks for the proposed modified patch. It looks great.
By the way, thanks for the pattern matcher style code, I did not know llvm has such a thing.
To answer your question, it is useful to perform the transformation even if the icmp has more than one use.
Indeed, it will allow to remove two branch instructions (the conditional AND the unconditional one) and one basic block.
Should I commit your modified patch? (When possible :))
-Quentin
On Jan 3, 2013, at 3:56 PM, Chandler Carruth <chandlerc at google.com> wrote:
> On Thu, Jan 3, 2013 at 11:04 AM, Quentin Colombet <qcolombet at apple.com> wrote:
> Hi Chandler,
>
> Here is the new patch that takes advantage of canonicalization previously done.
> 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.
>
> 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.
>
> 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.
>
>
> 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?
>
>
> On Thu, Jan 3, 2013 at 11:04 AM, Quentin Colombet <qcolombet at apple.com> wrote:
> Hi Chandler,
>
> Here is the new patch that takes advantage of canonicalization previously done.
> 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.
>
> Thank you,
>
> -Quentin
>
>
> On Jan 2, 2013, at 5:52 PM, Quentin Colombet <qcolombet at apple.com> wrote:
>
>> Hi Chandler,
>>
>> On Jan 2, 2013, at 4:59 PM, Chandler Carruth <chandlerc at google.com> wrote:
>>
>>> +// Move the call to free before a NULL test.
>>>
>>> Make this comment block use the doxygen format?
>>
>> I thought .cpp files were not proceeded in LLVM for the doxygen documentation.
>>
>>>
>>> + // If equal, it must reach the call to free by the false branch.
>>> + if ((Condition->getPredicate() == ICmpInst::ICMP_EQ && !FreeBBIsTrue) ||
>>> + // If not equal, by the true branch
>>> + (Condition->getPredicate() == ICmpInst::ICMP_NE && FreeBBIsTrue)) {
>>> + Value *FirstCondOp = Condition->getOperand(0);
>>> + Value *SecondCondOp = Condition->getOperand(1);
>>> + Value *NullPtr =
>>> + ConstantPointerNull::get(dyn_cast<PointerType>(Op->getType()));
>>> + // Does the condition checks NULL against the argument of Free
>>> + if ((Op == FirstCondOp && NullPtr == SecondCondOp) ||
>>> + (Op == SecondCondOp && NullPtr == FirstCondOp)) {
>>> + // Move the call to free in the predecessor
>>> + FI.moveBefore(PredBBBr);
>>> + return &FI;
>>> + }
>>> + }
>>>
>>> This is still a nested condition. I think you can simplify it by inverting and returning early.
>> Sure.
>>
>>> 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.
>> I didn't know that, thanks.
>>>
>>> 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.
>> I'll have a closer look to that.
>>
>> Thanks again for the review.
>>
>> -Quentin
>>>
>>>
>>> On Wed, Jan 2, 2013 at 3:56 PM, Quentin Colombet <qcolombet at apple.com> wrote:
>>> Hi Chandler,
>>>
>>> Thanks for the very detailed review!
>>>
>>> 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).
>>>
>>> I let you have a look to the patch to check if I miss anything.
>>>
>>> Thank you,
>>>
>>> -Quentin
>>>
>>>
>>>
>>> On Jan 2, 2013, at 1:16 PM, Chandler Carruth <chandlerc at google.com> wrote:
>>>
>>>> Cool, thanks.
>>>>
>>>> + // Check if this free is accessed after its argument has been test
>>>> + // against NULL (property 0).
>>>> + // If yes, it is legal to move this call in its predecessor block.
>>>> + // Currently, we consider that it is profitable only if the block
>>>> + // containing the call to free will be removed, i.e.:
>>>> + // 1. it has only one predecessor P, and P has two successors
>>>> + // 2. it contains the call and an unconditional branch
>>>> + // 3. its successor is the same as its predecessor's successor
>>>> + // FIXME: Could handle more general cases but we need a cost model
>>>>
>>>> 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.
>>>>
>>>> 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.
>>>>
>>>>
>>>> + // 1. Only one predecessor with 2 successors
>>>>
>>>> It took me a moment to realize what '1.' is here... Maybe "Validate constraint #1:"?
>>>>
>>>> + if (!PredBB ||
>>>> + // Check part of 0., i.e., is the terminator inst a branch?
>>>> + // If not, a fortiori, the condition cannot be a test against NULL
>>>> + !(PredBBBr = dyn_cast<BranchInst>(PredBB->getTerminator())) ||
>>>> + PredBBBr->getNumSuccessors() != 2)
>>>> + return 0;
>>>>
>>>> I think this wil lbe much easier to read if you break it out into more early returning conditions:
>>>>
>>>> if (!PredBB)
>>>> return 0;
>>>>
>>>> BranchInst *PrebBBBr = dyn_cast<BranchInst>(PredBB->getTerminator());
>>>> if (!PredBBBr || PredBBr->getNumSuccessors() != 2)
>>>> return 0;
>>>>
>>>> You can also simplify the assert code with this pattern.
>>>>
>>>> + if (FreeInstrBB->size() == 2 &&
>>>> + (FIBBBr = dyn_cast<BranchInst>(FreeInstrBB->getTerminator())) &&
>>>> + FIBBBr->isUnconditional() &&
>>>> + // 3. Same successor
>>>> + FIBBBr->getSuccessor(0) == SuccBB &&
>>>> + // 0. Does the condition is an equal or not equal to something.
>>>> + (Condition = dyn_cast<ICmpInst>(PredBBBr->getCondition())) &&
>>>> + // If equal, it must get reach the call to free by the false branch.
>>>> + ((Condition->getPredicate() == ICmpInst::ICMP_EQ && !FreeBBIsTrue) ||
>>>> + // If not equal, by the true branch
>>>> + (Condition->getPredicate() == ICmpInst::ICMP_NE &&
>>>> + FreeBBIsTrue))) {
>>>>
>>>> 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.
>>>>
>>>> Maybe hoist this whole thing into a helper function?
>>>>
>>>> if (MinimizeSize)
>>>> if (Instruction *I = tryToRemoveNullTestBeforeFree(BI))
>>>> return I;
>>>>
>>>> My name is terrible, maybe you can think of a better one.
>>>>
>>>> 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.
>>>>
>>>> On Wed, Jan 2, 2013 at 11:42 AM, Quentin Colombet <qcolombet at apple.com> wrote:
>>>> Hi Chandler,
>>>>
>>>> Thanks for the comments.
>>>>
>>>> Like you pointed out, the patch can really hurt the performance in some cases.
>>>>
>>>> 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.
>>>>
>>>> New patch attached.
>>>>
>>>> -Quentin
>>>>
>>>>
>>>>
>>>> On Jan 2, 2013, at 11:15 AM, Chandler Carruth <chandlerc at google.com> wrote:
>>>>
>>>>> I really do share Nick's concern.
>>>>>
>>>>> 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.
>>>>>
>>>>> 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.
>>>>>
>>>>> 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.
>>>>>
>>>>> On Wed, Jan 2, 2013 at 10:18 AM, Quentin Colombet <qcolombet at apple.com> wrote:
>>>>> Ping!
>>>>> -Quentin
>>>>>
>>>>>
>>>>>
>>>>> On Dec 21, 2012, at 4:04 PM, Quentin Colombet <qcolombet at apple.com> wrote:
>>>>>
>>>>>> Hi Nick,
>>>>>>
>>>>>> On Dec 21, 2012, at 3:59 PM, Dmitri Gribenko <gribozavr at gmail.com> wrote:
>>>>>>
>>>>>>> On Sat, Dec 22, 2012 at 1:53 AM, Nick Lewycky <nicholas at mxc.ca> wrote:
>>>>>>>> On 12/21/2012 03:43 PM, Quentin Colombet wrote:
>>>>>>>>>
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> Here is a patch to help llvm turning a code like this:
>>>>>>>>> if (foo)
>>>>>>>>> free(foo)
>>>>>>>>>
>>>>>>>>> into that:
>>>>>>>>> free(foo)
>>>>>>>>>
>>>>>>>>> It is legal and safe, since free is already checking its argument
>>>>>>>>> against NULL internally (I may find the reference in the Standard if
>>>>>>>>> needed :)).
>>>>>>>>
>>>>>>>> It's legal and safe, but is it desirable? Calling a function is expensive
>>>>>>>> even if the function doesn't do anything.
>>>>>>
>>>>>> Yes, you're right, it may not be desirable but I'm tempt to think like Dmitri.
>>>>>>
>>>>>> Do you think that this optimization should be controlled by a flag?
>>>>>> At least, for code size (Oz and maybe Os), it may always be desirable.
>>>>>>
>>>>>> I did not consider changing free(foo) into if(foo) free(foo)
>>>>>>
>>>>>>>
>>>>>>> That's true, but there's another viewpoint: programmers usually write
>>>>>>> 'if(!foo) free(foo);' not because they are manually inlining the fast
>>>>>>> path of free(), but because they think that free(NULL) is UB.
>>>>>>>
>>>>>>> Dmitri
>>>>>>>
>>>>>>> --
>>>>>>> main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
>>>>>>> (j){printf("%d\n",i);}}} /*Dmitri Gribenko <gribozavr at gmail.com>*/
>>>>>>
>>>>>> _______________________________________________
>>>>>> llvm-commits mailing list
>>>>>> llvm-commits at cs.uiuc.edu
>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> llvm-commits mailing list
>>>>> llvm-commits at cs.uiuc.edu
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
>
> <pattern-match-free-hoist.patch>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130103/7e1c321a/attachment.html>
More information about the llvm-commits
mailing list