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

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

Andrew Haley-2
On Fri, 22 Jan 2021 18:49:42 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)

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

> 84:
> 85:   switch (_num_int_args) {
> 86:   case 0:

I don't think you need such a large switch statement. I think this can be expressed as
if (num_int_args <= 6) {
    ldr(as_Register(num_int_args + 1), src);
... etc.

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

> 41: //describe amount of space in bytes occupied by type on native stack
> 42: #ifdef __APPLE__
> 43:     const int nativeByteSpace        = sizeof(jbyte);

I'm not sure these names should be in the global name space, and they're not very descriptive. Something like NativeStack::byteSpace would be better. Then you can use them with a `using namespace NativeStack` declaration.

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

> 431:       store_and_inc(_to, from_obj, nativeShortSpace);
> 432:
> 433:       _num_int_args++;

Please lift this increment out of the conditional.

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

> 392:
> 393: class SlowSignatureHandler
> 394:   : public NativeSignatureIterator {

SlowSignatureHandler is turning into a maintenance nightmare. This isn't the fault of this commit so much as some nasty cut-and=paste programming in the code you're editing. It might well be better to rewrite this whole thing to use a table-driven approach, with a table that contains the increment and the size of each native type: then we'd have only a single routine which could pass data of any type, and each OS needs only supply a table of constants.

src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 5272:

> 5270: void MacroAssembler::get_thread(Register dst) {
> 5271:   RegSet saved_regs = RegSet::range(r0, r1) + BSD_ONLY(RegSet::range(r2, r17)) + lr - dst;
> 5272:   push(saved_regs, sp);

This isn't very nice.

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

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 [v2]

Andrew Haley-2
On Sun, 24 Jan 2021 15:50:01 GMT, Anton Kozlov <[hidden email]> wrote:

>> src/hotspot/cpu/aarch64/interpreterRT_aarch64.cpp line 86:
>>
>>> 84:
>>> 85:   switch (_num_int_args) {
>>> 86:   case 0:
>>
>> I don't think you need such a large switch statement. I think this can be expressed as
>> if (num_int_args <= 6) {
>>     ldr(as_Register(num_int_args + r1.encoding()), src);
>> ... etc.
>
> I like the suggestion. For the defense, new functions were made this way intentionally, to match existing `pass_int`, `pass_long`,.. I take your comment as a blessing to fix all of them. But I feel that refactoring of existing code should go in a separate commit. Especially after `pass_int` used `ldr/str` instead of `ldrw/strw`, which looks wrong. https://github.com/openjdk/jdk/pull/2200/files#diff-1ff58ce70aeea7e9842d34e8d8fd9c94dd91182999d455618b2a171efd8f742cL87-R122

This is new code, and it should not repeat the mistakes of the past. There's no need to refactor the similar existing code as part of this patch.

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

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 [v4]

Anton Kozlov-2
On Mon, 25 Jan 2021 10:00:20 GMT, Andrew Haley <[hidden email]> wrote:

>> I like the suggestion. For the defense, new functions were made this way intentionally, to match existing `pass_int`, `pass_long`,.. I take your comment as a blessing to fix all of them. But I feel that refactoring of existing code should go in a separate commit. Especially after `pass_int` used `ldr/str` instead of `ldrw/strw`, which looks wrong. https://github.com/openjdk/jdk/pull/2200/files#diff-1ff58ce70aeea7e9842d34e8d8fd9c94dd91182999d455618b2a171efd8f742cL87-R122
>
> This is new code, and it should not repeat the mistakes of the past. There's no need to refactor the similar existing code as part of this patch.

Makes sense, I've reverted changes in the old code but will propose them in the separate PR shortly.

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

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 [v15]

Anton Kozlov-2
In reply to this post by Andrew Haley-2
On Sun, 31 Jan 2021 20:08:01 GMT, Vladimir Kempik <[hidden email]> wrote:

>> I'm not sure it can wait. This change turns already-messy code into something significantly messier, to the extent that it's not really good enough to go into mainline.
>
> Hello
> Does this look like something in the right direction ?
>
> https://github.com/VladimirKempik/jdk/commit/c2820734f4b10148154085a70d380b8c5775fa49

The latest merge with JDK-8261071 should resolve the issue. Please take a look.

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

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