RFR(S): 8238723: yank_alloc_node must remove membar

[Nils Eliasson]

Merging the two threads.

On 2020-02-19 15:11, Vladimir Ivanov wrote:
>
>
> On 18.02.2020 20:10, Nils Eliasson wrote:
>> Hi,
>>
>> Thanks for the feedback.
>>
>> Yes, the membar should be removed too, and membar::remove gets the 
>> job done, as Tobias suggested.
>>
>> I changed the removal to only look for MembarStoreStore since the 
>> InitializeNode is already removed, and I can't come up with a 
>> scenario where I would find an AddNode there.
>>
>> http://cr.openjdk.java.net/~neliasso/8238723/webrev.02/
>
> In addition to my previous comment.
>
> Why do you think MembarStoreStore is the only possible case left?

I read this comment in CallNode::result_cast() -

"// Expected uses are restricted to a CheckCastPP, an Initialize
  // node, a MemBarStoreStore (clone) and AddP nodes. If we
  // encounter any other use (a Phi node can be seen in rare
  // cases) return this to prevent incorrect optimizations."

In webrev.02 I used "use->isa_MemBarStoreStore()->..." to make sure that 
it fails on any unexpected case (other membar types and addPs). So far I 
haven't encountered a single case, but I think your comment is 
reasonable and will change it to handle all variants.

One explanation is that when running with EA - the membar will already 
have been removed (since there is no use - thats why we are removing the 
allocation in the first place). When running without EA the membar 
redundancy check is skipped [1] - so there will be no change of membar 
types.

And to address your concern on AddPs - I still haven't seen an example 
on when they occur. How would the graph look? What is using the addP? So 
far I have been content with asserting on addPs, and still hasn't found 
any occurrence in our testing. But I want to be safe so I will change to 
skip the allocation removal if there is an addP (but keep asserting in 
debug builds)

// Nils



[1] 
http://hg.openjdk.java.net/jdk/jdk/file/tip/src/hotspot/share/opto/parse1.cpp#l1010

>
> Sometimes MemBarRelease is added [1] which can degenerate into 
> MemBarCPUOrder [2].
>
> I'd prefer to see all the barriers which are linked to the allocation 
> be removed.
>
> Best regards,
> Vladimir Ivanov
>
> [1] 
> http://hg.openjdk.java.net/jdk/jdk/file/tip/src/hotspot/share/opto/parse1.cpp#l1005
>
> [2] 
> http://hg.openjdk.java.net/jdk/jdk/file/tip/src/hotspot/share/opto/escape.cpp#l1912
>
>> On 2020-02-18 14:01, Tobias Hartmann wrote:
>>> On 18.02.20 13:32, Vladimir Ivanov wrote:
>>>> In case of MemBar I assume you are handling the precedence edge 
>>>> (MemBarNode::Precedent). Why do you
>>>> replace it with TOP? I assume you want to eliminate the barrier, 
>>>> but I don't see how it helps.
>>> Actually, you might want to use MemBarNode::remove.
>>>
>>> Best regards,
>>> Tobias