On Wed, 8 Mar 2023 05:17:53 GMT, Vladimir Kozlov <[email protected]> wrote:
>> Implemented `Float.floatToFloat16` and `Float.float16ToFloat` intrinsics in
>> Interpreter and C1 compiler to produce the same results as C2 intrinsics on
>> x64, Aarch64 and RISC-V - all platforms where C2 intrinsics for these Java
>> methods were implemented originally.
>>
>> Replaced `SharedRuntime::f2hf()` and `hf2f()` C runtime functions with calls
>> to runtime stubs which use the same HW instructions as C2 intrinsics. Only
>> for 64-bit x64 because 32-bit x86 stub does not work: result is passed
>> through FPU register and NaN values become different from C2 intrinsic. This
>> runtime stub is only used to calculate constant values during C2 compilation
>> and can be skipped.
>>
>> I added new tests based on Tobias's `TestAll.java` And copied
>> `jdk/lang/Float/Binary16Conversion*.java` tests to run them with `-Xcomp` to
>> make sure code is compiled by C1 or C2. I modified
>> `Binary16ConversionNaN.java` to compare results from Interpreter, C1 and C2.
>>
>> Tested tier1-5, Xcomp, stress
>
> Vladimir Kozlov has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Remove RISC-V port code for float16 intrinsics
Overall, looks good. Minor comments/suggestions follow.
src/hotspot/share/opto/convertnode.cpp line 171:
> 169: if (t == Type::TOP) return Type::TOP;
> 170: if (t == Type::FLOAT) return TypeInt::SHORT;
> 171: if (StubRoutines::f2hf() == nullptr) return bottom_type();
What's the purpose of this check? My understanding is ConvF2HF/ConvHF2F require
intrinsification and on platforms where stubs are absent, intrinsification is
disabled.
src/hotspot/share/opto/convertnode.cpp line 244:
> 242:
> 243: const TypeInt *ti = t->is_int();
> 244: if (ti->is_con()) {
I find it confusing that `ConvHF2FNode::Value()` has `is_con()` check, but
`ConvF2HFNode::Value()`doesn't. I'd prefer to see both implementations unified.
src/hotspot/share/runtime/sharedRuntime.cpp line 451:
> 449: assert(StubRoutines::f2hf() != nullptr, "floatToFloat16 intrinsic is
> not supported on this platform");
> 450: typedef jshort (*f2hf_stub_t)(jfloat x);
> 451: return ((f2hf_stub_t)StubRoutines::f2hf())(x);
What's the point of keeping the wrappers around? The stubs can be called
directly, can't they?
-------------
PR: https://git.openjdk.org/jdk/pull/12869
