8302976: C2 intrinsification of Float.floatToFloat16 and Float.float16ToFloat yields different result than the interpreter #12869

vnkozlov · 2023-03-03T21:41:35Z

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

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8302976: C2 intrinsification of Float.floatToFloat16 and Float.float16ToFloat yields different result than the interpreter

Reviewers

Sandhya Viswanathan (@sviswa7 - Reviewer) ⚠️ Review applies to 9302d4bc
Jatin Bhateja (@jatin-bhateja - Reviewer) ⚠️ Review applies to 9f4b2474
Vladimir Ivanov (@iwanowww - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/12869/head:pull/12869
$ git checkout pull/12869

Update a local copy of the PR:
$ git checkout pull/12869
$ git pull https://git.openjdk.org/jdk pull/12869/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12869

View PR using the GUI difftool:
$ git pr show -t 12869

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/12869.diff

…6ToFloat yields different result than the interpreter

bridgekeeper · 2023-03-03T21:42:21Z

👋 Welcome back kvn! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-03-03T21:45:10Z

@vnkozlov The following labels will be automatically applied to this pull request:

core-libs
hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

vnkozlov · 2023-03-04T07:17:31Z

/label remove hotspot

vnkozlov · 2023-03-04T07:17:45Z

/label add hotspot-compiler

openjdk · 2023-03-04T07:17:58Z

@vnkozlov
The hotspot label was successfully removed.

vnkozlov · 2023-03-04T07:18:07Z

/label add hotspot-runtime

openjdk · 2023-03-04T07:19:08Z

@vnkozlov
The hotspot-compiler label was successfully added.

openjdk · 2023-03-04T07:20:25Z

@vnkozlov
The hotspot-runtime label was successfully added.

…new test

vnkozlov · 2023-03-05T06:16:34Z

GHA failure on linux-x86 in test compiler/vectorization/runner/LoopRangeStrideTest.java is due to JDK-8303105

mlbridge · 2023-03-06T16:09:10Z

Webrevs

vnkozlov · 2023-03-06T23:54:44Z

@fyang, please help to verify that new tests passed on RISC-V with these changes and review these changes. Thanks!

I tested x86 (64- and 32-bit) and AArch64.

sviswa7 · 2023-03-07T00:19:03Z

@vnkozlov Thanks a lot for taking this up. Is the following in the PR description still true:
"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."
From the PR it looks to me that for x86_64 you have the changes in place for SharedRuntime and the same result is produced across SharedRuntime, interpreter, c1, and c2.
For x86 32-bit also things are consistent across. Only the SharedRuntime optimization doesnt happen for x86 32bit as StubRoutines::hf2f() and StubRoutines::f2hf() are set as null. The fallback is handled correctly in interpreter, c1, and c2.

vnkozlov · 2023-03-07T00:48:27Z

For x86 32-bit also things are consistent across. Only the SharedRuntime optimization doesnt happen for x86 32bit as StubRoutines::hf2f() and StubRoutines::f2hf() are set as null. The fallback is handled correctly in interpreter, c1, and c2.

Correct, it is consistent. Only optimization to calculate constant value during compile time is skipped. C2 will generate HW instruction for ConvF2HF node as if its input was not constant. That is it.

It is possible to add similar Stub routines for AArch64 and RISC-V to be called from C2 but I am not expert in those platforms so I skipped them.

It may be not clear but StubRoutines::hf2f() and StubRoutines::f2hf() are used only by C2 (through SharedRuntime) for constant values in ConvF2HFNode::Value() and ConvHF2FNode::Value()

vnkozlov · 2023-03-07T00:52:37Z

Note, I removed ConvF2HFNode::Identity() optimization because tests show that it produces different NaN results due to skipped conversion.

sviswa7 · 2023-03-07T01:04:00Z

src/hotspot/cpu/x86/macroAssembler_x86.hpp

+    // Instruction requires different XMM registers
+    vcvtps2ph(tmp, src, 0x04, Assembler::AVX_128bit);


vcvtps2ph can have source and destination as same. Did you mean to say here in the comment that "Instruction requires XMM register as destination"?

flt_to_flt16 is used in x86.ad instruction which requires preserving src register.
I did not want to add an other macroassembler instruction for src->src case.
I will add this to this comment.

sviswa7 · 2023-03-07T01:12:44Z

src/hotspot/cpu/x86/stubGenerator_x86_64.cpp

+  if (VM_Version::supports_f16c() || VM_Version::supports_avx512vl()) {
+    // For results consistency both intrinsics should be enabled.
+    if (vmIntrinsics::is_intrinsic_available(vmIntrinsics::_float16ToFloat) &&
+        vmIntrinsics::is_intrinsic_available(vmIntrinsics::_floatToFloat16)) {


Should this also check for InlineIntrinsics?

vmIntrinsics::disabled_by_jvm_flags() checks InlineIntrinsics. See vmIntrinsics.cpp changes.

Yes you are right.

sviswa7 · 2023-03-07T01:16:56Z

src/hotspot/cpu/x86/templateInterpreterGenerator_x86_64.cpp

+    return nullptr; // Generate a vanilla entry
+  }
+  // For AVX CPUs only. f16c support is disabled if UseAVX == 0.
+  if (VM_Version::supports_f16c() || VM_Version::supports_avx512vl()) {


We could check for VM_Version::supports_float16() here instead.

Yes. And I need to remove !InlineIntrinsics check at line 340.

sviswa7 · 2023-03-07T01:17:34Z

src/hotspot/cpu/x86/stubGenerator_x86_64.cpp

@@ -3874,6 +3925,15 @@ void StubGenerator::generate_initial() {
     StubRoutines::_updateBytesAdler32 = generate_updateBytesAdler32();
  }

+  if (VM_Version::supports_f16c() || VM_Version::supports_avx512vl()) {


We could check for VM_Version::supports_float16() here instead.

sviswa7 · 2023-03-07T01:24:47Z

Note, I removed ConvF2HFNode::Identity() optimization because tests show that it produces different NaN results due to skipped conversion.

Yes, removing the Identity optimization is correct. It doesn't hold for NaN inputs.

jatin-bhateja · 2023-03-07T18:28:46Z

Hi @vnkozlov , There is some discrepancy in results b/w interpreter, C1 and C2 for following case.

public class Foo {
  public static short bar(float f) {return Float.floatToFloat16(f);}
  public static void main(String[] args) {
	 System.out.println(Float.floatToRawIntBits(Float.float16ToFloat((short) 31745)));
	 System.out.println(bar(Float.float16ToFloat((short) 31745)));
  }
}

CPROMPT>java  -Xint -cp . Foo
2143297536                                                       // FP32 QNaN + significand preserved
32257                                                            // FP16 QNaN + significand preserved
CPROMPT>java -Xbatch -Xcomp -cp . Foo
2139103232                                                       // FP32 SNaN + significand preserved
31745                                                            // FP16 SNaN + significand preserved
CPROMPT>java -XX:-TieredCompilation -Xbatch -Xcomp -cp . Foo
2139103232                                                       // FP32  SNaN + significand preserved
32257                                                            // FP16  QNaN + significand preserved.

vnkozlov · 2023-03-07T18:38:23Z

Hi @vnkozlov , There is some discrepancy in results b/w interpreter, C1 and C2 for following case.

And that is fine. Consistency have to be preserved only during one run. Different runs with different flags (with disabled intrinsics, for example) may produce different results.

EDIT: I should have paid more attention to the example outputs. The third (C2) run produces inconsistent result!

vnkozlov · 2023-03-07T18:46:14Z

On other hand -Xint should not disable intrinsics. I will look.

vnkozlov · 2023-03-07T18:58:53Z

It looks like C1 compilation does not invoke intrinsics. Investigating.

vnkozlov · 2023-03-07T20:25:19Z

We should not allow JIT compilation of Float.float16ToFloat and Float.floatToFloat16 Java methods as we do for other math methods: abstractInterpreter.hpp#L144

What happens with @jatin-bhateja test is Float class is still not loaded when we trigger compilation for Foo::main method with -Xcomp flag.
C2 generates Uncommon trap at the very start of main() and code is deoptimized and goes to Interpreter which uses intrinsics.
C1 on other hand generates calls to Float.float16ToFloat and Float.floatToFloat16 in compiled Foo::main method and then compiles Float.float16ToFloat and Float.floatToFloat16 which are called.

The fix ix simple:

+++ b/src/hotspot/share/interpreter/abstractInterpreter.hpp
@@ -159,6 +159,8 @@ class AbstractInterpreter: AllStatic {
       case vmIntrinsics::_dexp  : // fall thru
       case vmIntrinsics::_fmaD  : // fall thru
       case vmIntrinsics::_fmaF  : // fall thru
+      case vmIntrinsics::_floatToFloat16       : // fall thru
+      case vmIntrinsics::_float16ToFloat       : // fall thru
       case vmIntrinsics::_Continuation_doYield : // fall thru
         return false;

test now produce the same result:

$ java -Xint Foo
2143297536
32257
$ java -Xcomp -XX:-TieredCompilation Foo
2143297536
32257
$ java -Xcomp -XX:TieredStopAtLevel=1 Foo
2143297536
32257

vnkozlov · 2023-03-07T20:32:27Z

C2 also compiled float16ToFloat. That is why we got FP32 SNaN result with -XX:-TieredCompilation in Jatin's example.

…oFloat16

vnkozlov · 2023-03-07T21:33:04Z

@jatin-bhateja I applied the fix. Please, verify.

RealFYang · 2023-03-08T01:53:14Z

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

vnkozlov · 2023-03-08T05:12:21Z

Thank you very much @RealFYang for testing changes and preparing patch. I applied your patch.

jatin-bhateja

Hi @vnkozlov , Thanks for explanations, looks good to me now.

iwanowww

Overall, looks good. Minor comments/suggestions follow.

iwanowww · 2023-03-08T19:04:01Z

src/hotspot/share/runtime/sharedRuntime.cpp

-  return (jshort)(sign_bit | ( ((exp + 15) << 10) + signif_bits ) );
+  assert(StubRoutines::f2hf() != nullptr, "floatToFloat16 intrinsic is not supported on this platform");
+  typedef jshort (*f2hf_stub_t)(jfloat x);
+  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?

I wanted isolate function type cast and assert in one place.
BTW the comment in assert should be "the stub is not implemented on this platform".

iwanowww · 2023-03-08T19:38:56Z

src/hotspot/share/opto/convertnode.cpp

-  if( t == Type::FLOAT ) return TypeInt::SHORT;
+  if (t == Type::TOP) return Type::TOP;
+  if (t == Type::FLOAT) return TypeInt::SHORT;
+  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.

This code is optimization: use stub to calculate constant value during compilation instead of generating HW instruction in compiled code. It is not required to have this stub for intensification to work - ConvF2HFNode will be processed normally and will use intrinsics code (HW instruction) defined in .ad file.
These stubs are used only here, not in C1 and not in Interpreter. As consequence these stubs implementation is optional and I implemented them only on x64. That is why I have this check.
I debated to not have them at all to not confuse people but they did improve performance a little.

Thanks for the clarifications. Now it makes much more sense.

Still, the mix of StubRoutines::f2hf() and SharedRuntime::f2hf() looks a bit confusing.

What if you move the wrapper to StubRoutines class instead? (JRT_LEAF et al stuff looks redundant here. Also, even though there are other arithmetic operations declared on StubRoutines, they provide default implementations universally available across all platforms. f2hf case is different since it exposes a platform-specific stub and its availability is limited.)

Or encapsulate the constant folding logic (along with the guard) into SharedRuntime and return Type* (instead of int/float scalar).

Or encapsulate the constant folding logic (along with the guard) into SharedRuntime and return Type* (instead of int/float scalar).

I take this particular suggestion back. SharedRuntime is compiler-agnostic while Type is C2-specific.

iwanowww · 2023-03-08T19:42:26Z

src/hotspot/share/opto/convertnode.cpp


+  const TypeInt *ti = t->is_int();
+  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.

It follows the same pattern as other nodes here: ConvF2INode::Value() vs ConvI2FNode::Value().
If you want to change it we need to do that in separate RFE for all methods here.
But I don't think we need to do that because Float/Double does not have range values as Integer types.
Float have only 3 types of value: FloatTop, FloatBot, FloatCon. So we don't need to check for constant if checked for TOP and BOT. For Integer we need to check bool is_con() const { return _lo==_hi; }.

vnkozlov

Thank you for review @iwanowww

vnkozlov · 2023-03-08T20:55:29Z

src/hotspot/share/opto/convertnode.cpp

-  if( t == Type::FLOAT ) return TypeInt::SHORT;
+  if (t == Type::TOP) return Type::TOP;
+  if (t == Type::FLOAT) return TypeInt::SHORT;
+  if (StubRoutines::f2hf() == nullptr) return bottom_type();


This code is optimization: use stub to calculate constant value during compilation instead of generating HW instruction in compiled code. It is not required to have this stub for intensification to work - ConvF2HFNode will be processed normally and will use intrinsics code (HW instruction) defined in .ad file.
These stubs are used only here, not in C1 and not in Interpreter. As consequence these stubs implementation is optional and I implemented them only on x64. That is why I have this check.
I debated to not have them at all to not confuse people but they did improve performance a little.

vnkozlov · 2023-03-08T21:11:08Z

src/hotspot/share/opto/convertnode.cpp


+  const TypeInt *ti = t->is_int();
+  if (ti->is_con()) {


It follows the same pattern as other nodes here: ConvF2INode::Value() vs ConvI2FNode::Value().
If you want to change it we need to do that in separate RFE for all methods here.
But I don't think we need to do that because Float/Double does not have range values as Integer types.
Float have only 3 types of value: FloatTop, FloatBot, FloatCon. So we don't need to check for constant if checked for TOP and BOT. For Integer we need to check bool is_con() const { return _lo==_hi; }.

src/hotspot/cpu/x86/stubGenerator_x86_64.cpp

vnkozlov · 2023-03-08T22:51:01Z

What if you move the wrapper to StubRoutines class instead?
Okay, I will try it.

iwanowww

Looks good.

vnkozlov · 2023-03-09T00:06:40Z

Thank you, Sandhya, Jatin, Vladimir and Dean for review.

vnkozlov · 2023-03-09T03:25:48Z

/integrate

openjdk · 2023-03-09T03:26:49Z

Going to push as commit 8cfd74f.
Since your change was applied there have been 59 commits pushed to the master branch:

02875e7: 8171156: Class java.util.LocaleISOData has outdated information for country Code NP
ad326fc: 8299570: [JVMCI] Insufficient error handling when CodeBuffer is exhausted
05ceb37: 8303833: java.util.LocaleISOData has wrong comments for 'Norwegian Bokmål' and 'Volapük'
25de222: 8303839: [BACKOUT] JDK-8302799 and JDK-8302189
5b43804: 8282201: Consider removal of expiry check in VerifyCACerts.java test
f813dc7: 8302508: Add timestamp to the output TraceCompilerThreads
404d5bd: 8302161: Upgrade jQuery UI to version 1.13.2
d287a5e: 8303617: update for deprecated sprintf for jdk.jdwp.agent
ddcb369: 8303605: Memory leaks in Metaspace gtests
56512cf: 8303682: Simplify HttpClient DebugLogger
... and 49 more: https://git.openjdk.org/jdk/compare/5b2e2e4695768a6bd8090fb9a6c342fcddcbb3fd...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-03-09T03:27:04Z

@vnkozlov Pushed as commit 8cfd74f.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8302976: C2 intrinsification of Float.floatToFloat16 and Float.float1…

c0ff8e7

…6ToFloat yields different result than the interpreter

openjdk bot added hotspot hotspot-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Mar 3, 2023

vnkozlov added 2 commits March 3, 2023 14:19

Merge branch 'master' into 8302976

df1f991

Implement Aarch64 and RiscV parts, remove 32-bit x86 runtime stub

3616ee8

openjdk bot removed the hotspot hotspot-dev@openjdk.org label Mar 4, 2023

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Mar 4, 2023

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Mar 4, 2023

vnkozlov added 3 commits March 4, 2023 09:49

Update Copyright year. Add missing UnlockDiagnosticVMOptions flag in …

9c878bb

…new test

Check float16 instructions support on platforms for C1

90ca775

Copyright year update

1886e33

Remove ConvF2HFNode::Identity(). Updated tests

2eb47bf

vnkozlov marked this pull request as ready for review March 6, 2023 16:02

openjdk bot added the rfr Pull request is ready for review label Mar 6, 2023

sviswa7 reviewed Mar 7, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 7, 2023

Do not allow JIT compilation of Float.float16ToFloat and Float.floatT…

ed01863

…oFloat16

openjdk bot removed the ready Pull request is ready to be integrated label Mar 7, 2023

openjdk bot added the ready Pull request is ready to be integrated label Mar 7, 2023

Remove RISC-V port code for float16 intrinsics

9f4b247

jatin-bhateja approved these changes Mar 8, 2023

View reviewed changes

iwanowww reviewed Mar 8, 2023

View reviewed changes

vnkozlov commented Mar 8, 2023

View reviewed changes

dean-long reviewed Mar 8, 2023

View reviewed changes

src/hotspot/cpu/x86/stubGenerator_x86_64.cpp Show resolved Hide resolved

Remove SharedRuntime::f2hf and hf2f wrapper functions

fa79994

iwanowww approved these changes Mar 8, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Mar 9, 2023

openjdk bot closed this Mar 9, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 9, 2023

vnkozlov deleted the 8302976 branch March 9, 2023 03:41

		// Instruction requires different XMM registers
		vcvtps2ph(tmp, src, 0x04, Assembler::AVX_128bit);

8302976: C2 intrinsification of Float.floatToFloat16 and Float.float16ToFloat yields different result than the interpreter #12869

8302976: C2 intrinsification of Float.floatToFloat16 and Float.float16ToFloat yields different result than the interpreter #12869

Conversation

vnkozlov commented Mar 3, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Mar 3, 2023

openjdk bot commented Mar 3, 2023

vnkozlov commented Mar 4, 2023

vnkozlov commented Mar 4, 2023

openjdk bot commented Mar 4, 2023

vnkozlov commented Mar 4, 2023

openjdk bot commented Mar 4, 2023

openjdk bot commented Mar 4, 2023

vnkozlov commented Mar 5, 2023

mlbridge bot commented Mar 6, 2023 • edited Loading

Webrevs

vnkozlov commented Mar 6, 2023

sviswa7 commented Mar 7, 2023

vnkozlov commented Mar 7, 2023 • edited Loading

vnkozlov commented Mar 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sviswa7 commented Mar 7, 2023

jatin-bhateja commented Mar 7, 2023 • edited Loading

vnkozlov commented Mar 7, 2023 • edited Loading

vnkozlov commented Mar 7, 2023

vnkozlov commented Mar 7, 2023

vnkozlov commented Mar 7, 2023

vnkozlov commented Mar 7, 2023

vnkozlov commented Mar 7, 2023

RealFYang commented Mar 8, 2023 • edited Loading

vnkozlov commented Mar 8, 2023

jatin-bhateja left a comment

Choose a reason for hiding this comment

iwanowww left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vnkozlov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vnkozlov commented Mar 8, 2023

iwanowww left a comment

Choose a reason for hiding this comment

vnkozlov commented Mar 9, 2023 • edited Loading

vnkozlov commented Mar 9, 2023

openjdk bot commented Mar 9, 2023

openjdk bot commented Mar 9, 2023

vnkozlov commented Mar 3, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 6, 2023 •

edited

Loading

vnkozlov commented Mar 7, 2023 •

edited

Loading

jatin-bhateja commented Mar 7, 2023 •

edited

Loading

vnkozlov commented Mar 7, 2023 •

edited

Loading

RealFYang commented Mar 8, 2023 •

edited

Loading

vnkozlov commented Mar 9, 2023 •

edited

Loading