RFR: 8260369: [PPC64] Add support for JDK-8200555

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8260369: [PPC64] Add support for JDK-8200555

Martin Doerr
I'd like to add the PPC64 part of JDK-8200555 "OopHandle should use Access API". This will be required to support ShenandoahGC and zGC.

I have to change register usage. That's what makes this change a bit larger.

-------------

Commit messages:
 - add resolve_weak_handle which is also available on other platforms
 - remove debugging code
 - 8260369: [PPC64] Add support for JDK-8200555

Changes: https://git.openjdk.java.net/jdk/pull/2358/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2358&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8260369
  Stats: 89 lines in 7 files changed: 25 ins; 2 del; 62 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2358.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2358/head:pull/2358

PR: https://git.openjdk.java.net/jdk/pull/2358
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8260369: [PPC64] Add support for JDK-8200555

Lutz Schmidt
On Tue, 2 Feb 2021 16:14:17 GMT, Martin Doerr <[hidden email]> wrote:

> I'd like to add the PPC64 part of JDK-8200555 "OopHandle should use Access API". This will be required to support ShenandoahGC and zGC.
>
> I have to change register usage. That's what makes this change a bit larger.

Changes look good to me.
Please review and act on my inline comment on interp_masm_ppc_64.cpp(line 507).
Thanks, Lutz

src/hotspot/cpu/ppc/interp_masm_ppc_64.cpp line 507:

