infernap12/jdk
1
0
Fork 0
mirror of https://github.com/openjdk/jdk.git synced 2026-03-14 18:03:44 +00:00
Code Issues Packages Projects Releases Wiki Activity

8371065: C2 SuperWord: VTransformLoopPhiNode::apply setting type leads to assert/wrong result

Browse Source 
Co-authored-by: Roland Westrelin <roland@openjdk.org>
Reviewed-by: qamai, chagedorn
This commit is contained in:
Emanuel Peter 2025-11-12 07:10:29 +00:00
parent 15dcbf0bc8
commit 6df78c4585
4 changed files with 283 additions and 33 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
src/hotspot/share/opto/superwordVTransformBuilder.cpp
View File

@ -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()) {

91
src/hotspot/share/opto/vtransform.cpp
View File

@ -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());
}

34
src/hotspot/share/opto/vtransform.hpp
View File

@ -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

189
test/hotspot/jtreg/compiler/loopopts/superword/TestLoopPhiApplyBadType.java Normal file
View File

@ -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;
}
}
}