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
11 changes: 7 additions & 4 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 @@ -5917,11 +5917,14 @@ bool LibraryCallKit::inline_arraycopy() {
// (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);
const Type* tary = TypeAryPtr::make(TypePtr::BotPTR, TypeAry::make(Type::BOTTOM, TypeInt::POS), nullptr, false, Type::OffsetBot);
src = _gvn.transform(new CheckCastPPNode(control(), src, tary));
if (!stopped()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we simply return then?

Copy link
Member Author

Choose a reason for hiding this comment

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

But we need to set up the slow_region path, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

But we need to set up the slow_region path, right?

By that you mean have the slow_region feed into the uncommon trap that's only created later. It does feel weird that we know we have reached a dead end and we keep trying to add stuff, but ok then.
The other thing is shouldn't the cast be added in generate_non_array_guard()? I see it's used elsewhere (LibraryCallKit::inline_native_getLength()): couldn't the same bug occur there?

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.

By that you mean have the slow_region feed into the uncommon trap that's only created later.

Right. It's a bit weird but probably still the best solution in terms of complexity. The trap will lead to recompilation and then the too_many_traps check will trigger.

The other thing is shouldn't the cast be added in generate_non_array_guard()? I see it's used elsewhere (LibraryCallKit::inline_native_getLength()): couldn't the same bug occur there?

Right, good catch. I think the use in LibraryCallKit::inline_native_getLength has the same problem. We can't easily put the cast into generate_non_array_guard though because it operates on the Klass and not on the object. The other generate*array*guard methods potentially have the same issue but current uses look good. I guess it's best to fix the LibraryCallKit::inline_native_getLength as well, i.e., make it the caller's responsibility to add a cast. What do you think?

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.

Hmm, maybe inline_getObjectSize is affected as well:

jdk/src/hotspot/share/opto/library_call.cpp

Lines 8535 to 8543 in afe5434

Node* array_ctl = generate_array_guard(klass_node, nullptr);
if (array_ctl != nullptr) {
// Array case: size is round(header + element_size*arraylength).
// Since arraylength is different for every array instance, we have to
// compute the whole thing at runtime.
PreserveJVMState pjvms(this);
set_control(array_ctl);
Node* arr_length = load_array_length(obj);

And LibraryCallKit::inline_native_clone as well:

jdk/src/hotspot/share/opto/library_call.cpp

Lines 5257 to 5262 in afe5434

Node* array_ctl = generate_array_guard(obj_klass, (RegionNode*)nullptr);
if (array_ctl != nullptr) {
// It's an array.
PreserveJVMState pjvms(this);
set_control(array_ctl);
Node* obj_length = load_array_length(obj);

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it's best to fix the LibraryCallKit::inline_native_getLength as well, i.e., make it the caller's responsibility to add a cast. What do you think?

Maybe the methods need to take an extra parameter (the object to cast)?
Having the cast in the method would lead to less code duplication and a lower risk of forgetting the cast when new calls of the method are added so that's what I would go with unless it's really a pain.

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, I'll give that a try.

src = _gvn.transform(new CheckCastPPNode(control(), src, 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.

Why is this a CheckCastPP and not a CastPP? My understanding is that a CheckCastPP is used when we force changing the type of a node (e.g a raw pointer of Allocate into a typed pointer), so we do not join the type of the input with that of the output.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, I changed that.

}

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

// (2) src and dest arrays must have elements of the same BasicType
// done at macro expansion or at Ideal transformation time
Expand Down
4 changes: 3 additions & 1 deletion 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,6 +4683,7 @@ 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;
Expand Down
3 changes: 2 additions & 1 deletion 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,6 +1473,7 @@ class TypeAryPtr : public TypeOopPtr {
virtual const TypeKlassPtr* as_klass_type(bool try_for_exact = false) const;

// 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;
Expand Down