RFR(L): 8005071: Incremental inlining for JSR 292

[Vladimir Kozlov]

On 12/17/12 9:18 AM, Roland Westrelin wrote:
>
> Vladimir,
>
> Thanks for reviewing this.
>
>> Roland my main concern is GraphKit::kill_dead_call_outputs(). We avoiding such dead node elimination during paring since a graph may not complete and dominate information is not accurate. Can you bailout the round of later inlining if you detect such case and let PhaseRemoveUseless do cleanup for you?
>
> Let's say we have 2 calls in a method:
>
> A()
>
> B()
>
> A() dominates B(). During parsing, we build a complete graph with CallNodes for A() and B(). Let's say we decide to inline A() after parsing so we build a subgraph with A()'s inputs as inputs to parsing and then we replace B()'s outputs with the resulting new subgraph.

I assume you mean "replace A()'s outputs".

>
> Now let's say A()'s outputs are dead and we try to inline B(). It depends on A()'s outputs but they are dead and often C2 will see an inconsistent state when it builds the subgraph for B(): control not top so it proceeds with inlining, but a MergedMem with some slices set to top or if B() uses the return from A(), top as an input. These lead to crashes.

Top inputs should be processed correctly. If we crash, it indicates that 
we are missing checks for top inputs in some ideal nodes methods 
(Ideal(), Value()).

Also kill_dead_call_outputs() works only if final_ctl->is_top() which 
contradict to your case.

>
> That doesn't happen with parse-time inlining because A()'s subgraph is built inside the main graph, all results from A()'s inlining are propagated forward to B() by the gvn, the input state for B() is consistent and we don't try to inlining B() because this is a dead path.

We don't inline B() if control is top (stopped()). So it is the same 
situation as with kill_dead_call_outputs().

>
> I don't think PhaseRemoveUseless helps in this case. What we would need in the case of post parse inlining would be to inline A(), apply an igvn to propagate the results from the inlining forward to B(), then inline B(). But even an igvn is not sufficient if for instance, B() is inside a loop.

May be IGVN is not enough (it does not have deliminators information and 
works on small subgraphs) but PhaseIdealLoop can do that (it was the 
reason I called it before EA). Which brings the question: why you call 
PhaseIdealLoop only when live nodes reach limit?

>
> That's why I use GraphKit::kill_dead_call_outputs(). I don't understand why it's dangerous since it operates in well defined conditions. In the example above, we have a complete graph with a DirectCall to A(). We are done with parsing for A() and the subgraph for A() uses A()'s inputs but it is not yet connected to the rest of the graph by it's outputs. We want to find calls that A()'s control outputs dominate and we work on a complete graph, the subgraph for A() doesn't really matter.

I don't see the big need for new "dead code cleanup" code when we have 
already several implementations. We may need to adjust them to work for 
your case (PhaseIdealLoop work without inner loops).

Thanks,
Vladimir

>
>> The same about Compile::look_for_dead_calls(). Why you need this check if you do PhaseRemoveUseless?
>
> With CTW, I found some rare cases, where an igvn pass makes some path dead but similar to what I describe above the state at a call that we want to inline is not consistent.
>
> Roland.
>