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

[Vladimir Kozlov]

 > It's not truly incorrect is it?  It's just not easily provably equal.  The 
fact that we're compiling it says that the locks are block structured so they 
must be equal.  For an OSR in a nested loop with locking we're just trusting 
that they are referring to the same object.

Yes, I think, for this implementation it is just difficult to prove objects 
equivalence, generated code should be correct.

Anyway, I decided to push current 7128355 fix as it is and filed separate bug:

7129618: assert(obj_node->eqv_uncast(obj),"")

Thanks,
Vladimir

Tom Rodriguez wrote:
> On Jan 12, 2012, at 1:27 PM, Vladimir Kozlov wrote:
> 
>> Tom,
>>
>> before pushing I ran additional tests (nsk.jdi) which failed during Nightly and found that they still failing next assert in BoxLockNode::is_simple_lock_region():
>>
>>  assert(obj_node->eqv_uncast(obj),"");
>>
>> http://cr.openjdk.java.net/~kvn/7128355/webrev.01/src/share/vm/opto/locknode.cpp.html
>>
>> It verifies that safepoint references the same object as locks in simple lock region. It failed because SFP references Phi node of different CheckCastPP nodes of that object so uncast() did not help. I think I can remove this safepoint verification since at that case all locks and unlocks associated with this Box are referencing the same object and it does not matter what SFP references (it should be the same object).
>>
>> But I use the same check "obj_node->eqv_uncast(obj)" when looking for safepoints in which old Box should be replaced with its clone in PhaseMacroExpand::mark_eliminated_box(). Monitor info will be incorrect when it does not match but locks are eliminated (since it is non escaped object).
>>
>> It seems, for safepoint checks I need something like eqv_uncast_and_unphi(obj) but it again become complex. Note, this problem existed - we had incorrect monitors info in safe points.
> 
> It's not truly incorrect is it?  It's just not easily provably equal.  The fact that we're compiling it says that the locks are block structured so they must be equal.  For an OSR in a nested loop with locking we're just trusting that they are referring to the same object.
> 
> tom
> 
>> I will think more what I can do. Just letting you know that I may add more code to this fix.
>>
>> thanks,
>> Vladimir
>>
>> Vladimir Kozlov wrote:
>>> Thank you, Tom
>>> Vladimir
>>> Tom Rodriguez wrote:
>>>> On Jan 11, 2012, at 5:51 PM, Vladimir Kozlov wrote:
>>>>
>>>>> 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.
>>>> I like that better.
>>>>
>>>> tom
>>>>
>>>>> 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
>