On Mon, 13 Jan 2025 09:02:24 GMT, Jatin Bhateja <[email protected]> wrote:
>> Uncertain on what bits, but i am guessing it's mostly related to the
>> fallback code in the lambda. To avoid the intrinsics operating on Float16
>> instances we instead "unpack" the carrier (16bits) values and pass those as
>> arguments to the intrinsic. The fallback (when intrinsification is not
>> supported) also accepts those carrier values as arguments and we convert the
>> carriers to floats, operate on then, convert to the carrier, and then back
>> to float16 on the result.
>>
>> The code in the lambda could potentially be simplified if `Float16Math.fma`
>> accepted six arguments the first three being the carrier values used by the
>> intrinsic, and the subsequent three being the float16 values used by the
>> fallback. Then we could express the code in the original source in the
>> lambda. I believe when intrinsified there would be no penalty for those
>> extra arguments.
>
> Hi @PaulSandoz , In the current scheme we are passing unboxed carriers to
> intrinsic entry point, in the fallback implementation carrier type is first
> converted to floating point value using Float.float16ToFloat API which
> expects to receive a short type argument, after the operation we again
> convert float value to carrier type (short) using Float.floatToFloat16 API
> which expects a float argument, thus our intent here is to perform unboxing
> and boxing outside the intrinsic thereby avoiding all complexities around
> boxing by compiler. Even if we pass 3 additional parameters we still need to
> use Float16.floatValue which invokes Float.float16ToFloat underneath, thus
> this minor modification on Java side is on account of optimizing the
> intrinsic interface.
Yes, i understand the approach. It's about clarity of the fallback
implementation retaining what was expressed in the original code:
short res = Float16Math.fma(fa, fb, fc, a, b, c,
(a_, b_, c_) -> {
double product = (double)(a_.floatValue() *
b._floatValue());
return valueOf(product + c_.doubleValue());
});
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/22754#discussion_r1913502565
