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

[Tom Rodriguez]

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
>>> 
>>> Regression after 7125896 changes. Merge Phi was not created for BoxLockNode.
>>> Other problems showed up after fixing this:
>>> - EA did not recognize BoxLock phis. EA should ignore them.
>>> - Matcher and RA does not expect box phis in debug info. Next assert was hit:
>>>   assert((op == Op_BoxLock) == jvms->is_monitor_use(i), "boxes only at monitor sites");
>>> - Lock elimination code may miss some safepoints (because they separated by additional phi nodes) when replacing old box (which could be Phi) with new "eliminated" Box. As result debug info could be incorrect.
>>> 
>>> Do commoning of merged Box nodes (which has Phi uses) before locks elimination to resolve these problems.
>> 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.

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