[9] RFR(S): 8048879: "unexpected yanked node" opto/postaloc.cpp:139

[Igor Veresov]

Looks good.

igor

On Aug 18, 2014, at 3:31 AM, Tobias Hartmann <tobias.hartmann at oracle.com> wrote:

> Hi Vladimir,
> 
> thanks for the review.
> 
>> set_req_X() places dead store on _worklist. Look on remove_globally_dead_node() at lines 1246-1248. You can add new check into has_special_unique_user() to place membar on worklist.
> 
> Right, that's a better solution. I added a new check for a LoadNode uniquely used by a MemBarAcquireNode.
> 
>> You don't need next test's flags:
>> -Xbootclasspath/a:. -server
>> 
>> -server should not be specified in jtreg tests.
> 
> I removed the flags.
> 
> Tested with the failing test and JPRT.
> 
> New webrev: http://cr.openjdk.java.net/~thartmann/8048879/webrev.01/
> 
> Thanks,
> Tobias
> 
>> 
>> Thanks,
>> Vladimir
>> 
>> On 8/17/14 11:07 PM, Tobias Hartmann wrote:
>>> Hi,
>>> 
>>> please review the following patch that fixes JDK-8048879.
>>> 
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8048879
>>> Webrev: http://cr.openjdk.java.net/~thartmann/8048879/webrev.00/
>>> 
>>> == Problem ==
>>> The problem is caused by the following steps:
>>> 1) A volatile field access adds a LoadNNode (35) to the graph that is connected to a MemBarAcquireNode (37) to prevent
>>> following loads from floating up past. The result of the load is used by a StoreNNode (44) (see [1]).
>>> 2) During optimization the StoreNNode (44) is removed because a following StoreNNode (55) writes to the same memory
>>> location. The LoadNNode (35) is now useless but still connected to the MemBarAcquireNode (see [2]).
>>> 3) During matching a loadConP_load (9) is added together with a MachConstantBaseNode (10) to compute the address for the
>>> LoadNNode (see [3]).
>>> 4) During register allocation the assert in 'PhaseChaitin::yank_if_dead_recurse' is hit because a dead
>>> MachConstantBaseNode is not expected at this point.
>>> 
>>> The unused LoadNNode (35) should have been removed during IGVN. However, the corresponding optimization in
>>> 'MemBarNode::Ideal()' (see line 2954 of memnode.cpp) does not apply because at this point the LoadNNode (35) is still
>>> connected to the StoreNNode (44). After removing the StoreNNode (44) the MemBarAcquireNode (37) should be added to the
>>> IGVN worklist to be processed again.
>>> 
>>> == Solution ==
>>> The implementation of 'StoreNode::Ideal' is changed to add the users of the LoadNode to the IGVN worklist if the
>>> StoreNode is removed. This allows MemBarNode::Ideal to remove the LoadNode from its precedent input if it is dead (see [4]).
>>> 
>>> I added a jtreg test 'TestMemBarAcquire.java' that triggers the optimization. However, the test does not trigger the
>>> original bug because if 'PhaseChaitin::yank_if_dead_recurse' is executed on the MachConstantBaseNode highly depends on
>>> the execution of the register allocator. The graphs [1-3] correspond to an execution of the test without the fix. Graph [4]
>>> 
>>> == Testing ==
>>> - Failing test (Weblogic + medrec)
>>> - JPRT
>>> 
>>> Thanks,
>>> Tobias
>>> 
>>> [1] https://bugs.openjdk.java.net/secure/attachment/21801/graph_after_parsing.png
>>> [2] https://bugs.openjdk.java.net/secure/attachment/21799/graph_after_IGVN.png
>>> [3] https://bugs.openjdk.java.net/secure/attachment/21802/graph_before_reg_alloc.png
>>> [4] https://bugs.openjdk.java.net/secure/attachment/21800/graph_after_IGVN_fixed.png
>