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

[Jorn Vernee]

On Thu, 8 Oct 2020 20:25:31 GMT, Maurizio Cimadamore <mcimadamore 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
>
> src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/ProgrammableInvoker.java line 344:
> 
>> 342:                                 Map<VMStorage, Integer> retIndexMap) throws Throwable {
>> 343:         boolean hasBufferCopies = bufferCopySize != 0;
>> 344:         NativeScope scope = hasBufferCopies ? NativeScope.boundedScope(bufferCopySize) : null;
> 
> If Allocator had a close() method, this could be simplified a little?

This is not really a problem with Allocator, it's a problem with having either a NativeScope, or having none (null),
which will result in an NPE when trying to reference `scope::allocateNative`. That's why this code is conditional.

> src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/ProgrammableInvoker.java line 219:
> 
>> 217:         MethodHandle specializedHandle = leafHandle; // initial
>> 218:
>> 219:         int argInsertPos = -1;
> 
> The more we tweak this routine, the more I'm convinced we should just spin specialized bytecode :-) - it's becoming
> increasingly hard to follow what's happening here. (not for this review - more longer term)

Yeah, especially the indexing is tricky since we have to walk the recipe in reverse, but we shouldn't have that problem
when spinning bytecode.

> src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/Binding.java line 230:
> 
>> 228:
>> 229:     enum Tag {
>> 230:         VM_STORE,
> 
> Should these be called REG_STORE and REG_LOAD? Ah, I guess we also have stack, right? Ok, I agree with the name - these
> are "moves" that only the VM can perform, makes sense.

REG_LOAD and REG_STORE also works for me. The name is derived from VMReg in the HotSpot code -> VMStorage in our
code -> VM_LOAD/VM_STORE. (So, REG works equally well).

> src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/Binding.java line 591:
> 
>> 589:      * The [type] must be one of byte, short, char, int, long, float, or double
>> 590:      */
>> 591:     public static class BuffferStore extends Dereference {
> 
> Typo, there are 3 `f`

Thanks!

> src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/CallingSequenceBuilder.java line 93:
> 
>> 91:
>> 92:         for (Binding b : bindings) {
>> 93:             if (b.tag() == Binding.Tag.VM_LOAD
> 
> We could have static EnumSet for what's an box vs. unbox binding, so that you can filter by checking for presence in
> the right enum set?

Yeah, that's a good idea.

> src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/ProgrammableUpcallHandler.java line 102:
> 
>> 100:                                 : bufferBase.asSlice(layout.argOffset(storage));
>> 101:                             return SharedUtils.read(ptr, type);
>> 102:                         }, DEFAULT_ALLOCATOR);
> 
> For upcalls, couldn't we use a `NativeScope` and close at the end of the call? If the callback leaks segments
> outside... well, too bad.

Yes, exactly. But, seemed like a separate patch.

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

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