RFR: 8136596: Remove MemBarRelease when final field's allocation is NoEscape or ArgEscape

[Roland Westrelin]

> Could someone please sponsor this change and push it through JPRT as it involves changes to shared code.
> 
> I have filed a JIRA report
> 
> https://bugs.openjdk.java.net/browse/JDK-8136596
> 
> and generated a Webber
> 
> http://cr.openjdk.java.net/~enevill/8136596/webrev/

I can sponsor that change. It would be nice if the mostly identical code in memnode.cpp and macro.cpp is factored in a single method in AllocateNode with a comment that explains why we need to test the status of both alloc and init.

Thanks,
Roland.

> 
> This change was contributed by hui.shi at linaro.org.
> 
> I have tested it with JTreg hotspot + langtools with no regressions and also with jcstress with no failures.
> 
> Many thanks,
> Ed.
> 
> 
> On Wed, 2015-09-16 at 06:44 +0800, Hui Shi wrote:
>> Thanks Vladimir!
>> 
>> 
>> New patch keeps original "alloc->as_Allocate()->_is_non_escaping" check with InitializeNode->does_not_escape() check.
>> Another similar place is PhaseMacroExpand::expand_allocate_common, where generate MemBarStoreStore for allocation node. If AllocateNode is not escape, it doesn't need generate MemBarStoreStore no matter its InitializeNode NULL or not.
>> 
>> 
>> diff -r 720d0ff40323 src/share/vm/opto/macro.cpp
>> --- a/src/share/vm/opto/macro.cpp Mon Sep 14 07:03:04 2015 +0000
>> +++ b/src/share/vm/opto/macro.cpp Mon Sep 14 19:33:46 2015 +0800
>> @@ -1512,7 +1512,8 @@
>>     // MemBarStoreStore so that stores that initialize this object
>>     // can't be reordered with a subsequent store that makes this
>>     // object accessible by other threads.
>> -    if (init == NULL || (!init->is_complete_with_arraycopy() && !init->does_not_escape())) {
>> +    if (!alloc->_is_non_escaping &&
>> +        (init == NULL || (!init->is_complete_with_arraycopy() && !init->does_not_escape()))) {
>>       if (init == NULL || init->req() < InitializeNode::RawStores) {
>>         // No InitializeNode or no stores captured by zeroing
>>         // elimination. Simply add the MemBarStoreStore after object
>> diff -r 720d0ff40323 src/share/vm/opto/memnode.cpp
>> --- a/src/share/vm/opto/memnode.cpp Mon Sep 14 07:03:04 2015 +0000
>> +++ b/src/share/vm/opto/memnode.cpp Mon Sep 14 19:33:46 2015 +0800
>> @@ -2944,8 +2944,11 @@
>>     } else if (opc == Op_MemBarRelease) {
>>       // Final field stores.
>>       Node* alloc = AllocateNode::Ideal_allocation(in(MemBarNode::Precedent), phase);
>> +      InitializeNode* init = NULL; 
>>       if ((alloc != NULL) && alloc->is_Allocate() &&
>> -          alloc->as_Allocate()->_is_non_escaping) {
>> +          (alloc->as_Allocate()->_is_non_escaping ||
>> +           ((init = alloc->as_Allocate()->initialization()) != NULL &&
>> +            init->does_not_escape()))) {
>>         // The allocated object does not escape.
>>         eliminate = true;
>>       }
>> 
>> 
>> 
>> 
>> Regards
>> Shi Hui
>> 
>> On 14 September 2015 at 01:37, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
>>        Please keep original AllocateNode check and add new check with ||.
>>        There are cases when there is no InitializeNode associated with allocation (when there are no access to object) so we need to keep first check.
>> 
>>        Thanks,
>>        Vladimir
>> 
>>        On 9/8/15 5:41 AM, Hui Shi wrote:
>>                Hi JIT members,
>> 
>> 
>>                There might be better use of escape analysis result when optimizing
>>                MemBarRelease node for final field stores. Could anyone help review and
>>                comments?
>> 
>>                Patch in
>>                http://people.linaro.org/~hui.shi/MemBarRelease_Escape/MemBarEscape.patch
>> 
>>                __
>> 
>>                In hotspot, there are two different escape information recorded on a node.
>> 
>>                1. AlllocateNode._is_non_escaping, true means allocation node’s escape
>>                state is noEscape.____
>> 
>>                2. InitializeNode._does_not_escape, true means its allocation node’s
>>                escape state is noEscape or ArgEscape.____
>> 
>>                NoEscape has literal meaning. ArgEscape means allocation node is passed
>>                to callee but will not escape current thread. So ArgEscape is safe to
>>                remove MemBarRelase node for final field store in initialization method.____
>> 
>>                __ __
>> 
>>                Pasted test creates an Integer instance in foo, MemBarRelase node is
>>                created at the end of initialization method as Integer's value field is
>>                final. New instance is ArgEscaped because it is passed to bar (disable
>>                inlining bar). It fails to remove MemBarRelase node now, because
>>                AlllocateNode._is_non_escaping is used in MemBarNode::Ideal(PhaseGVN
>>                *phase, bool can_reshape) and this flag is false.____
>> 
>>                __ __
>> 
>>                public class TestInteger {____
>> 
>>                   public static void main(String[] args) {____
>> 
>>                      for(int i = 0; i < 1000000; i ++)____
>> 
>>                        foo(i);____
>> 
>>                   }____
>> 
>>                __ __
>> 
>>                   static int foo(int i) {____
>> 
>>                     Integer newi = new Integer(i);____
>> 
>>                     return bar(newi);____
>> 
>>                   }____
>> 
>>                __ __
>> 
>>                   static int bar(Integer i) {____
>> 
>>                     return i.intValue() + 2;____
>> 
>>                   }____
>> 
>>                }____
>> 
>>                __ __
>> 
>>                This patch deletes MemBarRelease node when its allocation node is
>>                ArgEscape or NoEscape by checking its InitializeNode._does_not_escape
>>                flag.____
>> 
>> 
>>                Regards
>> 
>>                Shi Hui
>> 
>> 
>> 
> 
>