From 6df78c4585fc5a71ceafa6f4b1dc0fe68db2657c Mon Sep 17 00:00:00 2001 From: Emanuel Peter Date: Wed, 12 Nov 2025 07:10:29 +0000 Subject: [PATCH] 8371065: C2 SuperWord: VTransformLoopPhiNode::apply setting type leads to assert/wrong result Co-authored-by: Roland Westrelin Reviewed-by: qamai, chagedorn --- .../share/opto/superwordVTransformBuilder.cpp | 2 +- src/hotspot/share/opto/vtransform.cpp | 91 ++++++--- src/hotspot/share/opto/vtransform.hpp | 34 +++- .../superword/TestLoopPhiApplyBadType.java | 189 ++++++++++++++++++ 4 files changed, 283 insertions(+), 33 deletions(-) create mode 100644 test/hotspot/jtreg/compiler/loopopts/superword/TestLoopPhiApplyBadType.java diff --git a/src/hotspot/share/opto/superwordVTransformBuilder.cpp b/src/hotspot/share/opto/superwordVTransformBuilder.cpp index 45c919ccffa..832e91603d9 100644 --- a/src/hotspot/share/opto/superwordVTransformBuilder.cpp +++ b/src/hotspot/share/opto/superwordVTransformBuilder.cpp @@ -64,7 +64,7 @@ void SuperWordVTransformBuilder::build_scalar_vtnodes_for_non_packed_nodes() { const VPointer& mem_p = _vloop_analyzer.vpointers().vpointer(mem); vtn = new (_vtransform.arena()) VTransformMemopScalarNode(_vtransform, mem, mem_p); } else if (n->is_Phi()) { - vtn = new (_vtransform.arena()) VTransformLoopPhiNode(_vtransform, n->as_Phi()); + vtn = new (_vtransform.arena()) VTransformPhiScalarNode(_vtransform, n->as_Phi()); } else if (n->is_CountedLoop()) { vtn = new (_vtransform.arena()) VTransformCountedLoopNode(_vtransform, n->as_CountedLoop()); } else if (n->is_CFG()) { diff --git a/src/hotspot/share/opto/vtransform.cpp b/src/hotspot/share/opto/vtransform.cpp index c617f8415e4..e775eb60cab 100644 --- a/src/hotspot/share/opto/vtransform.cpp +++ b/src/hotspot/share/opto/vtransform.cpp @@ -62,7 +62,7 @@ void VTransformGraph::optimize(VTransform& vtransform) { // 1. Memory phi uses are not modeled, so they appear to have no use here, but must be kept alive. // 2. Similarly, some stores may not have their memory uses modeled, but need to be kept alive. // 3. Outer node with strong inputs: is a use after the loop that we must keep alive. - !(vtn->isa_LoopPhi() != nullptr || + !(vtn->isa_PhiScalar() != nullptr || vtn->is_load_or_store_in_loop() || (vtn->isa_Outer() != nullptr && vtn->has_strong_in_edge()))) { vtn->mark_dead(); @@ -123,8 +123,10 @@ bool VTransformGraph::schedule() { // Skip dead nodes if (!use->is_alive()) { continue; } - // Skip LoopPhi backedge. - if ((use->isa_LoopPhi() != nullptr || use->isa_CountedLoop() != nullptr) && use->in_req(2) == vtn) { continue; } + // Skip backedges. + if ((use->is_loop_head_phi() || use->isa_CountedLoop() != nullptr) && use->in_req(2) == vtn) { + continue; + } if (post_visited.test(use->_idx)) { continue; } if (pre_visited.test(use->_idx)) { @@ -205,7 +207,7 @@ void VTransformGraph::mark_vtnodes_in_loop(VectorSet& in_loop) const { for (int i = 0; i < _schedule.length(); i++) { VTransformNode* vtn = _schedule.at(i); // Is vtn a loop-phi? - if (vtn->isa_LoopPhi() != nullptr || + if (vtn->is_loop_head_phi() || vtn->is_load_or_store_in_loop()) { is_not_before_loop.set(vtn->_idx); continue; @@ -232,8 +234,7 @@ void VTransformGraph::mark_vtnodes_in_loop(VectorSet& in_loop) const { for (uint i = 0; i < vtn->out_strong_edges(); i++) { VTransformNode* use = vtn->out_strong_edge(i); // Or is vtn a backedge or one of its transitive defs? - if (in_loop.test(use->_idx) || - use->isa_LoopPhi() != nullptr) { + if (in_loop.test(use->_idx) || use->is_loop_head_phi()) { in_loop.set(vtn->_idx); break; } @@ -957,7 +958,7 @@ VTransformApplyResult VTransformDataScalarNode::apply(VTransformApplyState& appl return VTransformApplyResult::make_scalar(_node); } -VTransformApplyResult VTransformLoopPhiNode::apply(VTransformApplyState& apply_state) const { +VTransformApplyResult VTransformPhiScalarNode::apply(VTransformApplyState& apply_state) const { PhaseIdealLoop* phase = apply_state.phase(); Node* in0 = apply_state.transformed_node(in_req(0)); Node* in1 = apply_state.transformed_node(in_req(1)); @@ -965,19 +966,14 @@ VTransformApplyResult VTransformLoopPhiNode::apply(VTransformApplyState& apply_s phase->igvn().replace_input_of(_node, 1, in1); // Note: the backedge is hooked up later. - // The Phi's inputs may have been modified, and the types changes, - // e.g. from scalar to vector. - const Type* t = in1->bottom_type(); - _node->as_Type()->set_type(t); - phase->igvn().set_type(_node, t); - return VTransformApplyResult::make_scalar(_node); } // Cleanup backedges. In the schedule, the backedges come after their phis. Hence, // we only have the transformed backedges after the phis are already transformed. // We hook the backedges into the phis now, during cleanup. -void VTransformLoopPhiNode::apply_backedge(VTransformApplyState& apply_state) const { +void VTransformPhiScalarNode::apply_backedge(VTransformApplyState& apply_state) const { + assert(_node == apply_state.transformed_node(this), "sanity"); PhaseIdealLoop* phase = apply_state.phase(); if (_node->is_memory_phi()) { // Memory phi/backedge @@ -1219,7 +1215,7 @@ bool VTransformReductionVectorNode::requires_strict_order() const { // are performed inside the loop. bool VTransformReductionVectorNode::optimize_move_non_strict_order_reductions_out_of_loop_preconditions(VTransform& vtransform) { // We have a phi with a single use. - VTransformLoopPhiNode* phi = in_req(1)->isa_LoopPhi(); + VTransformPhiScalarNode* phi = in_req(1)->isa_PhiScalar(); if (phi == nullptr) { return false; } @@ -1284,7 +1280,7 @@ bool VTransformReductionVectorNode::optimize_move_non_strict_order_reductions_ou // All uses must be outside loop body, except for the phi. for (uint i = 0; i < current_red->out_strong_edges(); i++) { VTransformNode* use = current_red->out_strong_edge(i); - if (use->isa_LoopPhi() == nullptr && + if (use->isa_PhiScalar() == nullptr && use->isa_Outer() == nullptr) { // Should not be allowed by SuperWord::mark_reductions assert(false, "reduction has use inside loop"); @@ -1339,16 +1335,28 @@ bool VTransformReductionVectorNode::optimize_move_non_strict_order_reductions_ou VTransformNode* vtn_identity_vector = new (vtransform.arena()) VTransformReplicateNode(vtransform, vlen, bt); vtn_identity_vector->init_req(1, vtn_identity); - // Turn the scalar phi into a vector phi. - VTransformLoopPhiNode* phi = in_req(1)->isa_LoopPhi(); - VTransformNode* init = phi->in_req(1); - phi->set_req(1, vtn_identity_vector); + // Look at old scalar phi. + VTransformPhiScalarNode* phi_scalar = in_req(1)->isa_PhiScalar(); + PhiNode* old_phi = phi_scalar->node(); + VTransformNode* init = phi_scalar->in_req(1); + + TRACE_OPTIMIZE( + tty->print(" phi_scalar "); + phi_scalar->print(); + ) + + // Create new vector phi + const VTransformVectorNodeProperties properties = VTransformVectorNodeProperties::make_for_phi_vector(old_phi, vlen, bt); + VTransformPhiVectorNode* phi_vector = new (vtransform.arena()) VTransformPhiVectorNode(vtransform, 3, properties); + phi_vector->init_req(0, phi_scalar->in_req(0)); + phi_vector->init_req(1, vtn_identity_vector); + // Note: backedge comes later // Traverse down the chain of reductions, and replace them with vector_accumulators. VTransformReductionVectorNode* first_red = this; - VTransformReductionVectorNode* last_red = phi->in_req(2)->isa_ReductionVector(); + VTransformReductionVectorNode* last_red = phi_scalar->in_req(2)->isa_ReductionVector(); VTransformReductionVectorNode* current_red = first_red; - VTransformNode* current_vector_accumulator = phi; + VTransformNode* current_vector_accumulator = phi_vector; while (true) { VTransformNode* vector_input = current_red->in_req(2); VTransformVectorNode* vector_accumulator = new (vtransform.arena()) VTransformElementWiseVectorNode(vtransform, 3, current_red->properties(), vopc); @@ -1366,15 +1374,15 @@ bool VTransformReductionVectorNode::optimize_move_non_strict_order_reductions_ou } // Feed vector accumulator into the backedge. - phi->set_req(2, current_vector_accumulator); + phi_vector->set_req(2, current_vector_accumulator); // Create post-loop reduction. last_red keeps all uses outside the loop. last_red->set_req(1, init); last_red->set_req(2, current_vector_accumulator); TRACE_OPTIMIZE( - tty->print(" phi "); - phi->print(); + tty->print(" phi_scalar "); + phi_scalar->print(); tty->print(" after loop "); last_red->print(); ) @@ -1398,6 +1406,37 @@ VTransformApplyResult VTransformReductionVectorNode::apply(VTransformApplyState& return VTransformApplyResult::make_vector(vn, vn->vect_type()); } +VTransformApplyResult VTransformPhiVectorNode::apply(VTransformApplyState& apply_state) const { + PhaseIdealLoop* phase = apply_state.phase(); + Node* in0 = apply_state.transformed_node(in_req(0)); + Node* in1 = apply_state.transformed_node(in_req(1)); + + // We create a new phi node, because the type is different to the scalar phi. + PhiNode* old_phi = approximate_origin()->as_Phi(); + PhiNode* new_phi = old_phi->clone()->as_Phi(); + + phase->igvn().replace_input_of(new_phi, 0, in0); + phase->igvn().replace_input_of(new_phi, 1, in1); + // Note: the backedge is hooked up later. + + // Give the new phi node the correct vector type. + const TypeVect* vt = TypeVect::make(element_basic_type(), vector_length()); + new_phi->as_Type()->set_type(vt); + phase->igvn().set_type(new_phi, vt); + + return VTransformApplyResult::make_vector(new_phi, vt); +} + +// Cleanup backedges. In the schedule, the backedges come after their phis. Hence, +// we only have the transformed backedges after the phis are already transformed. +// We hook the backedges into the phis now, during cleanup. +void VTransformPhiVectorNode::apply_backedge(VTransformApplyState& apply_state) const { + PhaseIdealLoop* phase = apply_state.phase(); + PhiNode* new_phi = apply_state.transformed_node(this)->as_Phi(); + Node* in2 = apply_state.transformed_node(in_req(2)); + phase->igvn().replace_input_of(new_phi, 2, in2); +} + float VTransformLoadVectorNode::cost(const VLoopAnalyzer& vloop_analyzer) const { uint vlen = vector_length(); BasicType bt = element_basic_type(); @@ -1535,7 +1574,7 @@ void VTransformDataScalarNode::print_spec() const { tty->print("node[%d %s]", _node->_idx, _node->Name()); } -void VTransformLoopPhiNode::print_spec() const { +void VTransformPhiScalarNode::print_spec() const { tty->print("node[%d %s]", _node->_idx, _node->Name()); } diff --git a/src/hotspot/share/opto/vtransform.hpp b/src/hotspot/share/opto/vtransform.hpp index a30f0ff098f..a3e4977494e 100644 --- a/src/hotspot/share/opto/vtransform.hpp +++ b/src/hotspot/share/opto/vtransform.hpp @@ -79,7 +79,7 @@ class VTransform; class VTransformNode; class VTransformMemopScalarNode; class VTransformDataScalarNode; -class VTransformLoopPhiNode; +class VTransformPhiScalarNode; class VTransformCFGNode; class VTransformCountedLoopNode; class VTransformOuterNode; @@ -88,6 +88,7 @@ class VTransformElementWiseVectorNode; class VTransformCmpVectorNode; class VTransformBoolVectorNode; class VTransformReductionVectorNode; +class VTransformPhiVectorNode; class VTransformMemVectorNode; class VTransformLoadVectorNode; class VTransformStoreVectorNode; @@ -539,7 +540,7 @@ public: } virtual VTransformMemopScalarNode* isa_MemopScalar() { return nullptr; } - virtual VTransformLoopPhiNode* isa_LoopPhi() { return nullptr; } + virtual VTransformPhiScalarNode* isa_PhiScalar() { return nullptr; } virtual VTransformCountedLoopNode* isa_CountedLoop() { return nullptr; } virtual VTransformOuterNode* isa_Outer() { return nullptr; } virtual VTransformVectorNode* isa_Vector() { return nullptr; } @@ -547,6 +548,7 @@ public: virtual VTransformCmpVectorNode* isa_CmpVector() { return nullptr; } virtual VTransformBoolVectorNode* isa_BoolVector() { return nullptr; } virtual VTransformReductionVectorNode* isa_ReductionVector() { return nullptr; } + virtual VTransformPhiVectorNode* isa_PhiVector() { return nullptr; } virtual VTransformMemVectorNode* isa_MemVector() { return nullptr; } virtual VTransformLoadVectorNode* isa_LoadVector() { return nullptr; } virtual VTransformStoreVectorNode* isa_StoreVector() { return nullptr; } @@ -554,6 +556,7 @@ public: virtual bool is_load_in_loop() const { return false; } virtual bool is_load_or_store_in_loop() const { return false; } virtual const VPointer& vpointer() const { ShouldNotReachHere(); } + virtual bool is_loop_head_phi() const { return false; } virtual bool optimize(const VLoopAnalyzer& vloop_analyzer, VTransform& vtransform) { return false; } @@ -613,21 +616,24 @@ public: }; // Identity transform for loop head phi nodes. -class VTransformLoopPhiNode : public VTransformNode { +class VTransformPhiScalarNode : public VTransformNode { private: PhiNode* _node; public: - VTransformLoopPhiNode(VTransform& vtransform, PhiNode* n) : + VTransformPhiScalarNode(VTransform& vtransform, PhiNode* n) : VTransformNode(vtransform, n->req()), _node(n) { assert(_node->in(0)->is_Loop(), "phi ctrl must be Loop: %s", _node->in(0)->Name()); } - virtual VTransformLoopPhiNode* isa_LoopPhi() override { return this; } + PhiNode* node() const { return _node; } + + virtual VTransformPhiScalarNode* isa_PhiScalar() override { return this; } + virtual bool is_loop_head_phi() const override { return in_req(0)->isa_CountedLoop() != nullptr; } virtual float cost(const VLoopAnalyzer& vloop_analyzer) const override { return 0; } virtual VTransformApplyResult apply(VTransformApplyState& apply_state) const override; virtual void apply_backedge(VTransformApplyState& apply_state) const override; - NOT_PRODUCT(virtual const char* name() const override { return "LoopPhi"; };) + NOT_PRODUCT(virtual const char* name() const override { return "PhiScalar"; };) NOT_PRODUCT(virtual void print_spec() const override;) }; @@ -754,6 +760,10 @@ public: return VTransformVectorNodeProperties(first, opc, vlen, bt); } + static VTransformVectorNodeProperties make_for_phi_vector(PhiNode* phi, int vlen, BasicType bt) { + return VTransformVectorNodeProperties(phi, phi->Opcode(), vlen, bt); + } + Node* approximate_origin() const { return _approximate_origin; } int scalar_opcode() const { return _scalar_opcode; } uint vector_length() const { return _vector_length; } @@ -870,6 +880,18 @@ private: bool optimize_move_non_strict_order_reductions_out_of_loop(const VLoopAnalyzer& vloop_analyzer, VTransform& vtransform); }; +class VTransformPhiVectorNode : public VTransformVectorNode { +public: + VTransformPhiVectorNode(VTransform& vtransform, uint req, const VTransformVectorNodeProperties properties) : + VTransformVectorNode(vtransform, req, properties) {} + virtual VTransformPhiVectorNode* isa_PhiVector() override { return this; } + virtual bool is_loop_head_phi() const override { return in_req(0)->isa_CountedLoop() != nullptr; } + virtual float cost(const VLoopAnalyzer& vloop_analyzer) const override { return 0; } + virtual VTransformApplyResult apply(VTransformApplyState& apply_state) const override; + virtual void apply_backedge(VTransformApplyState& apply_state) const override; + NOT_PRODUCT(virtual const char* name() const override { return "PhiVector"; };) +}; + class VTransformMemVectorNode : public VTransformVectorNode { private: const VPointer _vpointer; // with size of the vector diff --git a/test/hotspot/jtreg/compiler/loopopts/superword/TestLoopPhiApplyBadType.java b/test/hotspot/jtreg/compiler/loopopts/superword/TestLoopPhiApplyBadType.java new file mode 100644 index 00000000000..17bc3894c28 --- /dev/null +++ b/test/hotspot/jtreg/compiler/loopopts/superword/TestLoopPhiApplyBadType.java @@ -0,0 +1,189 @@ +/* + * Copyright (c) 2025, 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 id=all-flags-1 + * @bug 8371065 + * @summary A bug in VTransformLoopPhiNode::apply led us to copy the type of phi->in(1) + * which we did not expect to ever be a constant, but always the full type range. + * Setting the phi type to that value meant that the phi would wrongly constant + * fold, and lead to wrong results. + * @run main/othervm + * -XX:+IgnoreUnrecognizedVMOptions + * -XX:CompileCommand=compileonly,*TestLoopPhiApplyBadType::test* + * -XX:CompileCommand=dontinline,*TestLoopPhiApplyBadType::notInlined + * -XX:-TieredCompilation + * -XX:-UseOnStackReplacement + * -XX:-BackgroundCompilation + * -XX:PartialPeelNewPhiDelta=1 + * -Xbatch + * -XX:+UnlockDiagnosticVMOptions + * -XX:+StressIGVN + * -XX:StressSeed=212574406 + * compiler.loopopts.superword.TestLoopPhiApplyBadType + */ + +/* + * @test id=fewer-flags-1 + * @bug 8371065 + * @run main/othervm + * -XX:+IgnoreUnrecognizedVMOptions + * -XX:CompileCommand=compileonly,*TestLoopPhiApplyBadType::test* + * -XX:CompileCommand=dontinline,*TestLoopPhiApplyBadType::notInlined + * -XX:-TieredCompilation + * -XX:-UseOnStackReplacement + * -XX:-BackgroundCompilation + * -XX:PartialPeelNewPhiDelta=1 + * -Xbatch + * -XX:+UnlockDiagnosticVMOptions + * -XX:+StressIGVN + * compiler.loopopts.superword.TestLoopPhiApplyBadType + */ + +/* + * @test id=all-flags-2 + * @bug 8371065 8371472 + * @run main/othervm + * -XX:+IgnoreUnrecognizedVMOptions + * -XX:CompileCommand=compileonly,*TestLoopPhiApplyBadType::test* + * -XX:CompileCommand=dontinline,*TestLoopPhiApplyBadType::notInlined + * -XX:-TieredCompilation + * -Xbatch + * -XX:+UnlockDiagnosticVMOptions + * -XX:+StressIGVN + * -XX:StressSeed=3497198372 + * compiler.loopopts.superword.TestLoopPhiApplyBadType + */ + +/* + * @test id=fewer-flags-2 + * @bug 8371065 8371472 + * @run main/othervm + * -XX:+IgnoreUnrecognizedVMOptions + * -XX:CompileCommand=compileonly,*TestLoopPhiApplyBadType::test* + * -XX:CompileCommand=dontinline,*TestLoopPhiApplyBadType::notInlined + * -XX:-TieredCompilation + * -Xbatch + * -XX:+UnlockDiagnosticVMOptions + * -XX:+StressIGVN + * compiler.loopopts.superword.TestLoopPhiApplyBadType + */ + +/* + * @test id=minimal-flags + * @bug 8371065 + * @run main/othervm + * -XX:+IgnoreUnrecognizedVMOptions + * -XX:CompileCommand=compileonly,*TestLoopPhiApplyBadType::test* + * -XX:CompileCommand=dontinline,*TestLoopPhiApplyBadType::notInlined + * compiler.loopopts.superword.TestLoopPhiApplyBadType + */ + +/* + * @test id=no-flags + * @bug 8371065 + * @run main compiler.loopopts.superword.TestLoopPhiApplyBadType + */ + +package compiler.loopopts.superword; + +public class TestLoopPhiApplyBadType { + public static void main(String[] args) { + for (int i = 0; i < 20_000; i++) { + int[] array = test1(); + int j; + boolean abort = false; + for (j = 0; j < array.length-2; j++) { + if (array[j] != (j | 1)) { + System.out.println("For " + j + " " + array[j]); + abort = true; + } + } + for (; j < array.length; j++) { + if (array[j] != j) { + System.out.println("For " + j + " " + array[j]); + abort = true; + } + } + if (abort) { + throw new RuntimeException("Failure"); + } + } + + int gold2 = test2(); + for (int i = 0; i < 10; i++) { + int res = test2(); + if (gold2 != res) { + throw new RuntimeException("Wrong value: " + res + " vs " + gold2); + } + } + } + + private static int[] test1() { + int limit = 2; + for (; limit < 4; limit *= 2); + int k = limit / 4; + + int[] array = new int[1000]; + int[] flags = new int[1000]; + int[] flags2 = new int[1000]; + int j; + for (j = 0; j < 10; j += k) { + + } + notInlined(array); + for (int i = 0; ; i++) { + synchronized (new Object()) {} + int ii = Integer.min(Integer.max(i, 0), array.length-1); + int v = array[ii]; + if (flags2[ii] * (j-10) != 0) { + throw new RuntimeException("never taken" + v); + } + if (i * k >= array.length - 2) { + break; + } + if (flags[ii]*(j-10) != 0) { + throw new RuntimeException("never taken"); + } + array[ii] = v | 1; + } + return array; + } + + static int test2() { + int arr[] = new int[400]; + int x = 34; + for (int i = 1; i < 50; i++) { + for (int k = 3; 201 > k; ++k) { + x += Math.min(k, arr[k - 1]); + } + } + return x; + } + + private static void notInlined(int[] array) { + for (int i = 0; i < array.length; i++) { + array[i] = i; + } + } +}