[PATCH] D18827: Rework/enhance stack coloring data flow analysis.

[Wei Mi via llvm-commits]

On Mon, May 16, 2016 at 9:16 AM, Than McIntosh <thanm at google.com> wrote:
> thanm added a comment.
>
> Wei, you suggested adding an assert that triggers if an instruction is found otuside the the start/end lifetime that accesses memory.
>
> I experimented a bit with this; turns out there is a roadblock here relating to how I degenerate ranges are identified in my implementation.
>
> At the moment I am not doing flow analysis to determine ranges of instructions that fall in between lifetime start/end for a given slot -- the code is only doing a single pass over the CFG. This makes it fast, but there is some imprecision, e.g. if there is unstructure control flow, a slot may appear degenerate when it is not.
>
> Here is an example, abstracted from the 400.perlbench function store_scalar():
>
>   void store_scalar(...) {
>      ...
>      if (...) goto string;
>      ...
>      if (cond) {
>         int wlen;
>         ...
>         string:
>           wlen = ...;
>      }
>   }
>
> Note the 'goto'. The front end will place start and end lifetime markers within the body of the "if (cond)" true block, but due to the goto there is a path that leads from the entry block to the 'wlen' assignment that doesn't go pass through the lifetime start marker. If the ordering of blocks in "depth_first(MF)" happens to choose that path, then we'll see what looks like a reference to "wlen" before we see its lifetime start marker.
>
> I'm not sure it's worth it to slow down the pass by doing real dataflow analysis to collect lifetime ranges-- that seems like overkill to me.
>

I agree with you. That will be an overkill.

To catch the real degenerate cases, when ProtectFromEscapedAllocas is
true, StackColoring::removeInvalidSlotRanges already does it by
checking whether there is load/store outside the LiveInterval created
from lifetime markers. For me, I feel like it seems more meaningful to
change ProtectFromEscapedAllocas from a protection mechanism to a
sanity check, but I am not sure if any user depends on the option for
applications out of tree. Anyway, it can be a followup patch.

Two minor comments. Otherwise, LGTM.

Thanks,
Wei.