Request for reviews (M): 7128355: assert(!nocreate) failed: Cannot build a phi for a block already parsed

[Vladimir Kozlov]

Updated changes.

http://cr.openjdk.java.net/~kvn/7128355/webrev.01

Don not common BoxLock nodes and avoid creating phis of boxes. During parsing 
replace new BoxLock node with old BoxLock node at merge points. Use as_BoxLock() 
  which checks that node is BoxLockNode instead of BoxLockNode::box_node(). Call 
box_node() node only after RA which may create spills of BoxLock nodes.

Vladimir

Vladimir Kozlov wrote:
> Tom Rodriguez wrote:
>> On Jan 10, 2012, at 4:52 PM, Vladimir Kozlov wrote:
>>
>>> Tom Rodriguez wrote:
>>>> On Jan 10, 2012, at 3:36 PM, Vladimir Kozlov wrote:
>>>>> http://cr.openjdk.java.net/~kvn/7128355/webrev
>>>>>
>>>>> 7128355: assert(!nocreate) failed: Cannot build a phi for a block 
>>>>> already parsed
>>>> Doesn't that break nested lock elimination?
>>> Possible but rare since merged boxes are rare cases.
>>
>> But it would still be a bug wouldn't it?  Nested lock elimination is 
>> relying on the BoxLock identity to eliminate the useless operations 
>> for some locking region.
> 
> Why it is bug? It is only opportunity lost: nested lock will not be 
> eliminated if Box is marked as commoned.
> 
> But I agree with you, the logic become too complex. I will try to 
> replace Phi with Box node after Parse and add checks to make sure such 
> phis are not created during optimizations. I will not common boxes in 
> new code. But I want to keep EliminateNestedLocks flags to be able 
> restore previous behavior.
> 
> Thanks,
> Vladimir
> 
>>
>> I think the rules for the box lock are way too complicated after these 
>> changes.  The commoning behaviour shouldn't be conditional on 
>> EliminateNestedLocks so that we can settle on some standard set of 
>> rules for how they operate.  I think BoxLockNodes should never common 
>> and any Phi of BoxLocks should simplify into a single set BoxLock.  
>> This could be done with specific logic in PhiNode::Ideal that replaces 
>> any Phi with a single BoxLock during IGVN.  We'll have to make sure we 
>> reprocess them after parse but that doesn't seem that hard.  I think 
>> we want to maintain the invariant that after parse there aren't any 
>> Phi's of BoxLock.  That's a good rule.
>>
>> tom
>>
>>> It does not affect normal case when Box node is unique for nested 
>>> lock region. The failure I see is OSR compilations of nested loop 
>>> inside synchronized scope and compilations of irreducible loops. The 
>>> scenario where nested lock elimination will be broken is when during 
>>> first locks nodes elimination (after EA) Box node was commoned 
>>> because it has Phi user but later after CCP that Phi could be 
>>> eliminated so Box could became unique. I thought about doing box 
>>> commoning only after all optimizations done. But I concern that 
>>> during first locks elimination after EA not all safepoints will be 
>>> patched.
>>>
>>>> Two unrelated boxes that happened to participate in different Phis 
>>>> could end up commoned together wouldn't they?  Don't you want to 
>>>> perform explicit Phi simplification instead, replacing the Phi and 
>>>> all other boxes with one of the boxes feeding the Phi?
>>> For eliminating locks of non escaped objects it does not matter that 
>>> unrelated Boxes are commoned - it worked before. And I want this 
>>> because it allows to find all related safepoints. Consider case of 
>>> different lock regions of the same non escaped object.
>>>
>>> Thanks,
>>> Vladimir
>>>
>>>> tom
>>>>> Tested with full CTW and compiler regression tests.
>>>>>
>>>>> Thanks,
>>>>> Vladimir
>>