Skip to content

8347006: LoadRangeNode floats above array guard in arraycopy intrinsic #22967

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

8347006: LoadRangeNode floats above array guard in arraycopy intrinsic #22967

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
425bbb6
8347006: LoadRangeNode floats above array guard in arraycopy intrinsic
TobiHartmann Jan 8, 2025
5c0292a
Added missing stopped checks, refactoring and updated copyright dates
TobiHartmann Jan 8, 2025
3b465a4
Moved cast into guard
TobiHartmann Jan 9, 2025
0a1fe38
Copyright date
TobiHartmann Jan 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 18 additions & 18 deletions src/hotspot/share/opto/library_call.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4257,7 +4257,7 @@ bool LibraryCallKit::inline_native_subtype_check() {

//---------------------generate_array_guard_common------------------------
Node* LibraryCallKit::generate_array_guard_common(Node* kls, RegionNode* region,
bool obj_array, bool not_array) {
bool obj_array, bool not_array, Node** obj) {

if (stopped()) {
return nullptr;
Expand Down Expand Up @@ -4299,7 +4299,14 @@ Node* LibraryCallKit::generate_array_guard_common(Node* kls, RegionNode* region,
// invert the test if we are looking for a non-array
if (not_array) btest = BoolTest(btest).negate();
Node* bol = _gvn.transform(new BoolNode(cmp, btest));
return generate_fair_guard(bol, region);
Node* ctrl = generate_fair_guard(bol, region);
Node* is_array_ctrl = not_array ? control() : ctrl;
if (obj != nullptr && is_array_ctrl != nullptr && is_array_ctrl != top()) {
// Keep track of the fact that 'obj' is an array to prevent
// array specific accesses from floating above the guard.
*obj = _gvn.transform(new CastPPNode(is_array_ctrl, *obj, TypeAryPtr::BOTTOM));
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we do this for above code when layout is known for compiler (layout_con is checked)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about this as well but I don't think it's necessary because:

  • No cast is needed if we know the type already
  • We don't emit a guard and only one branch remains, so there is no risk of the array specific access floating above

So I went with the simplest changes for now, also since we need to backport this change (it got already much more complicated than I was aiming for).

}
return ctrl;
}


Expand Down Expand Up @@ -4394,7 +4401,7 @@ bool LibraryCallKit::inline_native_getLength() {
if (stopped()) return true;

// Deoptimize if it is a non-array.
Node* non_array = generate_non_array_guard(load_object_klass(array), nullptr);
Node* non_array = generate_non_array_guard(load_object_klass(array), nullptr, &array);

if (non_array != nullptr) {
PreserveJVMState pjvms(this);
Expand Down Expand Up @@ -5254,12 +5261,13 @@ bool LibraryCallKit::inline_native_clone(bool is_virtual) {
record_for_igvn(result_reg);

Node* obj_klass = load_object_klass(obj);
Node* array_ctl = generate_array_guard(obj_klass, (RegionNode*)nullptr);
Node* array_obj = obj;
Node* array_ctl = generate_array_guard(obj_klass, (RegionNode*)nullptr, &array_obj);
if (array_ctl != nullptr) {
// It's an array.
PreserveJVMState pjvms(this);
set_control(array_ctl);
Node* obj_length = load_array_length(obj);
Node* obj_length = load_array_length(array_obj);
Node* array_size = nullptr; // Size of the array without object alignment padding.
Node* alloc_obj = new_array(obj_klass, obj_length, 0, &array_size, /*deoptimize_on_exception=*/true);

Expand All @@ -5273,7 +5281,7 @@ bool LibraryCallKit::inline_native_clone(bool is_virtual) {
set_control(is_obja);
// Generate a direct call to the right arraycopy function(s).
// Clones are always tightly coupled.
ArrayCopyNode* ac = ArrayCopyNode::make(this, true, obj, intcon(0), alloc_obj, intcon(0), obj_length, true, false);
ArrayCopyNode* ac = ArrayCopyNode::make(this, true, array_obj, intcon(0), alloc_obj, intcon(0), obj_length, true, false);
ac->set_clone_oop_array();
Node* n = _gvn.transform(ac);
assert(n == ac, "cannot disappear");
Expand All @@ -5294,7 +5302,7 @@ bool LibraryCallKit::inline_native_clone(bool is_virtual) {
// the object.)

if (!stopped()) {
copy_to_clone(obj, alloc_obj, array_size, true);
copy_to_clone(array_obj, alloc_obj, array_size, true);

// Present the results of the copy.
result_reg->init_req(_array_path, control());
Expand Down Expand Up @@ -5915,16 +5923,8 @@ bool LibraryCallKit::inline_arraycopy() {
record_for_igvn(slow_region);

// (1) src and dest are arrays.
// Keep track of the information that src/dest are arrays to prevent below array specific accesses from floating above.
generate_non_array_guard(load_object_klass(src), slow_region);
if (!stopped()) {
src = _gvn.transform(new CheckCastPPNode(control(), src, TypeAryPtr::BOTTOM));
}

generate_non_array_guard(load_object_klass(dest), slow_region);
if (!stopped()) {
dest = _gvn.transform(new CheckCastPPNode(control(), dest, TypeAryPtr::BOTTOM));
}
generate_non_array_guard(load_object_klass(src), slow_region, &src);
generate_non_array_guard(load_object_klass(dest), slow_region, &dest);

// (2) src and dest arrays must have elements of the same BasicType
// done at macro expansion or at Ideal transformation time
Expand Down Expand Up @@ -8540,7 +8540,7 @@ bool LibraryCallKit::inline_getObjectSize() {
PhiNode* result_val = new PhiNode(result_reg, TypeLong::LONG);
record_for_igvn(result_reg);

Node* array_ctl = generate_array_guard(klass_node, nullptr);
Node* array_ctl = generate_array_guard(klass_node, nullptr, &obj);
if (array_ctl != nullptr) {
// Array case: size is round(header + element_size*arraylength).
// Since arraylength is different for every array instance, we have to
Expand Down
18 changes: 9 additions & 9 deletions src/hotspot/share/opto/library_call.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,20 +163,20 @@ class LibraryCallKit : public GraphKit {
RegionNode* region);
Node* generate_interface_guard(Node* kls, RegionNode* region);
Node* generate_hidden_class_guard(Node* kls, RegionNode* region);
Node* generate_array_guard(Node* kls, RegionNode* region) {
return generate_array_guard_common(kls, region, false, false);
Node* generate_array_guard(Node* kls, RegionNode* region, Node** obj = nullptr) {
return generate_array_guard_common(kls, region, false, false, obj);
}
Node* generate_non_array_guard(Node* kls, RegionNode* region) {
return generate_array_guard_common(kls, region, false, true);
Node* generate_non_array_guard(Node* kls, RegionNode* region, Node** obj = nullptr) {
return generate_array_guard_common(kls, region, false, true, obj);
}
Node* generate_objArray_guard(Node* kls, RegionNode* region) {
return generate_array_guard_common(kls, region, true, false);
Node* generate_objArray_guard(Node* kls, RegionNode* region, Node** obj = nullptr) {
return generate_array_guard_common(kls, region, true, false, obj);
}
Node* generate_non_objArray_guard(Node* kls, RegionNode* region) {
return generate_array_guard_common(kls, region, true, true);
Node* generate_non_objArray_guard(Node* kls, RegionNode* region, Node** obj = nullptr) {
return generate_array_guard_common(kls, region, true, true, obj);
}
Node* generate_array_guard_common(Node* kls, RegionNode* region,
bool obj_array, bool not_array);
bool obj_array, bool not_array, Node** obj = nullptr);
Node* generate_virtual_guard(Node* obj_klass, RegionNode* slow_region);
CallJavaNode* generate_method_call(vmIntrinsicID method_id, bool is_virtual, bool is_static, bool res_not_null);
CallJavaNode* generate_method_call_static(vmIntrinsicID method_id, bool res_not_null) {
Expand Down
20 changes: 10 additions & 10 deletions src/hotspot/share/opto/type.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4684,16 +4684,16 @@ bool TypeAryKlassPtr::is_meet_subtype_of_helper(const TypeKlassPtr *other, bool
//=============================================================================
// Convenience common pre-built types.
const TypeAryPtr* TypeAryPtr::BOTTOM;
const TypeAryPtr *TypeAryPtr::RANGE;
const TypeAryPtr *TypeAryPtr::OOPS;
const TypeAryPtr *TypeAryPtr::NARROWOOPS;
const TypeAryPtr *TypeAryPtr::BYTES;
const TypeAryPtr *TypeAryPtr::SHORTS;
const TypeAryPtr *TypeAryPtr::CHARS;
const TypeAryPtr *TypeAryPtr::INTS;
const TypeAryPtr *TypeAryPtr::LONGS;
const TypeAryPtr *TypeAryPtr::FLOATS;
const TypeAryPtr *TypeAryPtr::DOUBLES;
const TypeAryPtr* TypeAryPtr::RANGE;
const TypeAryPtr* TypeAryPtr::OOPS;
const TypeAryPtr* TypeAryPtr::NARROWOOPS;
const TypeAryPtr* TypeAryPtr::BYTES;
const TypeAryPtr* TypeAryPtr::SHORTS;
const TypeAryPtr* TypeAryPtr::CHARS;
const TypeAryPtr* TypeAryPtr::INTS;
const TypeAryPtr* TypeAryPtr::LONGS;
const TypeAryPtr* TypeAryPtr::FLOATS;
const TypeAryPtr* TypeAryPtr::DOUBLES;

//------------------------------make-------------------------------------------
const TypeAryPtr *TypeAryPtr::make(PTR ptr, const TypeAry *ary, ciKlass* k, bool xk, int offset,
Expand Down
20 changes: 10 additions & 10 deletions src/hotspot/share/opto/type.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1474,16 +1474,16 @@ class TypeAryPtr : public TypeOopPtr {

// Convenience common pre-built types.
static const TypeAryPtr* BOTTOM;
Copy link
Member

Choose a reason for hiding this comment

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

While you are here it may be better to change the other constant to TypeAryPtr* instead of TypeAryPtr *

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, done.

static const TypeAryPtr *RANGE;
static const TypeAryPtr *OOPS;
static const TypeAryPtr *NARROWOOPS;
static const TypeAryPtr *BYTES;
static const TypeAryPtr *SHORTS;
static const TypeAryPtr *CHARS;
static const TypeAryPtr *INTS;
static const TypeAryPtr *LONGS;
static const TypeAryPtr *FLOATS;
static const TypeAryPtr *DOUBLES;
static const TypeAryPtr* RANGE;
static const TypeAryPtr* OOPS;
static const TypeAryPtr* NARROWOOPS;
static const TypeAryPtr* BYTES;
static const TypeAryPtr* SHORTS;
static const TypeAryPtr* CHARS;
static const TypeAryPtr* INTS;
static const TypeAryPtr* LONGS;
static const TypeAryPtr* FLOATS;
static const TypeAryPtr* DOUBLES;
// selects one of the above:
static const TypeAryPtr *get_array_body_type(BasicType elem) {
assert((uint)elem <= T_CONFLICT && _array_body_type[elem] != nullptr, "bad elem type");
Expand Down