RFR: 8136596: Remove MemBarRelease when final field's allocation is NoEscape or ArgEscape
Hui Shi
hui.shi at linaro.org
Fri Sep 18 16:06:28 UTC 2015
Hi Roland,
comments is added for "bool AllocatedNode.does_not_escape_thread()".
Please help check updated patch.
http://people.linaro.org/~hui.shi/MemBarRelease_Escape/MemBarEscape_v3.patch
diff -r e3201914b83b src/share/vm/opto/callnode.hpp
--- a/src/share/vm/opto/callnode.hpp Fri Sep 18 10:11:11 2015 +0200
+++ b/src/share/vm/opto/callnode.hpp Fri Sep 18 16:03:04 2015 +0000
@@ -907,6 +907,16 @@
// Convenience for initialization->maybe_set_complete(phase)
bool maybe_set_complete(PhaseGVN* phase);
+
+ // Return true if allocation doesn't escape thread, its escape state need be
+ // noEscape or ArgEscape. InitializeNode._does_not_escape is true when its
+ // allocation's escape state is noEscape or ArgEscape. In case allocation's
+ // InitializeNode is NULL, need check AlllocateNode._is_non_escaping flag.
+ // AlllocateNode._is_non_escaping is true when its escape state is noEscape.
+ bool does_not_escape_thread() {
+ InitializeNode* init = NULL;
+ return _is_non_escaping || (((init = initialization()) != NULL)
&& init->does_not_escape());
+ }
};
//------------------------------AllocateArray---------------------------------
diff -r e3201914b83b src/share/vm/opto/macro.cpp
--- a/src/share/vm/opto/macro.cpp Fri Sep 18 10:11:11 2015 +0200
+++ b/src/share/vm/opto/macro.cpp Fri Sep 18 16:03:04 2015 +0000
@@ -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->does_not_escape_thread() &&
+ (init == NULL || !init->is_complete_with_arraycopy())) {
if (init == NULL || init->req() < InitializeNode::RawStores) {
// No InitializeNode or no stores captured by zeroing
// elimination. Simply add the MemBarStoreStore after object
diff -r e3201914b83b src/share/vm/opto/memnode.cpp
--- a/src/share/vm/opto/memnode.cpp Fri Sep 18 10:11:11 2015 +0200
+++ b/src/share/vm/opto/memnode.cpp Fri Sep 18 16:03:04 2015 +0000
@@ -2945,7 +2945,7 @@
// Final field stores.
Node* alloc =
AllocateNode::Ideal_allocation(in(MemBarNode::Precedent), phase);
if ((alloc != NULL) && alloc->is_Allocate() &&
- alloc->as_Allocate()->_is_non_escaping) {
+ alloc->as_Allocate()->does_not_escape_thread()) {
// The allocated object does not escape.
eliminate = true;
}
Regards
Shi Hui
On 18 September 2015 at 22:49, Roland Westrelin <roland.westrelin at oracle.com
> wrote:
>
> > On Sep 18, 2015, at 4:43 PM, Hui Shi <hui.shi at linaro.org> wrote:
> >
> > Thanks Edward!
> > Thanks for sponsoring Roland! New patch is pasted below
>
> Thanks.
>
> >
> > Add small method "bool AllocatedNode.does_not_escape_thread()", return
> true when allocate node will not escape thread. Patch looks clearer. In
> PhaseMacroExpand::expand_allocate_common, InitializationNode is already
> there, InitializationNode might be get again in AllocatedNode
> .does_not_escape_thread(). Is this acceptable?
>
> I think it’s ok.
> A comment that explains why we need to look at both the AllocateNode and
> InitializeNode would be nice.
>
> Roland.
>
> >
> >
> http://people.linaro.org/~hui.shi/MemBarRelease_Escape/MemBarEscape_v2.patch
> >
> > diff -r 476739c20b35 src/share/vm/opto/callnode.hpp
> > --- a/src/share/vm/opto/callnode.hpp Thu Sep 17 13:42:50 2015 -0700
> > +++ b/src/share/vm/opto/callnode.hpp Fri Sep 18 15:27:46 2015 +0800
> > @@ -907,6 +907,11 @@
> >
> > // Convenience for initialization->maybe_set_complete(phase)
> > bool maybe_set_complete(PhaseGVN* phase);
> > +
> > + bool does_not_escape_thread() {
> > + InitializeNode* init = NULL;
> > + return _is_non_escaping || (((init = initialization()) != NULL) &&
> init->does_not_escape());
> > + }
> > };
> >
> >
> //------------------------------AllocateArray---------------------------------
> > diff -r 476739c20b35 src/share/vm/opto/macro.cpp
> > --- a/src/share/vm/opto/macro.cpp Thu Sep 17 13:42:50 2015 -0700
> > +++ b/src/share/vm/opto/macro.cpp Fri Sep 18 15:27: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->does_not_escape_thread() &&
> > + (init == NULL || !init->is_complete_with_arraycopy())) {
> > if (init == NULL || init->req() < InitializeNode::RawStores) {
> > // No InitializeNode or no stores captured by zeroing
> > // elimination. Simply add the MemBarStoreStore after object
> > diff -r 476739c20b35 src/share/vm/opto/memnode.cpp
> > --- a/src/share/vm/opto/memnode.cpp Thu Sep 17 13:42:50 2015 -0700
> > +++ b/src/share/vm/opto/memnode.cpp Fri Sep 18 15:27:46 2015 +0800
> > @@ -2945,7 +2945,7 @@
> > // Final field stores.
> > Node* alloc =
> AllocateNode::Ideal_allocation(in(MemBarNode::Precedent), phase);
> > if ((alloc != NULL) && alloc->is_Allocate() &&
> > - alloc->as_Allocate()->_is_non_escaping) {
> > + alloc->as_Allocate()->does_not_escape_thread()) {
> > // The allocated object does not escape.
> > eliminate = true;
> > }
> >
> > Regards
> > Shi Hui
> >
> > On 17 September 2015 at 21:44, Roland Westrelin <
> roland.westrelin at oracle.com> wrote:
> >
> >
> > > 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
> > >>
> > >>
> > >>
> > >
> > >
> >
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20150919/72ba7eb6/attachment-0001.html>
More information about the hotspot-compiler-dev
mailing list