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

[Tobias Hartmann]

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