[llvm-commits] [llvm] r163678 - in /llvm/trunk: lib/CodeGen/StackColoring.cpp test/CodeGen/X86/StackColoring.ll

[Nadav Rotem]

On Sep 17, 2012, at 12:01 PM, Duncan Sands <baldrick at free.fr> wrote:

> Hi,
> 
> On 17/09/12 05:58, Daniel Berlin wrote:
>> On Sat, Sep 15, 2012 at 9:03 AM, Nadav Rotem <nrotem at apple.com> wrote:
>>> 
>>> 
>>> On Sep 15, 2012, at 14:24, Nuno Lopes <nunoplopes at sapo.pt> wrote:
>>> 
>>>> Wait, doesn't this mean that you don't need the lifetime markers at all??
>>>> If you remove the markers that you cannot prove that are ok, then it's equivalent to not having this information at all, and then use only the computed liveness intervals.
>>>> 
>>> 
>>> We use the lifetime markers to calculate the liveness intervals and throw away the intervals which are found to be broken. We need the markers to calculate the intervals.
>> 
>> As Duncan asked, shouldn't you just fix the producers of such broken
>> intervals (be they compilers or user code)?
>> For example, if someone returns a reference to a local variable,
>> beside being undefined, the code will often get broken by other
>> optimizations anyway.
>> 
>> This won't be a surprise either, since both clang and GCC warn about
>> it by default in the case of returning a reference to a local.
>> 
>> Besides Duncan's concern about cost, note that there are always
>> wonderfully exciting ways people will come up with broken/illegal code
>> that "breaks" a particular pass, so if you go down this route, you may
>> find other things you have to check for as well.  As they say, it's
>> impossible to make anything foolproof, because fools are so damn
>> ingenious.
> 
> what's more, it is possible to have a memory access outside the lifetime
> markers for correct code, when that code is unreachable.
> 
> If the point of view (taken by the rest of the compiler) is that IR is
> correct and that that fact should be exploited maximally, then when you
> see a "broken" memory access you would say: "great, this is telling me
> that this memory access is unreachable due to some complicated reasons
> that were beyond our power to analyse previously, let me replace it with
> unreachable for a nice code size reduction and potential speedup".
> 
> Instead you are penalizing correct IR (by not doing this optimization) and
> instead you're trying to sweep wrong IR under the carpet where no one will
> notice.  That's a losing game in my opinion.

I am also CC-ing LLVM-dev. 

Currently we have the flag "ProtectFromEscapedAllocas" which decides if we want to detect obvious violations of the lifetime zone and disable the coloring for that specific zone. 

I agree with what you are saying and I intend to move forward with the plan we agreed upon, which is, (1) disable the flag and find out what breaks, (2) fix the broken user code or compiler passes, (3) repeat. 

We also have three more things that we need to do:
1. We need to remove the inliner hack. I found that the StackColoring recovers 99% of the memory that is saved by the hack, but there are a few programs which still lose (allocate more stack space) when removing the hack.
2. We need to add a clang flag for disabling stack coloring.
3. We need to modify clang to generate more lifetime markers, not only when inlining. 


> Indeed, replacing such memory accesses by unreachable is also a good thing
> for incorrect IR: codegen will turn that into a "trap" instruction, and
> users (and compiler writers) will quickly discover the mistake in their code
> and fix it [*].  Or if they aren't willing to fix it then at least they will
> quickly find out what the problem is and turn off stack colouring via the
> appropriate gcc/clang option.
> 
> Ciao, Duncan.
> 
> [*] You can even market this as a feature, since using temporaries beyond
> their lifetimes is a common and very nasty mistake - but now clang will
> tell you about it by causing a trap to occur!


Thanks,
Nadav