8290204: FP16 initial backend implementation #204

smita-kamath · 2022-07-12T21:52:29Z

Initial backend implementation for enabling FP16.

Progress

Change must not contain extraneous whitespace
Change must be properly reviewed (1 review required, with at least 1 Committer)

Issue

JDK-8290204: FP16 initial backend implementation

Reviewers

Jatin Bhateja (@jatin-bhateja - Committer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/panama-vector pull/204/head:pull/204
$ git checkout pull/204

Update a local copy of the PR:
$ git checkout pull/204
$ git pull https://git.openjdk.org/panama-vector pull/204/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 204

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/panama-vector/pull/204.diff

bridgekeeper · 2022-07-12T21:53:15Z

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

mlbridge · 2022-07-12T22:14:24Z

Webrevs

jatin-bhateja

Hi Smita,
Please find my initial review comments.

Thanks

jatin-bhateja · 2022-07-14T17:13:56Z

src/hotspot/cpu/x86/assembler_x86.cpp

+void Assembler::evcvtph2pd(XMMRegister dst, XMMRegister src, int vector_len) {
+  assert(VM_Version::supports_avx512_fp16(), "");
+  InstructionAttr attributes(vector_len, /* rex_w */ false, /* legacy mode */ false, /* no_mask_reg */ true, /*uses_vl */ true);
+  attributes.set_is_evex_instruction();
+  int encode = vex_prefix_and_encode(dst->encoding(), 0, src->encoding(), VEX_SIMD_NONE, VEX_OPCODE_MAP5, &attributes);
+  emit_int8((unsigned char)0x5A);
+  emit_int8((unsigned char)(0xC0 | encode));
+}
+
+void Assembler::evcvtpd2ph(XMMRegister dst, XMMRegister src, int vector_len) {
+  assert(VM_Version::supports_avx512_fp16(), "");


It will be beneficial is we pass an extra opmask register to leaf level assembly routines for packed operations and have another wrapper without opmask which passes K0 as the opmask operand. It will help us while supporting predicated operations.

jatin-bhateja · 2022-07-14T17:26:38Z

src/hotspot/cpu/x86/vm_version_x86.hpp

@@ -363,7 +365,8 @@ class VM_Version : public Abstract_VM_Version {
    decl(HV,                "hv",                46) /* Hypervisor instructions */ \
    decl(SERIALIZE,         "serialize",         47) /* CPU SERIALIZE */ \
    decl(GFNI,              "gfni",              48) /* Vector GFNI instructions */ \
-    decl(AVX512_BITALG,     "avx512_bitalg",     49) /* Vector sub-word popcount and bit gather instructions */
+    decl(AVX512_BITALG,     "avx512_bitalg",     49) /* Vector sub-word popcount and bit gather instructions */\
+    decl(AVX512_FP16,       "avx512_fp16",       50) /* Vector FP16 instructions*/


Please also handle the newly added feature in
src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.amd64/src/jdk/vm/ci/amd64/AMD64.java

jatin-bhateja · 2022-07-14T17:29:49Z

src/hotspot/cpu/x86/x86.ad

+    case Op_AddVHF:
+    case Op_SubVHF:
+    case Op_MulVHF:
+    case Op_DivVHF:
+    case Op_AbsVHF:
+    case Op_NegVHF:
+      if(bt != T_SHORT && !VM_Version::supports_avx512_fp16()) {
+        return false;
+      }


Should it be moved Matcher::match_rule_supported_vector, backend does not support predicated half floats instructions currently.

jatin-bhateja · 2022-07-14T17:30:47Z

src/hotspot/cpu/x86/x86.ad

+    __ movdl($tmp1$$XMMRegister, $dst$$Register);
+    __ evaddsh($tmp1$$XMMRegister, $tmp1$$XMMRegister, $src2$$XMMRegister);
+    __ pshuflw($tmp$$XMMRegister, $src2$$XMMRegister, 0x01);
+    __ evaddsh($tmp1$$XMMRegister, $tmp1$$XMMRegister, $tmp$$XMMRegister);
+    __ pshuflw($tmp$$XMMRegister, $src2$$XMMRegister, 0x02);
+    __ evaddsh($tmp1$$XMMRegister, $tmp1$$XMMRegister, $tmp$$XMMRegister);
+    __ pshuflw($tmp$$XMMRegister, $src2$$XMMRegister, 0x03);
+    __ evaddsh($tmp1$$XMMRegister, $tmp1$$XMMRegister, $tmp$$XMMRegister);
+    __ pshufd($tmp2$$XMMRegister, $src2$$XMMRegister, 0x0E);
+    __ evaddsh($tmp1$$XMMRegister, $tmp1$$XMMRegister, $tmp2$$XMMRegister);
+    __ pshuflw($tmp$$XMMRegister, $tmp2$$XMMRegister, 0x01);
+    __ evaddsh($tmp1$$XMMRegister, $tmp1$$XMMRegister, $tmp$$XMMRegister);
+    __ pshuflw($tmp$$XMMRegister, $tmp2$$XMMRegister, 0x02);
+    __ evaddsh($tmp1$$XMMRegister, $tmp1$$XMMRegister, $tmp$$XMMRegister);
+    __ pshuflw($tmp$$XMMRegister, $tmp2$$XMMRegister, 0x03);
+    __ evaddsh($tmp1$$XMMRegister, $tmp1$$XMMRegister, $tmp$$XMMRegister);
+    __ movdl($dst$$Register, $tmp1$$XMMRegister);


We can move this into a macro assembly routine.

jatin-bhateja · 2022-07-14T17:36:34Z

src/hotspot/cpu/x86/x86.ad

@@ -4834,6 +4847,51 @@ instruct reduction16F(regF dst, legVec src, legVec vtmp1, legVec vtmp2) %{
  ins_pipe( pipe_slow );
 %}

+// =======================Half Float Reduction==========================================
+instruct reduction8HF(rRegI dst, vec src2, vec tmp, vec tmp1, vec tmp2) %{
+  predicate(UseAVX > 2);


This patterns handles 128 bit vector, in case we defer handling 256/512 bit vectors we should add a check in match_rule_supported_vector.

jatin-bhateja · 2022-07-14T17:44:39Z

src/hotspot/cpu/x86/x86.ad

+%}
+
+instruct vcvtDtoHF_reg(vec dst, vec src) %{
+  predicate(UseAVX > 2 && VM_Version::supports_avx512_fp16() && Matcher::vector_element_basic_type(n) == T_SHORT);


Same as above.

jatin-bhateja · 2022-07-14T17:44:51Z

src/hotspot/cpu/x86/x86.ad

+
+// Convert from other types to Halffloat
+instruct vcvtFtoHF_reg(vec dst, vec src) %{
+  predicate(UseAVX > 2 && Matcher::vector_element_basic_type(n) == T_SHORT);


Same as above.

jatin-bhateja · 2022-07-14T17:45:34Z

src/hotspot/cpu/x86/x86.ad

+instruct vabsHF_reg(vec dst, vec src) %{
+  predicate(UseAVX > 2);
+  match(Set dst (AbsVHF  src));
+  format %{ "vandps $dst,$src\t# $dst = |$src| abs packedHF" %}


Incorrect format string.

jatin-bhateja · 2022-07-14T17:52:15Z

src/hotspot/share/opto/vectornode.cpp

+ReductionNode* ReductionNode::make(int vopc, Node *ctrl, Node* n1, Node* n2) {
+  switch (vopc) {
+  case Op_AddReductionVHF: return new AddReductionVHFNode(ctrl, n1, n2);
+  case Op_MulReductionVF: return new MulReductionVFNode(ctrl, n1, n2);


Backend handling is missing for MulReductionVF

jatin-bhateja · 2022-07-14T17:53:15Z

src/hotspot/cpu/x86/stubGenerator_x86_64.cpp

+    StubRoutines::x86::_vector_halffloat_sign_mask = generate_vector_fp_mask("vector_halffloat_sign_mask", 0x7FFF7FFF7FFF7FFF);
+    StubRoutines::x86::_vector_halffloat_sign_flip = generate_vector_fp_mask("vector_halffloat_sign_flip", 0x8000800080008000);
+


Should be added to 32 bit stubGenerators also.

Bhavana-Kilambi · 2022-10-20T10:28:17Z

src/hotspot/share/opto/vectornode.cpp

@@ -290,6 +290,47 @@ int VectorNode::replicate_opcode(BasicType bt) {
  }
 }

+// Return the vector operator for the specified scalar operation
+// and vector length for half float
+int VectorNode::opcode(int sopc) {


Hi, where is this function being referenced ? Should extra handling be added in this method -

panama-vector/src/hotspot/share/opto/vectorIntrinsics.cpp

Line 349 in 113ec5d

bool LibraryCallKit::inline_vector_nary_operation(int n) {

to generate a call to this function - VectorNode::opcode(int sopc) added in this PR ?

Hi Bhavana, apologies for the delay in responding to your comment. The code to stitch everything together will be added later.

Hi Smitha, one more question - Since HalfFloat is not primitive datatype, the check to make sure the elem_type is primitive type might fail in LibraryCallKit::inline_vector_nary_operation(int n). Will you also be handling this? Or will this PR be merged after Valhalla integration when halffloat becomes a primitive class?

Hi Bhavana, we plan to explore this after the integration of this PR.

jatin-bhateja

Few more comments added.

jatin-bhateja · 2023-01-03T11:05:55Z

src/hotspot/cpu/x86/assembler_x86.cpp

+}
+
+void Assembler::evcvtph2ps(XMMRegister dst, KRegister mask, XMMRegister src, int vector_len) {
+  assert(VM_Version::supports_evex(), "");


Asserts should also check for VL feature if vector_len < Assembler::AVX_512bit.

jatin-bhateja · 2023-01-03T11:06:26Z

src/hotspot/cpu/x86/assembler_x86.cpp

+}
+
+void Assembler::evcvtps2ph(XMMRegister dst, KRegister mask, XMMRegister src, int imm8, int vector_len) {
+  assert(VM_Version::supports_evex(), "");


Asserts should also check for VL feature if vector_len < Assembler::AVX_512bit.

jatin-bhateja · 2023-01-03T11:06:40Z

src/hotspot/cpu/x86/assembler_x86.cpp

@@ -2134,6 +2134,61 @@ void Assembler::evcvtqq2pd(XMMRegister dst, XMMRegister src, int vector_len) {
  emit_int16((unsigned char)0xE6, (0xC0 | encode));
 }

+void Assembler::evcvtph2pd(XMMRegister dst, KRegister mask, XMMRegister src, int vector_len) {
+  assert(VM_Version::supports_avx512_fp16(), "");


Asserts should also check for VL feature if vector_len < Assembler::AVX_512bit.

jatin-bhateja · 2023-01-03T11:06:49Z

src/hotspot/cpu/x86/assembler_x86.cpp

+}
+
+void Assembler::evcvtpd2ph(XMMRegister dst, KRegister mask, XMMRegister src, int vector_len) {
+  assert(VM_Version::supports_avx512_fp16(), "");


Asserts should also check for VL feature if vector_len < Assembler::AVX_512bit.

jatin-bhateja · 2023-01-03T11:08:24Z

src/hotspot/cpu/x86/assembler_x86.cpp

@@ -6133,6 +6188,15 @@ void Assembler::vaddss(XMMRegister dst, XMMRegister nds, XMMRegister src) {
  emit_int16(0x58, (0xC0 | encode));
 }

+void Assembler::evaddsh(XMMRegister dst, XMMRegister nds, XMMRegister src) {
+  assert(VM_Version::supports_avx512_fp16(), "");


Same as above.

jatin-bhateja · 2023-01-03T11:09:58Z

src/hotspot/cpu/x86/assembler_x86.cpp

+
+void Assembler::evfmadd231ph(XMMRegister dst, KRegister mask, XMMRegister src1, Address src2, int vector_len) {
+  assert(VM_Version::supports_avx512_fp16(), "");
+  assert(VM_Version::supports_avx512vl(), "");


Same as above.

jatin-bhateja · 2023-01-03T11:10:09Z

src/hotspot/cpu/x86/assembler_x86.cpp

@@ -6520,6 +6661,20 @@ void Assembler::vdivps(XMMRegister dst, XMMRegister nds, Address src, int vector
  emit_operand(dst, src);
 }

+void Assembler::evdivph(XMMRegister dst, KRegister mask, XMMRegister nds, XMMRegister src, int vector_len) {
+  assert(VM_Version::supports_avx512_fp16(), "");


Same as above.

jatin-bhateja · 2023-01-04T08:08:20Z

src/hotspot/cpu/x86/assembler_x86.cpp

@@ -6373,6 +6451,20 @@ void Assembler::vsubps(XMMRegister dst, XMMRegister nds, Address src, int vector
  emit_operand(dst, src);
 }

+void Assembler::evsubph(XMMRegister dst, KRegister mask, XMMRegister nds, XMMRegister src, int vector_len) {
+  assert(VM_Version::supports_avx512_fp16(), "");


Same as above.

jatin-bhateja · 2023-01-04T08:19:47Z

src/hotspot/cpu/x86/x86.ad

+    case Op_AddVHF:
+    case Op_SubVHF:
+    case Op_MulVHF:
+    case Op_DivVHF:
+    case Op_AbsVHF:
+    case Op_NegVHF:
+      if (bt != T_SHORT && !VM_Version::supports_avx512_fp16()) {
+        return false;
+      }
+      break;


What if target does not support AVX512BW and vector length is 512 bits, vector size compatibility check will fail upfront and we won't reach till this point, currently the BasicType for halffloat is T_SHORT,
https://github.com/openjdk/panama-vector/pull/204/files#diff-d6a3624f0f0af65a98a47378a5c146eed5016ca09b4de1acd0a3acc823242e82L1690

Though, this may not show up in reality since FP16 ISA comes along with next gen Xeons which support both AVX512_FP16 and AVX512BW.

jatin-bhateja · 2023-01-04T08:23:56Z

src/hotspot/cpu/x86/x86.ad

@@ -7313,6 +7419,53 @@ instruct vucast(vec dst, vec src) %{
  ins_pipe( pipe_slow );
 %}

+// Convert from Halffloat to other types
+instruct vcvtHFtoD_reg(vec dst, vec src) %{
+  predicate(UseAVX > 2 && VM_Version::supports_avx512_fp16() && Matcher::vector_element_basic_type(n) == T_DOUBLE);


Being a common IR, we need to add a complimentary check in the short vector casting.

jatin-bhateja · 2023-01-04T08:37:43Z

Over all code looks clean, in absence of missing link b/w expanders and backend we cannot test it over SW emulator/HW.

Bhavana-Kilambi · 2023-01-16T13:19:50Z

src/hotspot/share/opto/classes.hpp

 macro(FmaVD)
 macro(FmaVF)
 macro(DivVF)
 macro(DivVD)
+macro(DivVHF)


Hi, in a previous commit, the DIV operation wasn't added in the binary operations list for Halffloats here -

panama-vector/src/jdk.incubator.vector/share/classes/jdk/incubator/vector/HalffloatVector.java

Line 789 in 9113b20

private static BinaryOperation<HalffloatVector, VectorMask<Halffloat>> binaryOperations(int opc_) {

Although this PR is specifically for backend impl but the backend ISA for "div" operation might not be generated due to this operation not being added to the list of binaryoperations for Halffloat. Is it possible to please add it in this PR?
Thanks

Hi Bhavana, there will be a follow up PR that Swati will work on to add div to the list of binary ops for Halffloat.

openjdk · 2023-01-20T04:40:03Z

@smita-kamath This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8290204: FP16 initial backend implementation

Reviewed-by: jbhateja

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 1 new commit pushed to the vectorIntrinsics+fp16 branch:

9113b20: 8299373: Extend vector conversion APIs for Halffloat type.

Please see this link for an up-to-date comparison between the source branch of this pull request and the vectorIntrinsics+fp16 branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the vectorIntrinsics+fp16 branch, type /integrate in a new comment.

smita-kamath · 2023-01-20T04:44:11Z

/integrate

openjdk · 2023-01-20T04:45:48Z

Going to push as commit 7460d93.
Since your change was applied there has been 1 commit pushed to the vectorIntrinsics+fp16 branch:

9113b20: 8299373: Extend vector conversion APIs for Halffloat type.

Your commit was automatically rebased without conflicts.

openjdk · 2023-01-20T04:46:13Z

@smita-kamath Pushed as commit 7460d93.

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

Initial fp16 backend implementation

e418333

smita-kamath marked this pull request as ready for review July 12, 2022 21:53

openjdk bot added the rfr label Jul 12, 2022

jatin-bhateja reviewed Jul 14, 2022

View reviewed changes

Bhavana-Kilambi reviewed Oct 20, 2022

View reviewed changes

smita-kamath added 3 commits December 13, 2022 16:01

Merge vectorIntrinsics+fp16

90ff380

Addressed review comments

6cb10da

Updated leaf level assembly routines as per review comment

7677dbb

jatin-bhateja reviewed Jan 4, 2023

View reviewed changes

smita-kamath added 4 commits January 10, 2023 14:10

Updated instructions as per review comment

831b266

Updated cast instructs with new nodes

1ba2734

typo in D2HF node

60b9e15

Correction in HF2D node

a19f869

Bhavana-Kilambi reviewed Jan 16, 2023

View reviewed changes

smita-kamath added 3 commits January 18, 2023 22:25

32 bit build failure fix

00df24e

fix

65a9e19

fix for build failures

399b6d2

jatin-bhateja approved these changes Jan 20, 2023

View reviewed changes

openjdk bot added the ready label Jan 20, 2023

openjdk bot added the integrated label Jan 20, 2023

openjdk bot closed this Jan 20, 2023

openjdk bot removed ready rfr labels Jan 20, 2023

		StubRoutines::x86::_vector_halffloat_sign_mask = generate_vector_fp_mask("vector_halffloat_sign_mask", 0x7FFF7FFF7FFF7FFF);
		StubRoutines::x86::_vector_halffloat_sign_flip = generate_vector_fp_mask("vector_halffloat_sign_flip", 0x8000800080008000);

8290204: FP16 initial backend implementation #204

8290204: FP16 initial backend implementation #204

Uh oh!

Conversation

smita-kamath commented Jul 12, 2022 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Jul 12, 2022

Uh oh!

mlbridge bot commented Jul 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

jatin-bhateja left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bhavana-Kilambi Jan 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jatin-bhateja left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jatin-bhateja Jan 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smita-kamath commented Jul 12, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Jul 12, 2022 •

edited

Loading

Bhavana-Kilambi Jan 6, 2023 •

edited

Loading

jatin-bhateja left a comment •

edited

Loading

jatin-bhateja Jan 4, 2023 •

edited

Loading