<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div apple-content-edited="true">Hi Eric & Jim,
</div><div apple-content-edited="true"><br></div><div apple-content-edited="true">Thanks you both for the reviews.</div><div apple-content-edited="true">Comments inlined.</div>
<br><div><div>On Sep 24, 2013, at 11:58 AM, Jim Grosbach <<a href="mailto:grosbach@apple.com">grosbach@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">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?</blockquote><div>I am not sure what you are proposing here.</div><div><br></div><div>Do you want to get rid of SmallPtrSet<> instance?</div><div><br></div><div>I.e., replace this:</div><div><div style="margin: 0px; font-size: 11px; font-family: Menlo;">+ if (!NoReturnInsts.count(&(*It))) {</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;">+ MRI->setPhysRegUsed(Reg);</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;">+ break;</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;">+ }</div><div style="margin: 0px;"><br></div><div style="margin: 0px;">By a check that It is a no-return instruction?</div><div style="margin: 0px;"><br></div><div style="margin: 0px;">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:</div><div style="margin: 0px;">1. Get to operand with the symbol.</div><div style="margin: 0px;">2. Check that the symbol carries the NoReturn attribute.</div><div style="margin: 0px;"><br></div><div style="margin: 0px;">For 1., you have to iterate through all the MI operands, unless again I am missing something.</div><div style="margin: 0px;">In the end, this check looks like:</div><div style="margin: 0px;"><div style="margin: 0px; font-size: 11px; font-family: Menlo;">+ // Check if this instruction is a call to a noreturn function.</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;">+ // In that case, all the definitions set by this instruction can</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;">+ // be ignored.</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;">+ if (IsExitBB && MI->isCall())</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;">+ for (MachineInstr::mop_iterator MOI = MI->operands_begin(),</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;">+ MOE = MI->operands_end(); MOI != MOE; ++MOI) {</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;">+ MachineOperand &MO = *MOI;</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;">+ if (!MO.isGlobal())</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;">+ continue;</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;">+ const Function *Func = dyn_cast<Function>(MO.getGlobal());</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;">+ if (!Func || !Func->hasFnAttribute(Attribute::NoReturn))</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;">+ continue;</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;">+ NoReturnInsts.insert(MI);</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;">+ break;</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;">+ }</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;"><br></div><div>So, we may not want to do that for each registers (i.e., one call is checked several times).</div><div><br></div><div>Jim, what do you think?</div><div><br></div><div>Again, I may have missed a more direct way to access the noreturn attribute, and I would be glad to fix the patch accordingly!</div><div><br></div><div>Thanks,</div><div>Quentin</div></div></div><br><blockquote type="cite"><br>-Jim<br><br>On Sep 24, 2013, at 11:51 AM, Eric Christopher <<a href="mailto:echristo@gmail.com">echristo@gmail.com</a>> wrote:<br><br><blockquote type="cite">Seems sane to me, Jim was one of the last people in here so he could<br>probably review as well.<br><br>-eric<br><br>On Tue, Sep 24, 2013 at 10:48 AM, Quentin Colombet <<a href="mailto:qcolombet@apple.com">qcolombet@apple.com</a>> wrote:<br><blockquote type="cite">Ping^3?<br><br>-Quentin<br><br>On Sep 17, 2013, at 10:54 AM, Quentin Colombet <<a href="mailto:qcolombet@apple.com">qcolombet@apple.com</a>> wrote:<br><br>Ping^2?<br><br>-Quentin<br><br>On Sep 10, 2013, at 10:54 AM, Quentin Colombet <<a href="mailto:qcolombet@apple.com">qcolombet@apple.com</a>> wrote:<br><br>Ping?<br><br>-Quentin<br><br>On Sep 3, 2013, at 2:16 PM, Quentin Colombet <<a href="mailto:qcolombet@apple.com">qcolombet@apple.com</a>> wrote:<br><br>Hi,<br><br>The proposed patch fixes PR16882:<br><a href="http://llvm.org/bugs/show_bug.cgi?id=16882">http://llvm.org/bugs/show_bug.cgi?id=16882</a>.<br>PR16882 reports a poor code generation issue where we were saving the link<br>register before a call to a noreturn function, whereas it is not needed.<br><br>Thanks for your reviews.<br><br>** Details **<br><br>PEI inserts a save/restore sequence for the link register, according to the<br>information it gets from the MachineRegisterInfo.<br>MachineRegisterInfo is populated by the VirtRegMap pass.<br>This pass was not aware of noreturn calls and was registering the<br>definitions of these calls the same way as regular operations.<br><br><br>** Proposed Solution **<br><br>Modify VirtRegPass so that it does not set the isPhysRegUsed information for<br>registers only defined by noreturn calls.<br>The rational is that a noreturn call is the "last instruction" of the<br>program (if it returns the behavior is undefined), so everything that is<br>defined by it cannot be used and will not interfere with anything else.<br>Therefore, it is pointless to account for then.<br><br><br>** Experiments **<br><br>I have made functional tests on x86_64 with O3 -fomit-frame-pointer on the<br>llvm test-suite + SPEC.<br>I did the same for ARMv7s.<br><br>I have attached the performance numbers for ARMv7s (lower is better). These<br>numbers only reports benches where the patch kicks in and the test runs for<br>more than 1s (to avoid noise).<br>The Reference column is LLVM r189613 and Test column is LLVM r189613 + the<br>proposed patch.<br><br>Cheers,<br>-Quentin<br><virtreg.svndiff><br><noreturn_perf.txt><br>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits<br><br><br>_______________________________________________<br>llvm-commits mailing list<br>llvm-commits@cs.uiuc.edu<br>http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits<br><br><br>_______________________________________________<br>llvm-commits mailing list<br>llvm-commits@cs.uiuc.edu<br>http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits<br><br><br><br>_______________________________________________<br>llvm-commits mailing list<br>llvm-commits@cs.uiuc.edu<br>http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits<br><br></blockquote></blockquote><br></blockquote></div><br></body></html>