Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v8]

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

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v8]

Anton Kozlov-2
> Please review the implementation of JEP 391: macOS/AArch64 Port.
>
> It's heavily based on existing ports to linux/aarch64, macos/x86_64, and windows/aarch64.
>
> Major changes are in:
> * src/hotspot/cpu/aarch64: support of the new calling convention (subtasks JDK-8253817, JDK-8253818)
> * src/hotspot/os_cpu/bsd_aarch64: copy of os_cpu/linux_aarch64 with necessary adjustments (JDK-8253819)
> * src/hotspot/share, test/hotspot/gtest: support of write-xor-execute (W^X), required on macOS/AArch64 platform. It's implemented with pthread_jit_write_protect_np provided by Apple. The W^X mode is local to a thread, so W^X mode change relates to the java thread state change (for java threads). In most cases, JVM executes in write-only mode, except when calling a generated stub like SafeFetch, which requires a temporary switch to execute-only mode. The same execute-only mode is enabled when a java thread executes in java or native states. This approach of managing W^X mode turned out to be simple and efficient enough.
> * src/jdk.hotspot.agent: serviceability agent implementation (JDK-8254941)

Anton Kozlov has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 62 commits:

 - Merge branch 'master' into jdk-macos
 - Update copyright year for BsdAARCH64ThreadContext.java
 - Fix inclusing of StubRoutines header
 - Redo buildsys fix
 - Revert harfbuzz changes, disable warnings for it
 - Little adjustement of SlowSignatureHandler
 - Partially bring previous commit
 - Revert "Address feedback for signature generators"
   
   This reverts commit 50b55f6684cd21f8b532fa979b7b6fbb4613266d.
 - Refactor CDS disabling
 - Redo builsys support for aarch64-darwin
 - ... and 52 more: https://git.openjdk.java.net/jdk/compare/8a9004da...b421e0b4

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

Changes: https://git.openjdk.java.net/jdk/pull/2200/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2200&range=07
  Stats: 3266 lines in 114 files changed: 3164 ins; 41 del; 61 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2200.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2200/head:pull/2200

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

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v8]

Andrew Haley-2
On Sun, 31 Jan 2021 20:14:01 GMT, Anton Kozlov <[hidden email]> wrote:

>> Please review the implementation of JEP 391: macOS/AArch64 Port.
>>
>> It's heavily based on existing ports to linux/aarch64, macos/x86_64, and windows/aarch64.
>>
>> Major changes are in:
>> * src/hotspot/cpu/aarch64: support of the new calling convention (subtasks JDK-8253817, JDK-8253818)
>> * src/hotspot/os_cpu/bsd_aarch64: copy of os_cpu/linux_aarch64 with necessary adjustments (JDK-8253819)
>> * src/hotspot/share, test/hotspot/gtest: support of write-xor-execute (W^X), required on macOS/AArch64 platform. It's implemented with pthread_jit_write_protect_np provided by Apple. The W^X mode is local to a thread, so W^X mode change relates to the java thread state change (for java threads). In most cases, JVM executes in write-only mode, except when calling a generated stub like SafeFetch, which requires a temporary switch to execute-only mode. The same execute-only mode is enabled when a java thread executes in java or native states. This approach of managing W^X mode turned out to be simple and efficient enough.
>> * src/jdk.hotspot.agent: serviceability agent implementation (JDK-8254941)
>
> Anton Kozlov has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 62 commits:
>
>  - Merge branch 'master' into jdk-macos
>  - Update copyright year for BsdAARCH64ThreadContext.java
>  - Fix inclusing of StubRoutines header
>  - Redo buildsys fix
>  - Revert harfbuzz changes, disable warnings for it
>  - Little adjustement of SlowSignatureHandler
>  - Partially bring previous commit
>  - Revert "Address feedback for signature generators"
>    
>    This reverts commit 50b55f6684cd21f8b532fa979b7b6fbb4613266d.
>  - Refactor CDS disabling
>  - Redo builsys support for aarch64-darwin
>  - ... and 52 more: https://git.openjdk.java.net/jdk/compare/8a9004da...b421e0b4

src/hotspot/cpu/aarch64/interpreterRT_aarch64.cpp line 84:

> 82: // on stack. Natural alignment for types are still in place,
> 83: // for example double/long should be 8 bytes alligned
> 84:

This comment is a bit confusing because it's no longer #ifdef APPLE. Better move it up to Line 41.

src/hotspot/cpu/aarch64/interpreterRT_aarch64.cpp line 352:

> 350:
> 351: #ifdef __APPLE__
> 352:   virtual void pass_byte()

Please remove ```#ifdef __APPLE__``` around this region.

src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp line 839:

> 837:           // The code unable to handle this, bailout.
> 838:           return -1;
> 839: #endif

This looks like a bug to me. The caller doesn't necessarily check the return value. See CallRuntimeNode::calling_convention.

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

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

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v8]

Andrew Haley-2
On Mon, 15 Feb 2021 18:00:50 GMT, Vladimir Kempik <[hidden email]> wrote:

>> src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp line 839:
>>
>>> 837:           // The code unable to handle this, bailout.
>>> 838:           return -1;
>>> 839: #endif
>>
>> This looks like a bug to me. The caller doesn't necessarily check the return value. See CallRuntimeNode::calling_convention.
>
> Hello, we have updated PR, now this bailout is used only by the code which can handle it (native wrapper generator), for the rest it will cause guarantee failed if this bailout is triggered

This is when passing a float, yes? In the case where we have more float arguments than n_float_register_parameters_c.
I don't understand why you think it's acceptable to bail in this case. Can you explain, please?

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

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

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v8]

Vladimir Kempik-3
On Mon, 15 Feb 2021 19:07:40 GMT, Andrew Haley <[hidden email]> wrote:

>> Hello, we have updated PR, now this bailout is used only by the code which can handle it (native wrapper generator), for the rest it will cause guarantee failed if this bailout is triggered
>
> This is when passing a float, yes? In the case where we have more float arguments than n_float_register_parameters_c.
> I don't understand why you think it's acceptable to bail in this case. Can you explain, please?

it's for everything that uses less than 8 bytes on a stack( ints ( 4), shorts(2), bytes(1), floats(4)).
currently native wrapper generation does not support such cases at all, it needs refactoring before this can be implemented.
So when a method has more argument than can be placed in registers, we may have issues.

So we just bailing out to interpreter in case when a smaller (<=4 b) type is going to be passed thru the stack.

There was attempt to implement handling such cases but currently it requires some hacks (like using some vectors for non-specific task) - https://github.com/openjdk/aarch64-port/pull/3

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

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