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

[Tobias Hartmann]

Vladimir, Igor, thanks for the reviews.

Best,
Tobias

On 19.08.2014 03:56, Igor Veresov wrote:
> 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