Chandler Carruth via llvm-dev
2016-Aug-03 00:33 UTC
[llvm-dev] RFC: We should stop merging allocas in the inliner
FYI, I ran some initial measurements just for sanity on the test-suite on x86-64: Total stack frame size: 13362520 -> 13365864 (+0.02%) Average stack frame size: 581.814 -> 581.96 (+0.02%) Average compile time: 5.71178 -> 5.67256 (within noise) Number of functions is exactly the same, so no trivially visible inlining decision changes. I've spot checked a few of the functions that had significant change in stack size. One shows a pattern that isn't too surprising in retrospect: this change allows us to hoist more code which causes live ranges to expand and increases spills. Nothing really scary here, and in many ways validates the core premise -- this does actually remove barriers to the mid-level optimizer. I've also found one case where stack coloring appears to just fail for no reason: "NArchive::N7z::GetStringForSizeValue(unsigned int)" in 7zHandler.cpp has exactly the same sequence of instructions after the change but 3x larger stack. But this appears to be rare and mostly relating to functions with already small stack sizes. I've filed PR28821 to track this. Anyways, while clearly some stuff will need cleaning up, I don't see anything really scary jumping out here. -Chandler On Tue, Aug 2, 2016 at 1:02 AM Chandler Carruth <chandlerc at gmail.com> wrote:> Seeing no big concerns raised with this direction... > > On Sun, Jul 31, 2016 at 9:47 PM Chandler Carruth <chandlerc at gmail.com> > wrote: > >> 1) Add a flag to turn this off. >> > > https://reviews.llvm.org/D23052 > > Will proceed from there. > > 2) Run benchmarks with flag to make sure things are actually working. >> 3) Change default for the flag, make sure chaos doesn't ensue. >> 4) Delete the (quite substantial) complexity associated with this and the >> flag. >> 5) Profit! >> >> -Chandler >> >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160803/aba2f7ea/attachment-0001.html>
David Majnemer via llvm-dev
2016-Aug-03 00:38 UTC
[llvm-dev] RFC: We should stop merging allocas in the inliner
On Tue, Aug 2, 2016 at 5:33 PM, Chandler Carruth via llvm-dev < llvm-dev at lists.llvm.org> wrote:> FYI, I ran some initial measurements just for sanity on the test-suite on > x86-64: > > Total stack frame size: 13362520 -> 13365864 (+0.02%) > Average stack frame size: 581.814 -> 581.96 (+0.02%) > Average compile time: 5.71178 -> 5.67256 (within noise) >Could you share the median stack frame sizes?> > Number of functions is exactly the same, so no trivially visible inlining > decision changes. > > I've spot checked a few of the functions that had significant change in > stack size. One shows a pattern that isn't too surprising in retrospect: > this change allows us to hoist more code which causes live ranges to expand > and increases spills. Nothing really scary here, and in many ways validates > the core premise -- this does actually remove barriers to the mid-level > optimizer. > > I've also found one case where stack coloring appears to just fail for no > reason: "NArchive::N7z::GetStringForSizeValue(unsigned int)" in > 7zHandler.cpp has exactly the same sequence of instructions after the > change but 3x larger stack. But this appears to be rare and mostly relating > to functions with already small stack sizes. I've filed PR28821 to track > this. > > Anyways, while clearly some stuff will need cleaning up, I don't see > anything really scary jumping out here. > > -Chandler > > On Tue, Aug 2, 2016 at 1:02 AM Chandler Carruth <chandlerc at gmail.com> > wrote: > >> Seeing no big concerns raised with this direction... >> >> On Sun, Jul 31, 2016 at 9:47 PM Chandler Carruth <chandlerc at gmail.com> >> wrote: >> >>> 1) Add a flag to turn this off. >>> >> >> https://reviews.llvm.org/D23052 >> >> Will proceed from there. >> >> 2) Run benchmarks with flag to make sure things are actually working. >>> 3) Change default for the flag, make sure chaos doesn't ensue. >>> 4) Delete the (quite substantial) complexity associated with this and >>> the flag. >>> 5) Profit! >>> >>> -Chandler >>> >> > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160802/9c344f9d/attachment.html>
Chandler Carruth via llvm-dev
2016-Aug-03 00:43 UTC
[llvm-dev] RFC: We should stop merging allocas in the inliner
On Tue, Aug 2, 2016 at 5:39 PM David Majnemer via llvm-dev < llvm-dev at lists.llvm.org> wrote:> On Tue, Aug 2, 2016 at 5:33 PM, Chandler Carruth via llvm-dev < > llvm-dev at lists.llvm.org> wrote: > >> FYI, I ran some initial measurements just for sanity on the test-suite on >> x86-64: >> >> Total stack frame size: 13362520 -> 13365864 (+0.02%) >> Average stack frame size: 581.814 -> 581.96 (+0.02%) >> Average compile time: 5.71178 -> 5.67256 (within noise) >> > > Could you share the median stack frame sizes? >24 bytes> > >> >> Number of functions is exactly the same, so no trivially visible inlining >> decision changes. >> >> I've spot checked a few of the functions that had significant change in >> stack size. One shows a pattern that isn't too surprising in retrospect: >> this change allows us to hoist more code which causes live ranges to expand >> and increases spills. Nothing really scary here, and in many ways validates >> the core premise -- this does actually remove barriers to the mid-level >> optimizer. >> >> I've also found one case where stack coloring appears to just fail for no >> reason: "NArchive::N7z::GetStringForSizeValue(unsigned int)" in >> 7zHandler.cpp has exactly the same sequence of instructions after the >> change but 3x larger stack. But this appears to be rare and mostly relating >> to functions with already small stack sizes. I've filed PR28821 to track >> this. >> >> Anyways, while clearly some stuff will need cleaning up, I don't see >> anything really scary jumping out here. >> >> -Chandler >> >> On Tue, Aug 2, 2016 at 1:02 AM Chandler Carruth <chandlerc at gmail.com> >> wrote: >> >>> Seeing no big concerns raised with this direction... >>> >>> On Sun, Jul 31, 2016 at 9:47 PM Chandler Carruth <chandlerc at gmail.com> >>> wrote: >>> >>>> 1) Add a flag to turn this off. >>>> >>> >>> https://reviews.llvm.org/D23052 >>> >>> Will proceed from there. >>> >>> 2) Run benchmarks with flag to make sure things are actually working. >>>> 3) Change default for the flag, make sure chaos doesn't ensue. >>>> 4) Delete the (quite substantial) complexity associated with this and >>>> the flag. >>>> 5) Profit! >>>> >>>> -Chandler >>>> >>> >> _______________________________________________ >> LLVM Developers mailing list >> llvm-dev at lists.llvm.org >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >> >> _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160803/6fc2eb81/attachment.html>