[foreign-abi] RFR 8228486: Add ABI-specific layout constants

[Maurizio Cimadamore]

On 23/07/2019 14:07, Jorn Vernee wrote:
> The tests pass on Windows. Overall the patch looks good
>
> - in TestUpcall you seem to be using the SysV.C_POINTER constant 
> instead of the magic one.
>
> - Do we need to distinguish between POINTER and INTEGER class? 
> Shouldn't this just be about in which register it's passed? It looks 
> like the isPointer predicate is only used in the tests in order to 
> construct dummy arguments, is there another way around this? (e.g. 
> checking that the carrier is a MemoryAddress).

Some ABI (e.g. SysV) do feature separate classes for INTEGER and 
POINTER, although in practice there's not much difference. I think I'd 
prefer to leave the pointer class there, at least for now.

I think moving forward we should think harder of what argument class 
really mean. I think originally there was some over-ambitious assumption 
that we could share classes across different ABIs. While some classes 
indeed do overlap, I think it's better to think that each ABI will just 
define its own classes.

But this ripples down to the ShuffleRecipe - e.g. ATM ShuffleRecipe is 
using a shared classification for all ABIs - which means universal 
invoker is the same on both x64 and aarch64 - which is probably, again, 
wrong.

I think that, ideally, we would like to have one ArgumentClassImpl per 
ABI, with its own enum constants, and let these constants also be the 
ones used for ShuffleRecipe. That is, ShuffleRecipe can work with 
different number of classes/argument types depending on the ABI.

At the same time, I'm mindful that spending too many cycles improving 
this is probably not worth it, since we want to switch to linkToNative 
anyway...

>
> Some things that I can look into:
>
> * We can mark vararg arguments with a layout annotation on Windows to 
> be able to support vargargs on Windows as well.

Yeah - that would be a natural next step.

Perhaps SystemABI could have an extra method (which is no-op in all ABI 
but windows) to create a variadic-ready layout.


> * I think some of the code in Window's CallingSequenceBuilderImpl 
> could be removed at this point (e.g. the x87 stuff), since it's not 
> supported by the Windows C ABI any ways.

Yep - this is a bit related to the point I made above vs. universal 
invoker improvements/sharing.


Maurizio

>
> Jorn
>
> On 2019-07-22 19:23, Maurizio Cimadamore wrote:
>> Hi,
>> the recent push for JDK-8228447, as expected, broke the foreign-abi
>> branch, since that branch depended on the ValueLayout::isIntegral
>> accessor.
>>
>> This patch replaces that logic with the custom layout annotation logic
>> discussed in [1]:
>>
>> http://cr.openjdk.java.net/~mcimadamore/panama/8228486/
>>
>> This patch makes the following changes:
>>
>> * introduces three set of annotated layout constants, one for 
>> supported ABI
>> * the annotations point directly at the ABI-specific argument classes
>> * I've consolidated ArgumentClass a bit, so that now there's a common,
>> ABI-agnostic super-interface with some general predicates
>> * AddressLayout has been removed (we can just look for 
>> argumentClass::isPointer)
>> * the tests have been tweaked to use the new ABI-specific constants;
>> since the test has to work across platforms, all ABI test use a new
>> interface which imports the 'right' set of constants
>>
>> While the patch might be a bit raw (I've only tested on Linux x64), I
>> like where this is going.
>>
>> Maurizio