On Tue, 7 Mar 2023 21:38:38 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:
>
> Do not allow JIT compilation of Float.float16ToFloat and
> Float.floatToFloat16
Hi, Thanks for handling linux-riscv64 at the same time.
Bad news is that we witnessed test failures when running following test with
QEMU (no riscv hardware available with Zfhmin extension for now):
test/hotspot/jtreg/compiler/intrinsics/float16/TestAllFloat16ToFloat.java
Exception in thread "main" java.lang.RuntimeException: Inconsistent result for
Float.floatToFloat16(NaN/7fc00000): 7e00 != fc01
at TestAllFloat16ToFloat.verify(TestAllFloat16ToFloat.java:62)
at TestAllFloat16ToFloat.run(TestAllFloat16ToFloat.java:72)
at TestAllFloat16ToFloat.main(TestAllFloat16ToFloat.java:94)
It looks like there is a problem when handling NaNs with fcvt.h.s/fmv.x.h and
fmv.h.x/fcvt.s.h instructions at the bottom.
It's also possible to be an issue of QEMU as well. It would take quite a while
to diagnose. But I don't want this to block this PR.
So I would prefer removing support of this feature for this port and adding
back once this is resolved. And I have prepared a patch for that purpose. See
attachement.
[12869-revert-riscv.txt](https://github.com/openjdk/jdk/files/10915606/12869-revert-riscv.txt)
-------------
PR: https://git.openjdk.org/jdk/pull/12869
