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

[Quentin Colombet]

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?

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!

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