[llvm-commits] [PATCH] Skip NULL pointer check for free
Chandler Carruth
chandlerc at google.com
Mon Jan 7 09:45:14 PST 2013
LGTM
On Mon, Jan 7, 2013 at 9:43 AM, Quentin Colombet <qcolombet at apple.com>wrote:
> Ping?
>
> -Quentin
>
> On Jan 3, 2013, at 4:44 PM, Quentin Colombet <qcolombet at apple.com> wrote:
>
> 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>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130107/2c245bb2/attachment.html>
More information about the llvm-commits
mailing list