> 505:   load_heap_oop(result, arrayOopDesc::base_offset_in_bytes(T_OBJECT), result,
> 506:                 tmp1, tmp2,
> 507:                 MacroAssembler::MacroAssembler::PRESERVATION_NONE,

Are you sure you need this duplicate class specification?

-------------

PR: https://git.openjdk.java.net/jdk/pull/2358
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8260369: [PPC64] Add support for JDK-8200555 [v2]

Martin Doerr
In reply to this post by Martin Doerr
> I'd like to add the PPC64 part of JDK-8200555 "OopHandle should use Access API". This will be required to support ShenandoahGC and zGC.
>
> I have to change register usage. That's what makes this change a bit larger.

Martin Doerr has updated the pull request incrementally with one additional commit since the last revision:

  remove duplicate MacroAssembler::MacroAssembler::

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2358/files
  - new: https://git.openjdk.java.net/jdk/pull/2358/files/7636d1af..021aec12

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2358&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2358&range=00-01

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2358.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2358/head:pull/2358

PR: https://git.openjdk.java.net/jdk/pull/2358
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8260369: [PPC64] Add support for JDK-8200555 [v2]

Lutz Schmidt
On Thu, 4 Feb 2021 12:53:58 GMT, Martin Doerr <[hidden email]> wrote:

>> I'd like to add the PPC64 part of JDK-8200555 "OopHandle should use Access API". This will be required to support ShenandoahGC and zGC.
>>
>> I have to change register usage. That's what makes this change a bit larger.
>
> Martin Doerr has updated the pull request incrementally with one additional commit since the last revision:
>
>   remove duplicate MacroAssembler::MacroAssembler::

LGTM

-------------

Marked as reviewed by lucy (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2358
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8260369: [PPC64] Add support for JDK-8200555 [v3]

Martin Doerr
In reply to this post by Martin Doerr
> I'd like to add the PPC64 part of JDK-8200555 "OopHandle should use Access API". This will be required to support ShenandoahGC and zGC.
>
> I have to change register usage. That's what makes this change a bit larger.

Martin Doerr has updated the pull request incrementally with one additional commit since the last revision:

  use PRESERVATION_NONE in load_field_cp_cache_entry

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2358/files
  - new: https://git.openjdk.java.net/jdk/pull/2358/files/021aec12..be66f4e5

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2358&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2358&range=01-02

  Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2358.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2358/head:pull/2358

PR: https://git.openjdk.java.net/jdk/pull/2358
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8260369: [PPC64] Add support for JDK-8200555 [v2]

Martin Doerr
In reply to this post by Lutz Schmidt
On Thu, 4 Feb 2021 12:53:57 GMT, Lutz Schmidt <[hidden email]> wrote:

>> Martin Doerr has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   remove duplicate MacroAssembler::MacroAssembler::
>
> LGTM

Thanks for the review!
MacroAssembler::MacroAssembler::PRESERVATION_NONE was a copy&paste bug.
And while fixing it, I noticed that I should use PRESERVATION_NONE in load_field_cp_cache_entry, too, which fits better to the interpreter design on PPC64 where we can usually call C without any save&restore code:
https://github.com/openjdk/jdk/pull/2358/commits/be66f4e533e46549bdfc43019920bcff09c4e49a
Tested by using clobber code and running a benchmark.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2358
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8260369: [PPC64] Add support for JDK-8200555 [v3]

Niklas Radomski
In reply to this post by Martin Doerr
On Thu, 4 Feb 2021 13:08:07 GMT, Martin Doerr <[hidden email]> wrote:

>> I'd like to add the PPC64 part of JDK-8200555 "OopHandle should use Access API". This will be required to support ShenandoahGC and zGC.
>>
>> I have to change register usage. That's what makes this change a bit larger.
>
> Martin Doerr has updated the pull request incrementally with one additional commit since the last revision:
>
>   use PRESERVATION_NONE in load_field_cp_cache_entry

Looks very good! Took some time to understand why `load_resolved_reference_at_index` can be used without the need for register preservation, but seems to work just fine! I just have some very minor styling complains. Would be great if you could address those, but that's definitely no requirement.

src/hotspot/cpu/ppc/templateInterpreterGenerator_ppc.cpp line 941:

> 939:            R10_tmp             = R10_ARG8;
> 940:
> 941:   assert_different_registers(Rsize_of_parameters, Rsize_of_locals, parent_frame_resize, top_frame_size);

Consider strengthening the assertion. `Rconst_method`, for instance, must not be equals to `Rsize_of_parameters`.

src/hotspot/cpu/ppc/templateInterpreterGenerator_ppc.cpp line 939:

> 937:            Rconst_method       = R8_ARG6,
> 938:            Rconst_pool         = R9_ARG7,
> 939:            R10_tmp             = R10_ARG8;

Three variable naming styles in 5 LOCs. Would be great to have a consistent naming style throughout this function.

src/hotspot/cpu/ppc/templateTable_ppc_64.cpp line 3365:

> 3363:   // Load receiver if needed (after appendix is pushed so parameter size is correct).
> 3364:   if (load_receiver) {
> 3365:     const Register Rparam_count = Rscratch1;

The other variables of type Register declared in this method aren't `const`, although they aren't subject to change as well.

-------------

Marked as reviewed by [hidden email] (no known OpenJDK username).

PR: https://git.openjdk.java.net/jdk/pull/2358
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8260369: [PPC64] Add support for JDK-8200555 [v3]

Niklas Radomski
In reply to this post by Martin Doerr
On Thu, 4 Feb 2021 13:08:07 GMT, Martin Doerr <[hidden email]> wrote:

>> I'd like to add the PPC64 part of JDK-8200555 "OopHandle should use Access API". This will be required to support ShenandoahGC and zGC.
>>
>> I have to change register usage. That's what makes this change a bit larger.
>
> Martin Doerr has updated the pull request incrementally with one additional commit since the last revision:
>
>   use PRESERVATION_NONE in load_field_cp_cache_entry

Seems like GitHub didn't show the latest changes to me. Looks good as well, nice optimization!

-------------

Marked as reviewed by [hidden email] (no known OpenJDK username).

PR: https://git.openjdk.java.net/jdk/pull/2358
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8260369: [PPC64] Add support for JDK-8200555 [v4]

Martin Doerr
In reply to this post by Martin Doerr
> I'd like to add the PPC64 part of JDK-8200555 "OopHandle should use Access API". This will be required to support ShenandoahGC and zGC.
>
> I have to change register usage. That's what makes this change a bit larger.

Martin Doerr has updated the pull request incrementally with one additional commit since the last revision:

  feedback from Niklas

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2358/files
  - new: https://git.openjdk.java.net/jdk/pull/2358/files/be66f4e5..32e919d6

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2358&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2358&range=02-03

  Stats: 31 lines in 2 files changed: 1 ins; 0 del; 30 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2358.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2358/head:pull/2358

PR: https://git.openjdk.java.net/jdk/pull/2358
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8260369: [PPC64] Add support for JDK-8200555 [v4]

Niklas Radomski
On Fri, 5 Feb 2021 11:36:55 GMT, Martin Doerr <[hidden email]> wrote:

>> I'd like to add the PPC64 part of JDK-8200555 "OopHandle should use Access API". This will be required to support ShenandoahGC and zGC.
>>
>> I have to change register usage. That's what makes this change a bit larger.
>
> Martin Doerr has updated the pull request incrementally with one additional commit since the last revision:
>
>   feedback from Niklas

Thanks for implementing the feeback. I have nothing to add!

-------------

Marked as reviewed by [hidden email] (no known OpenJDK username).

PR: https://git.openjdk.java.net/jdk/pull/2358
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8260369: [PPC64] Add support for JDK-8200555 [v4]

Lutz Schmidt
In reply to this post by Martin Doerr
On Fri, 5 Feb 2021 11:36:55 GMT, Martin Doerr <[hidden email]> wrote:

>> I'd like to add the PPC64 part of JDK-8200555 "OopHandle should use Access API". This will be required to support ShenandoahGC and zGC.
>>
>> I have to change register usage. That's what makes this change a bit larger.
>
> Martin Doerr has updated the pull request incrementally with one additional commit since the last revision:
>
>   feedback from Niklas

Still looks good to me.

-------------

Marked as reviewed by lucy (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2358
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8260369: [PPC64] Add support for JDK-8200555 [v4]

Martin Doerr
On Fri, 5 Feb 2021 11:57:37 GMT, Lutz Schmidt <[hidden email]> wrote:

>> Martin Doerr has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   feedback from Niklas
>
> Still looks good to me.

Thanks for the reviews!

-------------

PR: https://git.openjdk.java.net/jdk/pull/2358
Reply | Threaded
Open this post in threaded view
|

Integrated: 8260369: [PPC64] Add support for JDK-8200555

Martin Doerr
In reply to this post by Martin Doerr
On Tue, 2 Feb 2021 16:14:17 GMT, Martin Doerr <[hidden email]> wrote:

> I'd like to add the PPC64 part of JDK-8200555 "OopHandle should use Access API". This will be required to support ShenandoahGC and zGC.
>
> I have to change register usage. That's what makes this change a bit larger.

This pull request has now been integrated.

Changeset: 48f5220c
Author:    Martin Doerr <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/48f5220c
Stats:     113 lines in 7 files changed: 26 ins; 2 del; 85 mod

8260369: [PPC64] Add support for JDK-8200555

Reviewed-by: lucy

-------------

PR: https://git.openjdk.java.net/jdk/pull/2358