From bad6999634686dcfd04c88ddab855aa202cf35b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Lund=C3=A9n?= Date: Wed, 15 Nov 2023 09:19:15 +0000 Subject: [PATCH] 8313672: C2: PhaseCCP does not correctly track analysis dependencies involving shift, convert, and mask Reviewed-by: epeter, rcastanedalo, thartmann --- src/hotspot/share/opto/castnode.hpp | 22 ------- src/hotspot/share/opto/node.hpp | 36 +++++++++++ src/hotspot/share/opto/phaseX.cpp | 13 ++-- .../ccp/TestShiftConvertAndNotification.java | 64 +++++++++++++++++++ 4 files changed, 109 insertions(+), 26 deletions(-) create mode 100644 test/hotspot/jtreg/compiler/ccp/TestShiftConvertAndNotification.java diff --git a/src/hotspot/share/opto/castnode.hpp b/src/hotspot/share/opto/castnode.hpp index d3a41115ac4..82f54eb0289 100644 --- a/src/hotspot/share/opto/castnode.hpp +++ b/src/hotspot/share/opto/castnode.hpp @@ -80,28 +80,6 @@ public: Node* optimize_integer_cast(PhaseGVN* phase, BasicType bt); - // Visit all non-cast uses of the node, bypassing ConstraintCasts. - // Pattern: this (-> ConstraintCast)* -> non_cast - // In other words: find all non_cast nodes such that - // non_cast->uncast() == this. - template - static void visit_uncasted_uses(const Node* n, Callback callback) { - ResourceMark rm; - Unique_Node_List internals; - internals.push((Node*)n); // start traversal - for (uint j = 0; j < internals.size(); ++j) { - Node* internal = internals.at(j); // for every internal - for (DUIterator_Fast kmax, k = internal->fast_outs(kmax); k < kmax; k++) { - Node* internal_use = internal->fast_out(k); - if (internal_use->is_ConstraintCast()) { - internals.push(internal_use); // traverse this cast also - } else { - callback(internal_use); - } - } - } - } - bool higher_equal_types(PhaseGVN* phase, const Node* other) const; int extra_types_count() const { diff --git a/src/hotspot/share/opto/node.hpp b/src/hotspot/share/opto/node.hpp index 1866ef87def..0541409a312 100644 --- a/src/hotspot/share/opto/node.hpp +++ b/src/hotspot/share/opto/node.hpp @@ -1128,6 +1128,13 @@ public: // Set control or add control as precedence edge void ensure_control_or_add_prec(Node* c); + // Visit boundary uses of the node and apply a callback function for each. + // Recursively traverse uses, stopping and applying the callback when + // reaching a boundary node, defined by is_boundary. Note: the function + // definition appears after the complete type definition of Node_List. + template + void visit_uses(Callback callback, Check is_boundary) const; + //----------------- Code Generation // Ideal register class for Matching. Zero means unmatched instruction @@ -1628,6 +1635,35 @@ public: void dump_simple() const; }; +// Definition must appear after complete type definition of Node_List +template +void Node::visit_uses(Callback callback, Check is_boundary) const { + ResourceMark rm; + VectorSet visited; + Node_List worklist; + + // The initial worklist consists of the direct uses + for (DUIterator_Fast kmax, k = fast_outs(kmax); k < kmax; k++) { + Node* out = fast_out(k); + if (!visited.test_set(out->_idx)) { worklist.push(out); } + } + + while (worklist.size() > 0) { + Node* use = worklist.pop(); + // Apply callback on boundary nodes + if (is_boundary(use)) { + callback(use); + } else { + // Not a boundary node, continue search + for (DUIterator_Fast kmax, k = use->fast_outs(kmax); k < kmax; k++) { + Node* out = use->fast_out(k); + if (!visited.test_set(out->_idx)) { worklist.push(out); } + } + } + } +} + + //------------------------------Unique_Node_List------------------------------- class Unique_Node_List : public Node_List { friend class VMStructs; diff --git a/src/hotspot/share/opto/phaseX.cpp b/src/hotspot/share/opto/phaseX.cpp index efd9c68f05a..af293bee9e6 100644 --- a/src/hotspot/share/opto/phaseX.cpp +++ b/src/hotspot/share/opto/phaseX.cpp @@ -1606,7 +1606,9 @@ void PhaseIterGVN::add_users_to_worklist( Node *n ) { _worklist.push(n); } }; - ConstraintCastNode::visit_uncasted_uses(use, push_the_uses_to_worklist); + auto is_boundary = [](Node* n){ return !n->is_ConstraintCast(); }; + use->visit_uses(push_the_uses_to_worklist, is_boundary); + } // If changed LShift inputs, check RShift users for useless sign-ext if( use_op == Op_LShiftI ) { @@ -1832,7 +1834,7 @@ void PhaseCCP::verify_analyze(Unique_Node_List& worklist_verify) { // We should either make sure that these nodes are properly added back to the CCP worklist // in PhaseCCP::push_child_nodes_to_worklist() to update their type or add an exception // in the verification code above if that is not possible for some reason (like Load nodes). - assert(!failure, "Missed optimization opportunity in PhaseCCP"); + assert(!failure, "PhaseCCP not at fixpoint: analysis result may be unsound."); } #endif @@ -1974,7 +1976,7 @@ void PhaseCCP::push_load_barrier(Unique_Node_List& worklist, const BarrierSetC2* // AndI/L::Value() optimizes patterns similar to (v << 2) & 3 to zero if they are bitwise disjoint. // Add the AndI/L nodes back to the worklist to re-apply Value() in case the shift value changed. -// Pattern: parent -> LShift (use) -> ConstraintCast* -> And +// Pattern: parent -> LShift (use) -> (ConstraintCast | ConvI2L)* -> And void PhaseCCP::push_and(Unique_Node_List& worklist, const Node* parent, const Node* use) const { uint use_op = use->Opcode(); if ((use_op == Op_LShiftI || use_op == Op_LShiftL) @@ -1985,7 +1987,10 @@ void PhaseCCP::push_and(Unique_Node_List& worklist, const Node* parent, const No push_if_not_bottom_type(worklist, n); } }; - ConstraintCastNode::visit_uncasted_uses(use, push_and_uses_to_worklist); + auto is_boundary = [](Node* n) { + return !(n->is_ConstraintCast() || n->Opcode() == Op_ConvI2L); + }; + use->visit_uses(push_and_uses_to_worklist, is_boundary); } } diff --git a/test/hotspot/jtreg/compiler/ccp/TestShiftConvertAndNotification.java b/test/hotspot/jtreg/compiler/ccp/TestShiftConvertAndNotification.java new file mode 100644 index 00000000000..7ce44633670 --- /dev/null +++ b/test/hotspot/jtreg/compiler/ccp/TestShiftConvertAndNotification.java @@ -0,0 +1,64 @@ +/* + * 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. + */ + +/* + * @test + * @bug 8313672 + * @summary Test CCP notification for value update of AndL through LShiftI and + * ConvI2L. + * @run main/othervm -XX:+UnlockDiagnosticVMOptions + * -XX:RepeatCompilation=20 -XX:-TieredCompilation + * -XX:+StressIGVN -Xcomp + * -XX:CompileCommand=compileonly,compiler.ccp.TestShiftConvertAndNotification::test + * compiler.ccp.TestShiftConvertAndNotification + */ + +/* + * @test + * @bug 8313672 + * @summary Test CCP notification for value update of AndL through LShiftI and + * ConvI2L (no flags). + * @run driver compiler.ccp.TestShiftConvertAndNotification + * + */ + +package compiler.ccp; + +public class TestShiftConvertAndNotification { + static long instanceCount; + static void test() { + int i, i1 = 7; + for (i = 7; i < 45; i++) { + instanceCount = i; + instanceCount &= i1 * i << i * Math.max(instanceCount, instanceCount); + switch (i % 2) { + case 8: + i1 = 0; + } + } + } + public static void main(String[] strArr) { + for (int i = 0; i < 20_000; i++) + test(); + } +}