RFR: jextract generates BigDecimal for "long double" but binder fails to handle it

[Jorn Vernee]

Maurizio Cimadamore schreef op 2018-11-05 13:09:
> On 05/11/2018 11:47, Jorn Vernee wrote:
>> Hi,
>> 
>> I can't run the tests on this, but I thought I'd review any ways since 
>> I've spent a lot of time with the same code recently.
>> 
>> LongDoubleComplex.java:
>> 
>>     Not sure this diff should be in here, the rhs is what I already 
>> have locally.
> 
> Not sure what you mean - the tip of LongDoubleComplex is using 
> BigDecimal:
> 
> http://hg.openjdk.java.net/panama/dev/file/2e5e621e70d7/src/java.base/share/classes/java/foreign/memory/LongDoubleComplex.java
> 

Yep that was my bad, sorry.

>> 
>> Util.java:
>> 
>>     Somewhat confusing to do the get and set operations in one method, 
>> but that's up to you :)
>> 
>> - I think you've forgotten the diff for 
>> jdk/internal/foreign/abi/sysv/x64/StorageNames.java ?
> No, I just didn't change that file - is it used for debugging? I'll 
> check

It's used for debugging to output the register names chosen by 
CallingSequenceBuilder:

     if (DEBUG) {
         System.out.println("Argument " + arg.getName() + " will be 
passed in register " + StorageNames.getStorageName(storage));
     }

You've added new storage classes, so StorageNames.java needs to be 
updated to recognize those new classes and return the right names.

>> 
>> - libcomplex_aux.c, libcomplex_aux.h, and libLongDouble.c are missing 
>> copyright headers.
>> 
>> Rest looks good to me.
>> 
>> On a general note: Did you consider rolling your own 
>> `Struct<LongDouble>` type as a carrier?
> 
> I think that's a possibility and yes, I've considered it, but it needs
> to have enough methods on it so that people can do numeric computation
> w/o projecting into a BigDecimal? And probably we'd like it to be a
> valhalla value type too...

Maybe, or you could leave it at just 2 getters and setter for the 2 
component 'doubles'. I'd expect most usage to be on the native side any 
ways, and you just need to have a way to have a handle to the value on 
the Java side as well. But if someone really wanted to they could access 
the whole value on the Java side as well (e.g. to print it out for 
debugging purposes, not necessarily to do math with), they'd be 
accepting the slow-path.

Could also just wait and see what the feedback is on this, and revisit 
later if needed.

Jorn

> Maurizio
> 
>> 
>> Cheers,
>> Jorn
>> 
>> Maurizio Cimadamore schreef op 2018-11-05 11:27:
>>> Hi,
>>> this patch adds support for X87 values in the binder
>>> 
>>> http://cr.openjdk.java.net/~mcimadamore/panama/8213029/
>>> 
>>> The Hotspot changes are straightforward; there's a new X87 shuffle
>>> recipe category which needs to be taken into account (its size is 128
>>> bits). Changes are needed on the binder side too as the X87 arguments
>>> have to be properly classified.
>>> 
>>> More specifically, the ABI is very subtle when it comes to 'complex'
>>> values; they are generally passed in stack slots (if they are in
>>> argument position) but they are passed in registers (X87 st0 for 
>>> 'long
>>> double' or both 'st0' and 'st1' for 'long double complex') for return
>>> values. To achieve this, we need to teach the binder about the 
>>> special
>>> LongDoubleComplex struct, so that the right classification occurs 
>>> (see
>>> changes to CallingSequenceBuilderImpl).
>>> 
>>> Another issue is how to handle X87 values in Java - I started out
>>> trying to map these values to BigDecimal, but after getting it to 
>>> work
>>> I realized that there were too many unanswered questions: one
>>> performance side, to decode an X87 value into a BigDecimal you have 
>>> to
>>> create lots of intermediate BigIntegers/BigDecimal; on the
>>> expressiveness side, BigDecimal is lacking support for NaN/Infinity,
>>> making it an improper fit.
>>> 
>>> I then decided to 'erase' X87 values down to 'double' (using a JNI
>>> helper function to do the conversion, which should be efficient as it
>>> can take advantage of platform specific instructions such as flstp
>>> etc.).
>>> 
>>> I tested it with fast-debug to make sure there were no weird hotspot
>>> failures, but please give it a spin to make sure it all works.
>>> 
>>> I also updates existing complex tests to add more cases and upcall 
>>> support.
>>> 
>>> Cheers
>>> 
>>> Maurizio