[PATCH][PR16882] Ignore noreturn definitions when setting isPhysRegUsed.
Quentin Colombet
qcolombet at apple.com
Tue Sep 24 17:30:23 PDT 2013
Thanks!
Committed in r191349.
Thanks again Eric for the rewording.
-Quentin
On Sep 24, 2013, at 1:42 PM, Jim Grosbach <grosbach at apple.com> wrote:
>
> On Sep 24, 2013, at 1:08 PM, Quentin Colombet <qcolombet at apple.com> wrote:
>
>> Hi Eric & Jim,
>>
>> Thanks you both for the reviews.
>> Comments inlined.
>>
>> On Sep 24, 2013, at 11:58 AM, Jim Grosbach <grosbach at apple.com> wrote:
>>
>>> Seems OK. I wonder if it’s worth it calculating the set of no-return instructions ahead of time. Looking for the common case of there not being any at all seems a reasonable optimization, but in the case where we have one or more, just checking directly from the iterator rather than a lookup in a SmallPtrSet<> will probably be just as fast and be a bit simpler code?
>> I am not sure what you are proposing here.
>>
>> Do you want to get rid of SmallPtrSet<> instance?
>>
>> I.e., replace this:
>> + if (!NoReturnInsts.count(&(*It))) {
>> + MRI->setPhysRegUsed(Reg);
>> + break;
>> + }
>>
>> By a check that It is a no-return instruction?
>
> That’s what I was curious about exploring, yes.
>
>>
>> If yes, I do not think this is reasonable, because unless I am missing something, checking that the a MI has the NoReturn attribute is expensive:
>> 1. Get to operand with the symbol.
>> 2. Check that the symbol carries the NoReturn attribute.
>>
>> For 1., you have to iterate through all the MI operands, unless again I am missing something.
>> In the end, this check looks like:
>> + // Check if this instruction is a call to a noreturn function.
>> + // In that case, all the definitions set by this instruction can
>> + // be ignored.
>> + if (IsExitBB && MI->isCall())
>> + for (MachineInstr::mop_iterator MOI = MI->operands_begin(),
>> + MOE = MI->operands_end(); MOI != MOE; ++MOI) {
>> + MachineOperand &MO = *MOI;
>> + if (!MO.isGlobal())
>> + continue;
>> + const Function *Func = dyn_cast<Function>(MO.getGlobal());
>> + if (!Func || !Func->hasFnAttribute(Attribute::NoReturn))
>> + continue;
>> + NoReturnInsts.insert(MI);
>> + break;
>> + }
>>
>> So, we may not want to do that for each registers (i.e., one call is checked several times).
>>
>> Jim, what do you think?
>>
>> Again, I may have missed a more direct way to access the noreturn attribute, and I would be glad to fix the patch accordingly!
>
> I think I was remembering some of the helper classes we have at the IR level (e.g., CallSite) that make this sort of thing easy.
>
> Patch LGTM.
>
>
> -JIm
>
>>
>> Thanks,
>> Quentin
>>
>>>
>>> -Jim
>>>
>>> On Sep 24, 2013, at 11:51 AM, Eric Christopher <echristo at gmail.com> wrote:
>>>
>>>> Seems sane to me, Jim was one of the last people in here so he could
>>>> probably review as well.
>>>>
>>>> -eric
>>>>
>>>> On Tue, Sep 24, 2013 at 10:48 AM, Quentin Colombet <qcolombet at apple.com> wrote:
>>>>> Ping^3?
>>>>>
>>>>> -Quentin
>>>>>
>>>>> On Sep 17, 2013, at 10:54 AM, Quentin Colombet <qcolombet at apple.com> wrote:
>>>>>
>>>>> Ping^2?
>>>>>
>>>>> -Quentin
>>>>>
>>>>> On Sep 10, 2013, at 10:54 AM, Quentin Colombet <qcolombet at apple.com> wrote:
>>>>>
>>>>> Ping?
>>>>>
>>>>> -Quentin
>>>>>
>>>>> On Sep 3, 2013, at 2:16 PM, Quentin Colombet <qcolombet at apple.com> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> The proposed patch fixes PR16882:
>>>>> http://llvm.org/bugs/show_bug.cgi?id=16882.
>>>>> PR16882 reports a poor code generation issue where we were saving the link
>>>>> register before a call to a noreturn function, whereas it is not needed.
>>>>>
>>>>> Thanks for your reviews.
>>>>>
>>>>> ** Details **
>>>>>
>>>>> PEI inserts a save/restore sequence for the link register, according to the
>>>>> information it gets from the MachineRegisterInfo.
>>>>> MachineRegisterInfo is populated by the VirtRegMap pass.
>>>>> This pass was not aware of noreturn calls and was registering the
>>>>> definitions of these calls the same way as regular operations.
>>>>>
>>>>>
>>>>> ** Proposed Solution **
>>>>>
>>>>> Modify VirtRegPass so that it does not set the isPhysRegUsed information for
>>>>> registers only defined by noreturn calls.
>>>>> The rational is that a noreturn call is the "last instruction" of the
>>>>> program (if it returns the behavior is undefined), so everything that is
>>>>> defined by it cannot be used and will not interfere with anything else.
>>>>> Therefore, it is pointless to account for then.
>>>>>
>>>>>
>>>>> ** Experiments **
>>>>>
>>>>> I have made functional tests on x86_64 with O3 -fomit-frame-pointer on the
>>>>> llvm test-suite + SPEC.
>>>>> I did the same for ARMv7s.
>>>>>
>>>>> I have attached the performance numbers for ARMv7s (lower is better). These
>>>>> numbers only reports benches where the patch kicks in and the test runs for
>>>>> more than 1s (to avoid noise).
>>>>> The Reference column is LLVM r189613 and Test column is LLVM r189613 + the
>>>>> proposed patch.
>>>>>
>>>>> Cheers,
>>>>> -Quentin
>>>>> <virtreg.svndiff>
>>>>> <noreturn_perf.txt>
>>>>> _______________________________________________
>>>>> 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
>>>>>
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> 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/20130924/e2e5cda5/attachment.html>
More information about the llvm-commits
mailing list