[PATCH][PR16882] Ignore noreturn definitions when setting isPhysRegUsed.

[Jim Grosbach]

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/3e722319/attachment.html>