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 all commits
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
30 changes: 19 additions & 11 deletions src/hotspot/share/opto/library_call.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1999, 2024, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1999, 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
Expand Down 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,8 +5923,8 @@ bool LibraryCallKit::inline_arraycopy() {
record_for_igvn(slow_region);

// (1) src and dest are arrays.
generate_non_array_guard(load_object_klass(src), slow_region);
generate_non_array_guard(load_object_klass(dest), slow_region);
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 @@ -8532,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
20 changes: 10 additions & 10 deletions src/hotspot/share/opto/library_call.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2020, 2024, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2020, 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
Expand Down 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
24 changes: 13 additions & 11 deletions src/hotspot/share/opto/type.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1997, 2024, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1997, 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
Expand Down Expand Up @@ -582,6 +582,7 @@ void Type::Initialize_shared(Compile* current) {
TypeAryPtr::_array_interfaces = TypeInterfaces::make(&array_interfaces);
TypeAryKlassPtr::_array_interfaces = TypeAryPtr::_array_interfaces;

TypeAryPtr::BOTTOM = TypeAryPtr::make(TypePtr::BotPTR, TypeAry::make(Type::BOTTOM, TypeInt::POS), nullptr, false, Type::OffsetBot);
TypeAryPtr::RANGE = TypeAryPtr::make( TypePtr::BotPTR, TypeAry::make(Type::BOTTOM,TypeInt::POS), nullptr /* current->env()->Object_klass() */, false, arrayOopDesc::length_offset_in_bytes());

TypeAryPtr::NARROWOOPS = TypeAryPtr::make(TypePtr::BotPTR, TypeAry::make(TypeNarrowOop::BOTTOM, TypeInt::POS), nullptr /*ciArrayKlass::make(o)*/, false, Type::OffsetBot);
Expand Down Expand Up @@ -4682,16 +4683,17 @@ bool TypeAryKlassPtr::is_meet_subtype_of_helper(const TypeKlassPtr *other, bool

//=============================================================================
// Convenience common pre-built types.
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::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;

//------------------------------make-------------------------------------------
const TypeAryPtr *TypeAryPtr::make(PTR ptr, const TypeAry *ary, ciKlass* k, bool xk, int offset,
Expand Down
23 changes: 12 additions & 11 deletions src/hotspot/share/opto/type.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1997, 2024, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1997, 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
Expand Down Expand Up @@ -1473,16 +1473,17 @@ class TypeAryPtr : public TypeOopPtr {
virtual const TypeKlassPtr* as_klass_type(bool try_for_exact = false) const;

// Convenience common pre-built types.
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* 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;
// 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
7 changes: 5 additions & 2 deletions test/hotspot/jtreg/compiler/arraycopy/TestArrayCopyNoInit.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2014, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2014, 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
Expand All @@ -23,11 +23,14 @@

/*
* @test
* @bug 8064703
* @bug 8064703 8347006
* @summary Deoptimization between array allocation and arraycopy may result in non initialized array
*
* @run main/othervm -XX:-BackgroundCompilation -XX:-UseOnStackReplacement -XX:TypeProfileLevel=020
* compiler.arraycopy.TestArrayCopyNoInit
* @run main/othervm -XX:-BackgroundCompilation -XX:-UseOnStackReplacement -XX:TypeProfileLevel=020
* -XX:+UnlockExperimentalVMOptions -XX:+UseCompactObjectHeaders -XX:-UseTLAB
Copy link
Member

Choose a reason for hiding this comment

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

You have been able to reproduce this with -UseCompressedClassPointers, right? If so, I'd suggest we do a run config with -UseCCP instead of +UseCOH, because this gives us a cleaner way for backports, if we need one later.

Copy link
Member Author

@TobiHartmann TobiHartmann Jan 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

You have been able to reproduce this with -UseCompressedClassPointers, right?

No, I was never able to reproduce this with -XX:-UseCompressedClassPointers.

Copy link
Member

Choose a reason for hiding this comment

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

OK, I was confused by this in PR body then:

I was able to reliably reproduce the issue with compiler/arraycopy/TestArrayCopyNoInit.java and -XX:-UseTLAB -XX:+UnlockExperimentalVMOptions -XX:-UseCompressedClassPointers on Linux AArch64 and verified that the fix solves the problem.

But fine, if it reproduces with +UCOH, let it be there.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, that's actually a typo, good catch. Should be -XX:+UseCompactObjectHeaders. I'll fix it in the description.

Copy link
Member

Choose a reason for hiding this comment

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

Cant you please add this '@run' as a separate testcase with it's own id. So it is easier to identify and exclude the failures.

* compiler.arraycopy.TestArrayCopyNoInit
*/

package compiler.arraycopy;
Expand Down