On Wed, 8 Mar 2023 05:17:53 GMT, Vladimir Kozlov <k...@openjdk.org> 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