[aarch64-port-dev ] RFR: Bulk integration of Shenandoah 2018-05-15
Aleksey Shipilev
shade at redhat.com
Mon Jun 4 16:57:30 UTC 2018
On 06/04/2018 06:30 PM, Andrew Haley wrote:
> On 05/31/2018 09:37 AM, Roman Kennke wrote:
>> Has anybody else gotten to review this in the meantime? Is it ok to go?
>
> I had a look. I didn't scan the Shenadoah changes, but the shared code
> changes mostly look innocuous. I didn't look at the tests either.
Thank you!
Rearranging replies a bit:
=== Real changes
--- This change is safe: exclude_fp is false by default, and it is true on some Shenandoah-specific
paths. We can guard the "if (exclude_fp)" with additional UseShenandoahGC, but it looks like overkill.
> +++ b/src/share/vm/opto/lcm.cpp
> @@ -638,9 +638,12 @@
>
> //------------------------------add_call_kills-------------------------------------
> // helper function that adds caller save registers to MachProjNode
> -static void add_call_kills(MachProjNode *proj, RegMask& regs, const char* save_policy, bool exclude_soe) {
> +static void add_call_kills(MachProjNode *proj, RegMask& regs, const char* save_policy, bool exclude_soe, bool exclude_fp) {
> // Fill in the kill mask for the call
> for( OptoReg::Name r = OptoReg::Name(0); r < _last_Mach_Reg; r=OptoReg::add(r,1) ) {
> + if (exclude_fp && (register_save_type[r] == Op_RegF || register_save_type[r] == Op_RegD)) {
> + continue;
> + }
>
> @@ -930,7 +938,7 @@
> map_node_to_block(proj, block);
> block->insert_node(proj, phi_cnt++);
>
> - add_call_kills(proj, regs, _matcher._c_reg_save_policy, false);
> + add_call_kills(proj, regs, _matcher._c_reg_save_policy, false, false);
> }
=== Reverts of Shenandoah-specific code
--- These blocks, including all these has_only_data_users(), were added long ago for Shenandoah, and
this is the revert to upstream state -- not sure why weren't they protected with UseShenandoahGC
> diff --git a/src/share/vm/opto/cfgnode.cpp b/src/share/vm/opto/cfgnode.cpp
> --- a/src/share/vm/opto/cfgnode.cpp
> +++ b/src/share/vm/opto/cfgnode.cpp
> @@ -1153,20 +1153,6 @@
> if (id != NULL) return id;
> }
>
> - if (phase->is_IterGVN()) {
> - // A memory Phi could only have data inputs if that Phi was input
> - // to a MergeMem and MergeMemNode::Ideal() found that it's
> - // equivalent to the base memory Phi. If data uses are removed,
> - // the Phi will be removed as well. If one of the Phi inputs has
> - // changed in the meantime (shenandoah write barrier moved out of
> - // loop for instance), that input is disconnected from the memory
> - // graph.
> - Node* other_phi = has_only_data_users();
> - if (other_phi != NULL) {
> - return other_phi;
> - }
> - }
> -
> return this; // No identity
> }
> @@ -2122,30 +2108,6 @@
> _dest_bci == ((JumpProjNode&)n)._dest_bci;
> }
>
> -PhiNode* PhiNode::has_only_data_users() const {
> - if (!UseShenandoahGC || bottom_type() != Type::MEMORY || adr_type() == TypePtr::BOTTOM || outcnt() == 0) {
> - return NULL;
> - }
> - for (DUIterator_Fast imax, i = fast_outs(imax); i < imax; i++) {
> - Node* u = fast_out(i);
> - if (u->Opcode() != Op_ShenandoahReadBarrier) {
> - return NULL;
> - }
> - }
> - Node* r = in(0);
> - if (r == NULL) {
> - return NULL;
> - }
> - for (DUIterator_Fast imax, i = r->fast_outs(imax); i < imax; i++) {
> - Node* u = r->fast_out(i);
> - if (u != this && u->is_Phi() && u->bottom_type() == Type::MEMORY &&
> - u->adr_type() == TypePtr::BOTTOM) {
> - return u->as_Phi();
> - }
> - }
> - return NULL;
> -}
> -
> #ifndef PRODUCT
> void JumpProjNode::dump_spec(outputStream *st) const {
> ProjNode::dump_spec(st);
> diff --git a/src/share/vm/opto/cfgnode.hpp b/src/share/vm/opto/cfgnode.hpp
> --- a/src/share/vm/opto/cfgnode.hpp
> +++ b/src/share/vm/opto/cfgnode.hpp
> @@ -220,8 +220,6 @@
> #else //ASSERT
> void verify_adr_type(bool recursive = false) const {}
> #endif //ASSERT
> -
> - PhiNode* has_only_data_users() const;
> };
> //------------------------------GotoNode---------------------------------------
> diff --git a/src/share/vm/opto/compile.cpp b/src/share/vm/opto/compile.cpp
> --- a/src/share/vm/opto/compile.cpp
> +++ b/src/share/vm/opto/compile.cpp
> @@ -416,9 +416,6 @@
> record_for_igvn(n->fast_out(i));
> }
> }
> - if (n->is_Phi() && n->as_Phi()->has_only_data_users()) {
> - record_for_igvn(n);
> - }
> }
> // Remove useless macro and predicate opaq nodes
> for (int i = C->macro_count()-1; i >= 0; i--) {
> @@ -3149,7 +3146,6 @@
> n->subsume_by(unique_in, this);
> }
> }
> - assert(!n->as_Phi()->has_only_data_users(), "memory Phi has no memory use");
> break;
>
> #endif
> diff --git a/src/share/vm/opto/node.cpp b/src/share/vm/opto/node.cpp
> --- a/src/share/vm/opto/node.cpp
> +++ b/src/share/vm/opto/node.cpp
> @@ -1406,8 +1406,6 @@
> igvn->add_users_to_worklist( n );
> } else if (n->Opcode() == Op_AddP && CallLeafNode::has_only_g1_wb_pre_uses(n)) {
> igvn->add_users_to_worklist(n);
> - } else if (n->is_Phi() && n->as_Phi()->has_only_data_users()) {
> - igvn->_worklist.push(n);
> }
> }
> }
> diff --git a/src/share/vm/opto/phaseX.cpp b/src/share/vm/opto/phaseX.cpp
> --- a/src/share/vm/opto/phaseX.cpp
> +++ b/src/share/vm/opto/phaseX.cpp
> @@ -1300,8 +1300,6 @@
> _worklist.push(in);
> } else if (in->Opcode() == Op_AddP && CallLeafNode::has_only_g1_wb_pre_uses(in)) {
> add_users_to_worklist(in);
> - } else if (in->is_Phi() && in->as_Phi()->has_only_data_users()) {
> - _worklist.push(in);
> }
> if (ReduceFieldZeroing && dead->is_Load() && i == MemNode::Memory &&
> in->is_Proj() && in->in(0) != NULL && in->in(0)->is_Initialize()) {
> @@ -1978,9 +1976,6 @@
> if (old->Opcode() == Op_AddP && CallLeafNode::has_only_g1_wb_pre_uses(old)) {
> igvn->add_users_to_worklist(old);
> }
> - if (old->is_Phi() && old->as_Phi()->has_only_data_users()) {
> - igvn->_worklist.push(old);
> - }
> }
>
> }
>
--- This is the revert to upstream state, see:
http://hg.openjdk.java.net/jdk8u/jdk8u/hotspot/file/64fe89b445cd/src/share/vm/opto/memnode.cpp#l4379
> diff --git a/src/share/vm/opto/memnode.cpp b/src/share/vm/opto/memnode.cpp
> --- a/src/share/vm/opto/memnode.cpp
> +++ b/src/share/vm/opto/memnode.cpp
> @@ -4229,11 +4229,7 @@
> // Warning: Do not combine this "if" with the previous "if"
> // A memory slice might have be be rewritten even if it is semantically
> // unchanged, if the base_memory value has changed.
> - if (can_reshape) {
> - set_req_X(i, new_in, phase->is_IterGVN());
> - } else {
> - set_req(i, new_in);
> - }
> + set_req(i, new_in);
> progress = this; // Report progress
> }
> }
Thanks,
-Aleksey
More information about the aarch64-port-dev
mailing list