[foreign-abi] RFR: 8254260: Consider splitting binding recipe operators that serve a dual role [v3]

[Maurizio Cimadamore]

On Fri, 9 Oct 2020 13:14:30 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

>> Hi,
>> 
>> This patch splits the binding recipe operators that currently serve a dual purpose into 2 operators each:
>> - MOVE -> VM_STORE and VM_LOAD
>> - DEREFERENCE -> BUFFER_STORE and BUFFER_LOAD
>> - CONVERT_ADDRESS -> BOX_ADDRESS and UNBOX_ADDRESS
>> 
>> Note that I also added a TO_SEGMENT operator which converts a MemoryAddress into a MemorySegment. This was previously
>> done as part of the COPY operator, but only for upcalls, so I had to split out this functionality so that COPY could
>> have only one interpretation as well. This will also enable some more optimizations down the line, such as merely
>> wrapping a by-value struct passed as a pointer in a MemorySegment, instead of having to make a copy.  This also
>> regularized the use of allocators by the operators. Now an allocator is always passed as an argument, and that's what
>> is used to do allocations, instead of relying implicitly on MemorySegment::allocateNative. (The index of the allocator
>> argument is now also always passed when specializing an operator, to indicate the dependency, and so that operator
>> impls don't have to 'guess' which argument index is the allocator (see for instance the implementation of
>> Copy::specialize).  The rest of the patch is more or less a mechanical renaming, and updating the tests/CallArrangers
>> to use the different operator names.  Thanks, Jorn
>
> Jorn Vernee has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev
> excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since
> the last revision:
>  - Merge branch 'foreign-abi' into Op_Split
>  - Review Comments
>  - - Split Move operator into VMStore and VMLoad
>    - Split Dereference operator into BufferStore and BufferLoad
>    - Add ToSegment operator for converting a MemoryAddress into a MemorySegment
>    - updated implementation and tests.

Very nice - just added another minor stylistic comment on binding MH constants

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/Binding.java line 218:

> 216:             MH_BASE_ADDRESS = lookup.findVirtual(MemorySegment.class, "address",
> 217:                     methodType(MemoryAddress.class));
> 218:             MH_COPY_BUFFER = lookup.findStatic(Binding.Copy.class, "copyBuffer",

I'm assuming these are only really required in the specialize() method of the various subclasses - should we move those
constants there too?

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/ProgrammableInvoker.java line 344:

> 342:                                 Map<VMStorage, Integer> argIndexMap,
> 343:                                 Map<VMStorage, Integer> retIndexMap) throws Throwable {
> 344:         SharedUtils.Allocator unboxAllocator = bufferCopySize != 0

Nice! I also like THROWING_ALLOCATOR more than `null`

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

Marked as reviewed by mcimadamore (Committer).

PR: https://git.openjdk.java.net/panama-foreign/pull/375