RFR: 8296477: Foreign linker implementation update following JEP 434 [v4]

[Vladimir Ivanov]

On Thu, 10 Nov 2022 16:48:19 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

>> Pull in linker implementation changes, that include non-trivial changes to VM code, from the panama-foreign repo into the main JDK.
>> 
>> This is split off from the main JEP integration to make reviewing easier.
>> 
>> This includes the following patches:
>> 
>> 1. https://github.com/openjdk/panama-foreign/pull/698
>> 2. https://github.com/openjdk/panama-foreign/pull/699
>> 3. (part of) https://github.com/openjdk/panama-foreign/pull/731
>> 4. https://github.com/openjdk/panama-foreign/pull/740
>> 5. https://github.com/openjdk/panama-foreign/pull/746
>> 6. https://github.com/openjdk/panama-foreign/pull/742
>> 7. https://github.com/openjdk/panama-foreign/pull/743
>> 
>> Probably the biggest change to the code comes from replacing `VMReg` - which can not represent offsets into the stack that are not a multiple of the VM's stack slot size (32-bits) - with the new `VMStorage` class, which can describe byte offsets into the stack, as well as having a register mask to indicate only certain register segments.
>> 
>> The only part of 3. that is in this PR is the part that turns the `VMStorage` class in Java into a record.
>> 
>> Please refer to the PR of each individual patch for a more detailed description.
>
> Jorn Vernee has updated the pull request incrementally with three additional commits since the last revision:
> 
>  - Tweak copyright headers
>  - Use @requires to disable some tests on x86
>  - Use AssertionError for internal exceptions

Haven't finished reviewing VM part. 

2 questions so far:
  * `VMStorage` looks very similar to `VMReg`. What's the purpose of the new representation?
  * why do you structure the header files the way you do? `vmstorage.inline.hpp`, `vmstorage_<cpu>.inline.hpp`, `vmstorageBase.inline.hpp` instead of just `vmstorage.hpp`/`vmstorage_<cpu>.hpp`

src/hotspot/cpu/aarch64/downcallLinker_aarch64.cpp line 146:

> 144:   Register tmp2 = r10;
> 145: 
> 146:   VMStorage shuffle_reg = VMS_R19;

I'd prefer to see `as_VMStorage(Register)` used instead and all `VMS_...` constants go away.

src/hotspot/cpu/aarch64/foreignGlobals_aarch64.cpp line 51:

> 49: 
> 50:   objArrayOop inputStorage = jdk_internal_foreign_abi_ABIDescriptor::inputStorage(abi_oop);
> 51:   parse_register_array(inputStorage, (int) StorageType::INTEGER, abi._integer_argument_registers, as_Register);

Converting `type_index` argument from `int` to `StorageType`  would allow to avoid explicit casts.

src/hotspot/cpu/aarch64/vmstorage_aarch64.inline.hpp line 68:

> 66: }
> 67: 
> 68: inline VMStorage as_VMStorage(Register reg) {

Mark as `constexpr` maybe?

src/hotspot/cpu/x86/downcallLinker_x86_64.cpp line 239:

> 237:     __ vzeroupper();
> 238: 
> 239:     if(should_save_return_value) {

Missing space (here and below).

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

PR: https://git.openjdk.org/jdk/pull/11019