Skip to content

Commit

Permalink
8314997: Missing optimization opportunities due to missing try_clean_…
Browse files Browse the repository at this point in the history
…mem_phi() calls

Reviewed-by: roland, kvn, thartmann
  • Loading branch information
chhagedorn committed Sep 4, 2023
1 parent ab12c5d commit 2dc930d
Show file tree
Hide file tree
Showing 3 changed files with 230 additions and 58 deletions.
156 changes: 100 additions & 56 deletions src/hotspot/share/opto/cfgnode.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -447,7 +447,7 @@ void RegionNode::verify_can_be_irreducible_entry() const {
}
#endif //ASSERT

bool RegionNode::try_clean_mem_phi(PhaseGVN *phase) {
void RegionNode::try_clean_mem_phis(PhaseIterGVN* igvn) {
// Incremental inlining + PhaseStringOpts sometimes produce:
//
// cmpP with 1 top input
Expand All @@ -464,32 +464,60 @@ bool RegionNode::try_clean_mem_phi(PhaseGVN *phase) {
// replaced by If's control input but because there's still a Phi,
// the Region stays in the graph. The top input from the cmpP is
// propagated forward and a subgraph that is useful goes away. The
// code below replaces the Phi with the MergeMem so that the Region
// is simplified.

PhiNode* phi = has_unique_phi();
if (phi && phi->type() == Type::MEMORY && req() == 3 && phi->is_diamond_phi(true)) {
MergeMemNode* m = nullptr;
assert(phi->req() == 3, "same as region");
for (uint i = 1; i < 3; ++i) {
Node *mem = phi->in(i);
if (mem && mem->is_MergeMem() && in(i)->outcnt() == 1) {
// Nothing is control-dependent on path #i except the region itself.
m = mem->as_MergeMem();
uint j = 3 - i;
Node* other = phi->in(j);
if (other && other == m->base_memory()) {
// m is a successor memory to other, and is not pinned inside the diamond, so push it out.
// This will allow the diamond to collapse completely.
phase->is_IterGVN()->replace_node(phi, m);
return true;
}
}
// code in PhiNode::try_clean_memory_phi() replaces the Phi with the
// MergeMem in order to remove the Region if its last phi dies.

if (!is_diamond()) {
return;
}

for (DUIterator_Fast imax, i = fast_outs(imax); i < imax; i++) {
Node* phi = fast_out(i);
if (phi->is_Phi() && phi->as_Phi()->try_clean_memory_phi(igvn)) {
--i;
--imax;
}
}
return false;
}

// Does this region merge a simple diamond formed by a proper IfNode?
//
// Cmp
// /
// ctrl Bool
// \ /
// IfNode
// / \
// IfFalse IfTrue
// \ /
// Region
bool RegionNode::is_diamond() const {
if (req() != 3) {
return false;
}

Node* left_path = in(1);
Node* right_path = in(2);
if (left_path == nullptr || right_path == nullptr) {
return false;
}
Node* diamond_if = left_path->in(0);
if (diamond_if == nullptr || !diamond_if->is_If() || diamond_if != right_path->in(0)) {
// Not an IfNode merging a diamond or TOP.
return false;
}

// Check for a proper bool/cmp
const Node* bol = diamond_if->in(1);
if (!bol->is_Bool()) {
return false;
}
const Node* cmp = bol->in(1);
if (!cmp->is_Cmp()) {
return false;
}
return true;
}
//------------------------------Ideal------------------------------------------
// Return a node which is more "ideal" than the current node. Must preserve
// the CFG, but we can still strip out dead paths.
Expand All @@ -501,10 +529,8 @@ Node *RegionNode::Ideal(PhaseGVN *phase, bool can_reshape) {
// arm of the same IF. If found, then the control-flow split is useless.
bool has_phis = false;
if (can_reshape) { // Need DU info to check for Phi users
try_clean_mem_phis(phase->is_IterGVN());
has_phis = (has_phi() != nullptr); // Cache result
if (has_phis && try_clean_mem_phi(phase)) {
has_phis = false;
}

if (!has_phis) { // No Phi users? Nothing merging?
for (uint i = 1; i < req()-1; i++) {
Expand Down Expand Up @@ -1327,42 +1353,60 @@ const Type* PhiNode::Value(PhaseGVN* phase) const {
return ft;
}


//------------------------------is_diamond_phi---------------------------------
// Does this Phi represent a simple well-shaped diamond merge? Return the
// index of the true path or 0 otherwise.
// If check_control_only is true, do not inspect the If node at the
// top, and return -1 (not an edge number) on success.
int PhiNode::is_diamond_phi(bool check_control_only) const {
// Check for a 2-path merge
Node *region = in(0);
if( !region ) return 0;
if( region->req() != 3 ) return 0;
if( req() != 3 ) return 0;
// Check that both paths come from the same If
Node *ifp1 = region->in(1);
Node *ifp2 = region->in(2);
if( !ifp1 || !ifp2 ) return 0;
Node *iff = ifp1->in(0);
if( !iff || !iff->is_If() ) return 0;
if( iff != ifp2->in(0) ) return 0;
if (check_control_only) return -1;
// Check for a proper bool/cmp
const Node *b = iff->in(1);
if( !b->is_Bool() ) return 0;
const Node *cmp = b->in(1);
if( !cmp->is_Cmp() ) return 0;

// Check for branching opposite expected
if( ifp2->Opcode() == Op_IfTrue ) {
assert( ifp1->Opcode() == Op_IfFalse, "" );
return 2;
} else {
assert( ifp1->Opcode() == Op_IfTrue, "" );
int PhiNode::is_diamond_phi() const {
Node* region = in(0);
assert(region != nullptr && region->is_Region(), "phi must have region");
if (!region->as_Region()->is_diamond()) {
return 0;
}

if (region->in(1)->is_IfTrue()) {
assert(region->in(2)->is_IfFalse(), "bad If");
return 1;
} else {
// Flipped projections.
assert(region->in(2)->is_IfTrue(), "bad If");
return 2;
}
}

// Do the following transformation if we find the corresponding graph shape, remove the involved memory phi and return
// true. Otherwise, return false if the transformation cannot be applied.
//
// If If
// / \ / \
// IfFalse IfTrue /- Some Node IfFalse IfTrue
// \ / / / \ / Some Node
// Region / /-MergeMem ===> Region |
// / \---Phi | MergeMem
// [other phis] \ [other phis] |
// use use
bool PhiNode::try_clean_memory_phi(PhaseIterGVN* igvn) {
if (_type != Type::MEMORY) {
return false;
}
assert(is_diamond_phi() > 0, "sanity");
assert(req() == 3, "same as region");
const Node* region = in(0);
for (uint i = 1; i < 3; i++) {
Node* phi_input = in(i);
if (phi_input != nullptr && phi_input->is_MergeMem() && region->in(i)->outcnt() == 1) {
// Nothing is control-dependent on path #i except the region itself.
MergeMemNode* merge_mem = phi_input->as_MergeMem();
uint j = 3 - i;
Node* other_phi_input = in(j);
if (other_phi_input != nullptr && other_phi_input == merge_mem->base_memory()) {
// merge_mem is a successor memory to other_phi_input, and is not pinned inside the diamond, so push it out.
// This will allow the diamond to collapse completely if there are no other phis left.
igvn->replace_node(this, merge_mem);
return true;
}
}
}
return false;
}
//----------------------------check_cmove_id-----------------------------------
// Check for CMove'ing a constant after comparing against the constant.
// Happens all the time now, since if we compare equality vs a constant in
Expand Down
6 changes: 4 additions & 2 deletions src/hotspot/share/opto/cfgnode.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,8 @@ class RegionNode : public Node {
virtual Node* Ideal(PhaseGVN* phase, bool can_reshape);
void remove_unreachable_subgraph(PhaseIterGVN* igvn);
virtual const RegMask &out_RegMask() const;
bool try_clean_mem_phi(PhaseGVN* phase);
bool is_diamond() const;
void try_clean_mem_phis(PhaseIterGVN* phase);
bool optimize_trichotomy(PhaseIterGVN* igvn);
NOT_PRODUCT(virtual void dump_spec(outputStream* st) const;)
};
Expand Down Expand Up @@ -233,7 +234,8 @@ class PhiNode : public TypeNode {
LoopSafety simple_data_loop_check(Node *in) const;
// Is it unsafe data loop? It becomes a dead loop if this phi node removed.
bool is_unsafe_data_reference(Node *in) const;
int is_diamond_phi(bool check_control_only = false) const;
int is_diamond_phi() const;
bool try_clean_memory_phi(PhaseIterGVN* igvn);
virtual int Opcode() const;
virtual bool pinned() const { return in(0) != 0; }
virtual const TypePtr *adr_type() const { verify_adr_type(true); return _adr_type; }
Expand Down
126 changes: 126 additions & 0 deletions test/hotspot/jtreg/compiler/c2/irTests/igvn/TestCleanMemPhi.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
/*
* Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/
package compiler.c2.irTests.igvn;

import compiler.lib.ir_framework.*;
import jdk.test.lib.Asserts;

/*
* @test
* @bug 8314997
* @requires vm.debug == true & vm.compiler2.enabled
* @summary Test that diamond if-region is removed due to calling try_clean_mem_phi().
* @library /test/lib /
* @run driver compiler.c2.irTests.igvn.TestCleanMemPhi
*/
public class TestCleanMemPhi {
static boolean flag, flag2;
static int iFld;

public static void main(String[] args) {
TestFramework testFramework = new TestFramework();
testFramework.setDefaultWarmup(0);
testFramework.addFlags("-XX:+AlwaysIncrementalInline", "-XX:-PartialPeelLoop", "-XX:-LoopUnswitching");
testFramework.addScenarios(new Scenario(1, "-XX:-StressIGVN"),
new Scenario(2, "-XX:+StressIGVN"));
testFramework.start();
}

static class A {
int i;
}


static A a1 = new A();
static A a2 = new A();


@Test
@IR(counts = {IRNode.COUNTED_LOOP, "> 0"})
static void testCountedLoop() {
int zero = 34;

int limit = 2;
for (; limit < 4; limit *= 2) ;
for (int i = 2; i < limit; i++) {
zero = 0;
}

// Loop is not converted to a counted loop because a diamond is not removed due to missing to call
// try_clean_mem_phi() again on the diamond region.
int i = 0;
do {
iFld = 34;
if (flag2) {
iFld++;
}

int z = 34;
if (flag) {
lateInline(); // Inlined late -> leaves a MergeMem
if (zero == 34) { // False but only cleaned up after CCP
iFld = 38;
}
z = 32; // Ensures to get a diamond If-Region
}
// Region merging a proper diamond after CCP with a memory phi merging loop phi and the MergeMem from lateInline().
// Region is not added to the IGVN worklist anymore once the second phi dies.
i++;
} while (zero == 34 || (i < 1000 && a1 == a2)); // Could be converted to a counted loop after the diamond is removed after CCP.
}

@Test
@IR(failOn = IRNode.LOOP)
static void testRemoveLoop() {
int zero = 34;

int limit = 2;
for (; limit < 4; limit *= 2) ;
for (int i = 2; i < limit; i++) {
zero = 0;
}

// Loop is not converted to a counted loop and thus cannot be removed as empty loop because a diamond is not
// removed due to missing to call try_clean_mem_phi() again on the diamond region.
int i = 0;
do {
iFld = 34;

int z = 34;
if (flag) {
lateInline(); // Inlined late -> leaves a MergeMem
if (zero == 34) { // False but only cleaned up after CCP
iFld = 38;
}
z = 32; // Ensures to get a diamond If-Region
}
// Region merging a proper diamond after CCP with a memory phi merging loop phi and the MergeMem from lateInline().
// Region is not added to the IGVN worklist anymore once the second phi dies.

i++;
} while (zero == 34 || (i < 21 && a1 == a2));
}

static void lateInline() {
}
}

1 comment on commit 2dc930d

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.