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

[Jorn Vernee]

On Thu, 8 Oct 2020 20:11:21 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/Binding.java line 303:
> 
>> 301:
>> 302:     // FIXME should register with scope
>> 303:     private static MemorySegment toSegment(MemoryAddress operand, long size) {
> 
> This seems to be only used inside the `ToSegment` binding, so, perhaps this could should be moved there (also because
> this is an interpreter function, not a binding factory - e.g. the overload with the `toSegment` binding factory is
> confusing)

Yeah, good idea. This and 2 others are used for the `specialize` implemantion, but they can be moved inside the binding
classes.

> src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/Binding.java line 363:
> 
>> 361:
>> 362:     @Override
>> 363:     public boolean equals(Object o) {
> 
> General question - where do we use equals/hash? Just for testing purposes right?

Yeah, we use them at least equals for the CallArranger tests, which is comparing arrays of Bindings.

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

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