Request for reviews (S): 6809798: SafePointScalarObject node placed into incorrect block during GCM

[Vladimir Kozlov]

I changed comments as suggested, added the assert into
replace_block_proj_ctrl() and ran CTW with -XX:+DoEscapeAnalysis.
I did not get any failures.

I updated webrev http://cr.openjdk.java.net/~kvn/6809798/webrev.01
and I will push the fix with Tom as reviewer.

Thanks,
Vladimir

Vladimir Kozlov wrote:
> I looked which nodes are pinned and all of them are CFG nodes
> except Phi and SafePointScalarObject.
> The block projection control edge of CFG nodes will be replaced
> with new Region nodes during PhaseCFG::build_cfg() before GCM.
> And Phi node can not have block projection control - only Region.
> Which leaves only SafePointScalarObject node we need to patch.
> 
> I can add additional assert to verify this:
> 
>  void PhaseCFG::replace_block_proj_ctrl( Node *n ) {
>    const Node *in0 = n->in(0);
>    assert(in0 != NULL, "Only control-dependent");
>    const Node *p = in0->is_block_proj();
>    if (p != NULL && p != n) {    // Control from a block projection?
> +    assert(!n->pinned() || n->is_SafePointScalarObject(), "not 
> SafePointScalarObject");
>      // Find trailing Region
> 
> Vladimir
> 
> Vladimir Kozlov wrote:
>> I only saw asserts for SafePointScalarObjectNodes.
>> But patching control edge of pinned nodes before scheduling
>> affects all pinned nodes: safepoints, phis.
>> And I think this change is correct for all pinned nodes.
>>
>> I actually was not able to catch the case were
>> safepoint has block projection control to see what
>> happened. In the bugs case it was scheduled correctly
>> even with current code. But it could be the case
>> when control was set differently for SFPT and SPSO nodes
>> during IGVN in IfNode::dominated_by() which I fixed by
>> SafePointScalarObjectNode::depends_only_on_test() const { return false; }
>>
>> Thanks,
>> Vladimir
>>
>> Tom Rodriguez wrote:
>>> Were SafePointScalarObjectNodes the only one affected by this or will 
>>> this change the schedule of other nodes?
>>>
>>> tom
>>>
>>> On Feb 26, 2009, at 10:15 AM, Vladimir Kozlov wrote:
>>>
>>>> http://cr.openjdk.java.net/~kvn/6809798/webrev.00
>>>>
>>>> Fixed 6809798: SafePointScalarObject node placed into incorrect 
>>>> block during GCM
>>>>
>>>> Problem:
>>>> The main problem is that the control edge of a pinned node
>>>> could be replaced during schedule_early after the pinned
>>>> node scheduled into a block if the control node is a block
>>>> projection.
>>>> Also there were few places where the control edge of
>>>> a SafePointScalarObject node was not adjusted correctly.
>>>>
>>>> Solution:
>>>> Replace the control edge of a pinned node before scheduling.
>>>> Adjust the control edge of a SafePointScalarObject when needed.
>>>> Added few asserts to verify scheduling correctness for
>>>> pinned nodes and SafePointScalarObject node.
>>>>
>>>> Reviewed by:
>>>>
>>>> Fix verified (y/n): y
>>>>
>>>> Other testing:
>>>> JPRT, CTW (with -XX:+DoEscapeAnalysis)
>>>>
>>>