On Thu, 10 Nov 2022 16:48:19 GMT, Jorn Vernee <jver...@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
