Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8266086: [lworld][lw3] C1 produces incorrect code when GlobalValueNumbering is used #395

Closed
wants to merge 2 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -1780,7 +1780,7 @@ Value GraphBuilder::make_constant(ciConstant field_value, ciField* field) {
}
}

void GraphBuilder::copy_inline_content(ciInlineKlass* vk, Value src, int src_off, Value dest, int dest_off, ValueStack* state_before) {
void GraphBuilder::copy_inline_content(ciInlineKlass* vk, Value src, int src_off, Value dest, int dest_off, ValueStack* state_before, ciField* enclosing_field) {
assert(vk->nof_nonstatic_fields() > 0, "Empty inline type access should be removed");
for (int i = 0; i < vk->nof_nonstatic_fields(); i++) {
ciField* inner_field = vk->nonstatic_field_at(i);
@@ -1789,6 +1789,7 @@ void GraphBuilder::copy_inline_content(ciInlineKlass* vk, Value src, int src_off
LoadField* load = new LoadField(src, src_off + off, inner_field, false, state_before, false);
Value replacement = append(load);
StoreField* store = new StoreField(dest, dest_off + off, inner_field, replacement, false, state_before, false);
store->set_enclosing_field(enclosing_field);
append(store);
}
}
@@ -2053,7 +2054,7 @@ void GraphBuilder::access_field(Bytecodes::Code code) {
} else {
assert(!needs_patching, "Can't patch flattened inline type field access");
ciInlineKlass* inline_klass = field->type()->as_inline_klass();
copy_inline_content(inline_klass, val, inline_klass->first_field_offset(), obj, offset, state_before);
copy_inline_content(inline_klass, val, inline_klass->first_field_offset(), obj, offset, state_before, field);
}
break;
}
@@ -2109,7 +2110,7 @@ void GraphBuilder::withfield(int field_index) {
if (field->is_flattened()) {
ciInlineKlass* vk = field->type()->as_inline_klass();
if (!vk->is_empty()) {
copy_inline_content(vk, obj, offset, new_instance, vk->first_field_offset(), state_before);
copy_inline_content(vk, obj, offset, new_instance, vk->first_field_offset(), state_before, field);
}
} else {
LoadField* load = new LoadField(obj, offset, field, false, state_before, false);
@@ -2130,7 +2131,7 @@ void GraphBuilder::withfield(int field_index) {
assert(!needs_patching, "Can't patch flattened inline type field access");
ciInlineKlass* vk = field_modify->type()->as_inline_klass();
if (!vk->is_empty()) {
copy_inline_content(vk, val, vk->first_field_offset(), new_instance, offset_modify, state_before);
copy_inline_content(vk, val, vk->first_field_offset(), new_instance, offset_modify, state_before, field_modify);
}
} else {
StoreField* store = new StoreField(new_instance, offset_modify, field_modify, val, false, state_before, needs_patching);
@@ -295,7 +295,7 @@ class GraphBuilder {
// inline types
void default_value(int klass_index);
void withfield(int field_index);
void copy_inline_content(ciInlineKlass* vk, Value src, int src_off, Value dest, int dest_off, ValueStack* state_before);
void copy_inline_content(ciInlineKlass* vk, Value src, int src_off, Value dest, int dest_off, ValueStack* state_before, ciField* encloding_field = NULL);

// stack/code manipulation helpers
Instruction* append_with_bci(Instruction* instr, int bci);
@@ -416,6 +416,7 @@ StoreField::StoreField(Value obj, int offset, ciField* field, Value value, bool
ValueStack* state_before, bool needs_patching)
: AccessField(obj, offset, field, is_static, state_before, needs_patching)
, _value(value)
, _enclosing_field(NULL)
{
set_flag(NeedsWriteBarrierFlag, as_ValueType(field_type())->is_object());
#ifdef ASSERT
@@ -879,6 +879,7 @@ LEAF(LoadField, AccessField)
LEAF(StoreField, AccessField)
private:
Value _value;
ciField* _enclosing_field; // enclosing field (the flattened one) for nested fields

public:
// creation
@@ -888,6 +889,8 @@ LEAF(StoreField, AccessField)
// accessors
Value value() const { return _value; }
bool needs_write_barrier() const { return check_flag(NeedsWriteBarrierFlag); }
ciField* enclosing_field() const { return _enclosing_field; }
void set_enclosing_field(ciField* field) { _enclosing_field = field; }

// generic
virtual void input_values_do(ValueVisitor* f) { AccessField::input_values_do(f); f->visit(&_value); }
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1999, 2019, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1999, 2021, 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
@@ -148,6 +148,9 @@ class ValueNumberingVisitor: public InstructionVisitor {
kill_memory();
} else {
kill_field(x->field(), x->needs_patching());
if (x->enclosing_field() != NULL) {
kill_field(x->enclosing_field(), true);
Copy link
Member

@TobiHartmann TobiHartmann Apr 29, 2021

Choose a reason for hiding this comment

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

Why do you need all_offsets == true? The offset of the enclosing field should always be known, right?

}
}
}
void do_StoreIndexed (StoreIndexed* x) { kill_array(x->type()); }
@@ -0,0 +1,71 @@
/*
* Copyright (c) 2021, 2021, 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.valhalla.inlinetypes;

import jdk.test.lib.Asserts;

/*
* @test
* @summary Test value numbering behaves correctly with flattened fields
* @library /testlibrary /test/lib
* @run main/othervm -Xcomp -XX:TieredStopAtLevel=1 -ea
* -XX:CompileCommand=compileonly,compiler.valhalla.inlinetypes.TestC1ValueNumbering::*
* compiler.valhalla.inlinetypes.TestC1ValueNumbering
*/

public class TestC1ValueNumbering {
static primitive class Point {
int x,y;
public Point() {
x = 0; y = 0;
}
public Point(int x, int y) {
this.x = x; this.y = y;
}
}

Point p;

// Notes on test 1:
// 1 - asserts are important create several basic blocks (asserts create branches)
// 2 - local variables x, y must be read in the same block as the putfield
static void test1() {
Point p = new Point(4,5);
TestC1ValueNumbering test = new TestC1ValueNumbering();
assert test.p.x == 0;
assert test.p.y == 0;
test.p = p;
int x = test.p.x;
int y = test.p.y;
Asserts.assertEQ(x, 4, "Bad field value");
Asserts.assertEQ(y, 5, "Bad field value");
}

public static void main(String[] args) {
for (int i = 0; i < 10; i++) {
test1();
}
}
}