8331935: Add support for primitive array C1 clone intrinsic in PPC #19250
Conversation
|
👋 Welcome back varadam! A progress list of the required criteria for merging this PR into |
|
@varada1110 This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 91 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@TheRealMDoerr, @offamitkumar) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
|
@varada1110 The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
varada1110
changed the title
openjdk
bot
changed the title
|
The test failures will be fixed by #19218. Unrelated. |
There was a problem hiding this comment.
I got crashes when testing on linux ppc64le and noticed that we need one more adaptation to handle stub == nullptr. I suggest the following addition:
diff --git a/src/hotspot/cpu/ppc/c1_LIRAssembler_ppc.cpp b/src/hotspot/cpu/ppc/c1_LIRAssembler_ppc.cpp
index b6d9200b261..dba662a2212 100644
--- a/src/hotspot/cpu/ppc/c1_LIRAssembler_ppc.cpp
+++ b/src/hotspot/cpu/ppc/c1_LIRAssembler_ppc.cpp
@@ -1968,7 +1968,11 @@ void LIR_Assembler::emit_arraycopy(LIR_OpArrayCopy* op) {
int shift = shift_amount(basic_type);
if (!(flags & LIR_OpArrayCopy::type_check)) {
- __ b(cont);
+ if (stub != nullptr) {
+ __ b(cont);
+ __ bind(slow);
+ __ b(*stub->entry());
+ }
} else {
// We don't know the array types are compatible.
if (basic_type != T_OBJECT) {
@@ -2089,9 +2093,9 @@ void LIR_Assembler::emit_arraycopy(LIR_OpArrayCopy* op) {
__ add(dst_pos, tmp, dst_pos);
}
}
+ __ bind(slow);
+ __ b(*stub->entry());
}
- __ bind(slow);
- __ b(*stub->entry());
__ bind(cont);
#ifdef ASSERT
Hi @TheRealMDoerr , I have applied the suggested changes and I have fixed the indentation fixes. Testing is also done. |
|
Thanks @TheRealMDoerr |
Hi @TheRealMDoerr , this test failure was not showing for me. |
|
I can reproduce it on linux with the fastdebug build. |
Yes. The test failing with fastdebug build |
|
I also have a minor cleanup proposal for diff --git a/src/hotspot/cpu/ppc/c1_LIRAssembler_ppc.cpp b/src/hotspot/cpu/ppc/c1_LIRAssembler_ppc.cpp
index dba662a2212..2424d820177 100644
--- a/src/hotspot/cpu/ppc/c1_LIRAssembler_ppc.cpp
+++ b/src/hotspot/cpu/ppc/c1_LIRAssembler_ppc.cpp
@@ -1827,18 +1827,17 @@ void LIR_Assembler::emit_arraycopy(LIR_OpArrayCopy* op) {
int flags = op->flags();
ciArrayKlass* default_type = op->expected_type();
- BasicType basic_type = default_type != nullptr ? default_type->element_type()->basic_type() : T_ILLEGAL;
+ BasicType basic_type = (default_type != nullptr) ? default_type->element_type()->basic_type() : T_ILLEGAL;
if (basic_type == T_ARRAY) basic_type = T_OBJECT;
// Set up the arraycopy stub information.
ArrayCopyStub* stub = op->stub();
- const int frame_resize = frame::native_abi_reg_args_size - sizeof(frame::java_abi); // C calls need larger frame.
// Always do stub if no type information is available. It's ok if
// the known type isn't loaded since the code sanity checks
// in debug mode and the type isn't required when we know the exact type
// also check that the type is an array type.
- if (op->expected_type() == nullptr) {
+ if (default_type == nullptr) {
assert(src->is_nonvolatile() && src_pos->is_nonvolatile() && dst->is_nonvolatile() && dst_pos->is_nonvolatile() &&
length->is_nonvolatile(), "must preserve");
address copyfunc_addr = StubRoutines::generic_arraycopy();
@@ -1873,7 +1872,7 @@ void LIR_Assembler::emit_arraycopy(LIR_OpArrayCopy* op) {
return;
}
- assert(default_type != nullptr && default_type->is_array_klass(), "must be true at this point");
+ assert(default_type != nullptr && default_type->is_array_klass() && default_type->is_loaded(), "must be true at this point");
Label cont, slow, copyfunc;
bool simple_check_flag_set = flags & (LIR_OpArrayCopy::src_null_check |Would be nice to have. |
There was a problem hiding this comment.
I guess you can update this as well:
diff --git a/src/hotspot/cpu/ppc/c1_LIRAssembler_ppc.cpp b/src/hotspot/cpu/ppc/c1_LIRAssembler_ppc.cpp
index 2424d820177..0c1e23c6353 100644
--- a/src/hotspot/cpu/ppc/c1_LIRAssembler_ppc.cpp
+++ b/src/hotspot/cpu/ppc/c1_LIRAssembler_ppc.cpp
@@ -2107,7 +2107,7 @@ void LIR_Assembler::emit_arraycopy(LIR_OpArrayCopy* op) {
// subtype which we can't check or src is the same array as dst
// but not necessarily exactly of type default_type.
Label known_ok, halt;
- metadata2reg(op->expected_type()->constant_encoding(), tmp);
+ metadata2reg(default_type->constant_encoding(), tmp);
if (UseCompressedClassPointers) {
// Tmp holds the default type. It currently comes uncompressed after the
// load of a constant, so encode it.
|
Thank you @TheRealMDoerr @offamitkumar . I am running the tests: hotspot_compiler, hotspot_gc, hotspot_serviceability and hotspot_runtime for tier1, tier2 and tier3 with fastdebug, slowdebug and release. I will update the results. |
I think with fastdebug is sufficient. |
|
I've put it again into our nightly tests and haven't seen any errors which may have been caused by this PR. There are currently some unrelated errors. So, I think it's good to go. |
|
There is currently a regression in the original code, JDK-8332670, which may explain some instability on PPC. |
|
Thanks for the hint! We should wait for that one to be fixed. |
Completed the testing for fastdebug. There are few unrelated test failures |
|
#19538 is integrated, so we can ship this one, too. |
|
/integrate |
|
@varada1110 |
|
/sponsor |
|
Going to push as commit 6968770.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@offamitkumar @varada1110 Pushed as commit 6968770. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
https://bugs.openjdk.org/browse/JDK-8302850 port for PPC64
JMH Benchmark Results
Hotspot compiler tests results :
2 test failures shown here is not related to code change. It is present without this changes
Reported Issue : JDK-8331935
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19250/head:pull/19250$ git checkout pull/19250Update a local copy of the PR:
$ git checkout pull/19250$ git pull https://git.openjdk.org/jdk.git pull/19250/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 19250View PR using the GUI difftool:
$ git pr show -t 19250Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19250.diff
Webrev
Link to Webrev Comment