<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Mon, Aug 1, 2016 at 7:23 PM Gerolf Hoflehner via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org">llvm-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
> On Jul 31, 2016, at 9:47 PM, Chandler Carruth via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<br>
><br>
> Chris added alloca merging in the inliner a looooong time ago, 2009. The reason he added it was because at the time we didn't do stack coloring and without it we had serious stack size problems in LLVM.<br>
><br>
> Since then, a few rather important things have changed:<br>
> - We have reasonably powerful stack coloring support in the backend based on lifetime markers<br>
> - Clang (the primary frontend I'm aware of that hit issues with this) is now emitting lifetime markers for essentially everything<br>
> - Clang even has front-end based -O0 stack reuse tricks<br>
> - AliasAnalysis has become even more important (extended to codegen time etc) and relies heavily on distinguishing between allocas.<br>
> - We have lots of tools like the sanitizers that directly reason about allocas to catch bugs in user programs.<br>
><br>
> The first two changes pretty much completely obviate the need for alloca merging as far as I'm aware, and the second two changes make the information loss caused by this practice, IMO, really bad.<br>
><br>
> Even if there are problems exposed by turning off alloca merging in the inliner, they are almost certainly deficiencies in stack coloring, Clang, or other parts of the optimizer that we should fix rather than continue to ignore.<br>
><br>
> Thoughts? The code changes are easy and mechanical. My plan would be:<br>
<br>
I like the principal idea.<br>
> 1) Add a flag to turn this off.<br>
> 2) Run benchmarks with flag to make sure things are actually working.<br>
> 3) Change default for the flag, make sure chaos doesn't ensue.<br>
<br>
What does this imply? Just run-time?  I’d like to see stack size data w/ and w/o the change and  - just in case - an investigation/fix of the outliers.</blockquote><div><br></div><div>Sure, I'll do both some benchmarks and also look at some stack sizes.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Any thoughts about compile-time and code size?<br></blockquote><div><br></div><div>I think compile-time regressions are certainly possible with heavier use of AA and stack coloring, but we'd likely be hitting those through other patterns than just this one already. It doesn't seem likely this will really cause much churn there, but certainly if reports come in, we will have a flag that we can toggle until things are fixed.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
You probably also want to collect the data across more than one platform.<br></blockquote><div><br></div><div>I'll certainly collect it for the platforms I have access to and such, but hopefully those interested in other platforms and worried about stack space can also do some testing. That's why I want to add a flag so its super easy to check whether this regresses something you care about.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
> 4) Delete the (quite substantial) complexity associated with this and the flag.<br>
You might need to allow target specific behavior for some grace period.<br></blockquote><div><br></div><div>I really don't think this warrants a long grace period much less target specific behavior.</div><div><br></div><div>The goal is simplification. We have lots of infrastructure for a target to control how stack coloring and layout is done. Honestly, I suspect most of the stack objects already go down this path. It is only array type allocas that we even bother with this merging for right now, so I'm not expecting any dramatic changes.</div><div><br></div><div>Clearly, if folks report problems, we'll turn it back on and have to sort out what happened, but I think we can take a pretty simple approach to get from here to there...</div></div></div>