Skip to content

Commit

Permalink
8335221: Some C2 intrinsics incorrectly assume that type argument is …
Browse files Browse the repository at this point in the history
…compile-time constant

Reviewed-by: roland, chagedorn
  • Loading branch information
Vladimir Kozlov committed Jun 28, 2024
1 parent 3e23e9c commit 166f9d9
Show file tree
Hide file tree
Showing 2 changed files with 67 additions and 38 deletions.
104 changes: 66 additions & 38 deletions src/hotspot/share/opto/library_call.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5489,42 +5489,72 @@ void LibraryCallKit::create_new_uncommon_trap(CallStaticJavaNode* uncommon_trap_
uncommon_trap_call->set_req(0, top()); // not used anymore, kill it
}

// Common checks for array sorting intrinsics arguments.
// Returns `true` if checks passed.
bool LibraryCallKit::check_array_sort_arguments(Node* elementType, Node* obj, BasicType& bt) {
// check address of the class
if (elementType == nullptr || elementType->is_top()) {
return false; // dead path
}
const TypeInstPtr* elem_klass = gvn().type(elementType)->isa_instptr();
if (elem_klass == nullptr) {
return false; // dead path
}
// java_mirror_type() returns non-null for compile-time Class constants only
ciType* elem_type = elem_klass->java_mirror_type();
if (elem_type == nullptr) {
return false;
}
bt = elem_type->basic_type();
// Disable the intrinsic if the CPU does not support SIMD sort
if (!Matcher::supports_simd_sort(bt)) {
return false;
}
// check address of the array
if (obj == nullptr || obj->is_top()) {
return false; // dead path
}
const TypeAryPtr* obj_t = _gvn.type(obj)->isa_aryptr();
if (obj_t == nullptr || obj_t->elem() == Type::BOTTOM) {
return false; // failed input validation
}
return true;
}

//------------------------------inline_array_partition-----------------------
bool LibraryCallKit::inline_array_partition() {
address stubAddr = StubRoutines::select_array_partition_function();
if (stubAddr == nullptr) {
return false; // Intrinsic's stub is not implemented on this platform
}
assert(callee()->signature()->size() == 9, "arrayPartition has 8 parameters (one long)");

Node* elementType = null_check(argument(0));
// no receiver because it is a static method
Node* elementType = argument(0);
Node* obj = argument(1);
Node* offset = argument(2);
Node* offset = argument(2); // long
Node* fromIndex = argument(4);
Node* toIndex = argument(5);
Node* indexPivot1 = argument(6);
Node* indexPivot2 = argument(7);
// PartitionOperation: argument(8) is ignored

Node* pivotIndices = nullptr;
BasicType bt = T_ILLEGAL;

if (!check_array_sort_arguments(elementType, obj, bt)) {
return false;
}
null_check(obj);
// If obj is dead, only null-path is taken.
if (stopped()) {
return true;
}
// Set the original stack and the reexecute bit for the interpreter to reexecute
// the bytecode that invokes DualPivotQuicksort.partition() if deoptimization happens.
{ PreserveReexecuteState preexecs(this);
jvms()->set_should_reexecute(true);

const TypeInstPtr* elem_klass = gvn().type(elementType)->isa_instptr();
ciType* elem_type = elem_klass->const_oop()->as_instance()->java_mirror_type();
BasicType bt = elem_type->basic_type();
// Disable the intrinsic if the CPU does not support SIMD sort
if (!Matcher::supports_simd_sort(bt)) {
return false;
}
address stubAddr = nullptr;
stubAddr = StubRoutines::select_array_partition_function();
// stub not loaded
if (stubAddr == nullptr) {
return false;
}
// get the address of the array
const TypeAryPtr* obj_t = _gvn.type(obj)->isa_aryptr();
if (obj_t == nullptr || obj_t->elem() == Type::BOTTOM ) {
return false; // failed input validation
}
Node* obj_adr = make_unsafe_address(obj, offset);

// create the pivotIndices array of type int and size = 2
Expand Down Expand Up @@ -5557,31 +5587,29 @@ bool LibraryCallKit::inline_array_partition() {

//------------------------------inline_array_sort-----------------------
bool LibraryCallKit::inline_array_sort() {
address stubAddr = StubRoutines::select_arraysort_function();
if (stubAddr == nullptr) {
return false; // Intrinsic's stub is not implemented on this platform
}
assert(callee()->signature()->size() == 7, "arraySort has 6 parameters (one long)");

Node* elementType = null_check(argument(0));
// no receiver because it is a static method
Node* elementType = argument(0);
Node* obj = argument(1);
Node* offset = argument(2);
Node* offset = argument(2); // long
Node* fromIndex = argument(4);
Node* toIndex = argument(5);
// SortOperation: argument(6) is ignored

const TypeInstPtr* elem_klass = gvn().type(elementType)->isa_instptr();
ciType* elem_type = elem_klass->const_oop()->as_instance()->java_mirror_type();
BasicType bt = elem_type->basic_type();
// Disable the intrinsic if the CPU does not support SIMD sort
if (!Matcher::supports_simd_sort(bt)) {
return false;
}
address stubAddr = nullptr;
stubAddr = StubRoutines::select_arraysort_function();
//stub not loaded
if (stubAddr == nullptr) {
BasicType bt = T_ILLEGAL;

if (!check_array_sort_arguments(elementType, obj, bt)) {
return false;
}

// get address of the array
const TypeAryPtr* obj_t = _gvn.type(obj)->isa_aryptr();
if (obj_t == nullptr || obj_t->elem() == Type::BOTTOM ) {
return false; // failed input validation
null_check(obj);
// If obj is dead, only null-path is taken.
if (stopped()) {
return true;
}
Node* obj_adr = make_unsafe_address(obj, offset);

Expand Down
1 change: 1 addition & 0 deletions src/hotspot/share/opto/library_call.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,7 @@ class LibraryCallKit : public GraphKit {
JVMState* arraycopy_restore_alloc_state(AllocateArrayNode* alloc, int& saved_reexecute_sp);
void arraycopy_move_allocation_here(AllocateArrayNode* alloc, Node* dest, JVMState* saved_jvms_before_guards, int saved_reexecute_sp,
uint new_idx);
bool check_array_sort_arguments(Node* elementType, Node* obj, BasicType& bt);
bool inline_array_sort();
bool inline_array_partition();
typedef enum { LS_get_add, LS_get_set, LS_cmp_swap, LS_cmp_swap_weak, LS_cmp_exchange } LoadStoreKind;
Expand Down

3 comments on commit 166f9d9

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

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

@vnkozlov
Copy link
Contributor

Choose a reason for hiding this comment

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

/backport jdk:jdk23

@openjdk
Copy link

@openjdk openjdk bot commented on 166f9d9 Jul 1, 2024

Choose a reason for hiding this comment

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

@vnkozlov the backport was successfully created on the branch backport-vnkozlov-166f9d9a-jdk23 in my personal fork of openjdk/jdk. To create a pull request with this backport targeting openjdk/jdk:jdk23, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit 166f9d9a from the openjdk/jdk repository.

The commit being backported was authored by Vladimir Kozlov on 28 Jun 2024 and was reviewed by Roland Westrelin and Christian Hagedorn.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk:

$ git fetch https://github.com/openjdk-bots/jdk.git backport-vnkozlov-166f9d9a-jdk23:backport-vnkozlov-166f9d9a-jdk23
$ git checkout backport-vnkozlov-166f9d9a-jdk23
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk.git backport-vnkozlov-166f9d9a-jdk23

Please sign in to comment.