8294588: Auto vectorize half precision floating point conversion APIs by smita-kamath · Pull Request #11471 · openjdk/jdk

smita-kamath · 2022-12-02T04:22:39Z

Hi All,

I have added changes for autovectorizing Float.float16ToFloat and Float.floatToFloat16 API's.
Following are the performance numbers of JMH micro Fp16ConversionBenchmark:
Before code changes:
Benchmark | (size) | Mode | Cnt | Score | Error | Units
Fp16ConversionBenchmark.float16ToFloat | 2048 | thrpt | 3 | 1044.653 | ±     0.041 | ops/ms
Fp16ConversionBenchmark.float16ToFloatMemory | 2048 | thrpt | 3 | 2341529.9 | ± 11765.453 | ops/ms
Fp16ConversionBenchmark.floatToFloat16 | 2048 | thrpt | 3 | 2156.662 | ±     0.653 | ops/ms
Fp16ConversionBenchmark.floatToFloat16Memory | 2048 | thrpt | 3 | 2007988.1 | ±   361.696 | ops/ms

After:
Benchmark | (size) | Mode | Cnt | Score | Error | Units
Fp16ConversionBenchmark.float16ToFloat | 2048 | thrpt | 3 | 20460.349 |± 372.327 | ops/ms
Fp16ConversionBenchmark.float16ToFloatMemory | 2048 | thrpt | 3 | 2342125.200 |± 9250.899 |ops/ms
Fp16ConversionBenchmark.floatToFloat16 | 2048 | thrpt | 3 | 22553.977 |± 483.034 | ops/ms
Fp16ConversionBenchmark.floatToFloat16Memory | 2048 | thrpt | 3 | 2007899.797 |± 150.296 | ops/ms

Kindly review and share your feedback.

Thanks.
Smita

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-8294588: Auto vectorize half precision floating point conversion APIs

Reviewers

Sandhya Viswanathan (@sviswa7 - Reviewer) ⚠️ Review applies to 09816f70
Vladimir Kozlov (@vnkozlov - Reviewer)
Jatin Bhateja (@jatin-bhateja - Reviewer) ⚠️ Review applies to 981ea9f4
Fei Gao (@fg1417 - Author)
Xiaohong Gong (@XiaohongGong - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11471

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

Using diff file

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

bridgekeeper · 2022-12-02T04:24:03Z

👋 Welcome back svkamath! 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.

smita-kamath · 2022-12-02T04:26:01Z

label /hotspot

openjdk · 2022-12-02T04:28:00Z

@smita-kamath The following label will be automatically applied to this pull request:

hotspot-compiler

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

mlbridge · 2022-12-02T06:47:52Z

Webrevs

jatin-bhateja · 2022-12-02T18:50:42Z

test/hotspot/jtreg/compiler/vectorization/TestFloatConversionsVector.java

+      System.out.println("PASSED");
+  }
+
+  @Test


New IR node checking annotations missing.

jatin-bhateja · 2022-12-02T18:53:24Z

src/hotspot/share/opto/vectornode.hpp

  virtual int Opcode() const;
 };

+class HF2FVNode : public VectorNode {


You may use same naming convention as used for other vector casting IR nodes
VectorCastH2F and F2H

jatin-bhateja · 2022-12-02T19:21:54Z

src/hotspot/cpu/x86/x86.ad

  ins_pipe( pipe_slow );
 %}

+instruct vconvF2HF(vec dst, vec src) %{


We do have a destination memory flavour of VCVTPS2PH, adding a memory pattern will fold subsequent store in one instruction.

sviswa7 · 2022-12-02T19:38:14Z

src/hotspot/cpu/x86/x86.ad

+    case Op_F2HFV:
      if (!VM_Version::supports_f16c() && !VM_Version::supports_avx512vl()) {


We need different check for vector flavors (HF2FV/F2HV) vs the scalar flavors (ConvF2HF/ConvHF2F).
The check needed for vector flavors is:
if (!VM_Version::supports_f16c() && !VM_Version::supports_avx512()) { return false; }

Also in vm_version_x86.cpp, the F16C features should be disabled when UseAVX is set to 0, i.e. the following
if (UseAVX < 1) {
_features &= ~CPU_AVX;
_features &= ~CPU_VZEROUPPER;
}
should be updated to:
if (UseAVX < 1) {
_features &= ~CPU_AVX;
_features &= ~CPU_VZEROUPPER;
_features &= ~CPU_F16C;
}

sviswa7 · 2022-12-02T19:39:38Z

src/hotspot/cpu/x86/x86.ad

+    case Op_HF2FV:
+    case Op_F2HFV:
+      if (!VM_Version::supports_f16c() && !VM_Version::supports_avx512vl()) {
+        return false;
+      }
+      break;


This can be removed as match_rule_supported() has previously happened.

sviswa7 · 2022-12-02T21:37:33Z

src/hotspot/cpu/x86/x86.ad

+    int src_size = Matcher::vector_length_in_bytes(this, $src);
+    int dst_size = src_size * 2;
+    int vlen_enc = vector_length_encoding(dst_size);


This could now be changed to:
int vlen_enc = Matcher::vector_length_encoding(this);

openjdk · 2022-12-06T18:33:50Z

@smita-kamath this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8294588
git fetch https://git.openjdk.org/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

sviswa7 · 2022-12-06T18:30:08Z

src/hotspot/cpu/x86/assembler_x86.cpp

 void Assembler::vcvtph2ps(XMMRegister dst, XMMRegister src, int vector_len) {
  assert(VM_Version::supports_avx512vl() || VM_Version::supports_f16c(), "");


This should be VM_Version::supports_evex(). Also same for vcvtps2ph.

sviswa7 · 2022-12-06T20:01:44Z

@smita-kamath The patch looks good to me. You will need another review.
@vnkozlov could you please help review this patch?

openjdk · 2022-12-06T20:03:24Z

@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:

8294588: Auto vectorize half precision floating point conversion APIs

Reviewed-by: sviswanathan, kvn, jbhateja, fgao, xgong

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 42 new commits pushed to the master branch:

1166c8e: 8296896: Change virtual Thread.yield to use external submit
5175965: 8298323: trivial typo in JOptionPane.OK_OPTION
d5cf18e: 8296198: JFileChooser throws InternalError java.lang.InternalError with Windows shortcuts
74f346b: 8298075: RISC-V: Implement post-call NOPs
3aa4070: 8294047: HttpResponseInputStream swallows interrupts
af8fb7e: 8282578: AIOOBE in javax.sound.sampled.Clip
8b69a2e: 8298099: [JVMCI] decouple libgraal from JVMCI module at runtime
8a9911e: 8295803: Console should be usable in jshell and other environments
5d4c71c: 8281236: (D)TLS key exchange named groups
10356e7: 8298303: (fs) temporarily remove Path.getExtension
... and 32 more: https://git.openjdk.org/jdk/compare/336d230a39e41eeed8b9d2ce3fec42e9de1d11fe...master

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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@sviswa7, @vnkozlov, @jatin-bhateja, @XiaohongGong) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

vnkozlov

Changes are straight-forward but I have few comments.

And we need to test it again.

vnkozlov · 2022-12-06T23:23:09Z

src/hotspot/cpu/x86/vm_version_x86.cpp

  if (UseAVX < 1) {
    _features &= ~CPU_AVX;
    _features &= ~CPU_VZEROUPPER;
+    _features &= ~CPU_F16C;


Is is_knights_family() supports f16c? We switch off some avx512 features for it. But it looks like f16c is not connected to avx512.

Hi Vladimir, you're correct that f16c is not connected to avx512.

vnkozlov · 2022-12-06T23:26:46Z

src/hotspot/cpu/x86/assembler_x86.cpp

+  assert(VM_Version::supports_evex() || VM_Version::supports_f16c(), "");
+  InstructionMark im(this);
+  InstructionAttr attributes(vector_len, /* rex_w */ false, /* legacy_mode */ false, /* no_mask_reg */ true, /*uses_vl */ true);
+  attributes.set_address_attributes(/* tuple_type */ EVEX_HVM, /* input_size_in_bits */ EVEX_NObit);


Is it correct to set EVEX_* attributes in case EVEX is switched off (by UseAVX flag)?

Or a CPU supports F16C but does not EVEX (avx512f).

Hi Vladimir, we have a prior example of vpaddb instruction where these attributes are set. The assembler will ignore these attributes if UseAVX < 3.

Good. Thank you for answering my questions.

jatin-bhateja · 2022-12-07T00:15:02Z

test/hotspot/jtreg/compiler/vectorization/TestFloatConversionsVector.java

+  }
+
+  @Test
+  @IR(counts = {IRNode.VECTOR_CAST_F2H, "> 0"}, applyIfCPUFeature = {"avx512f", "true"})


You can add "FC16" also in the feature list and use applyIfCPUFeaturesOr

This is good suggestion.

jatin-bhateja · 2022-12-07T00:17:13Z

test/hotspot/jtreg/compiler/vectorization/TestFloatConversionsVector.java

+ * @bug 8294588
+ * @summary Auto-vectorize Float.floatToFloat16, Float.float16ToFloat API's
+ * @requires vm.compiler2.enabled
+ * @requires vm.cpu.features ~= ".*avx.*"


Test may also execute on target if it has FC16, you can remove this CPU feature check. Feature since IR annotations already has a feature check.

jatin-bhateja

Verified my comments addressed. IR test is enabled for AVX, but can also be enabled for FC16 since some VM features can be selectively enabled in instances.

smita-kamath · 2022-12-07T21:44:58Z

@vnkozlov I have addressed comments from Fei Gao and Xiaohong Gong. I have limited vectorization to avx2 and higher. If the changes look good to you, could you kindly run the tests? Thanks for all your help.

vnkozlov · 2022-12-07T22:12:20Z

@vnkozlov I have addressed comments from Fei Gao and Xiaohong Gong. I have limited vectorization to avx2 and higher. If the changes look good to you, could you kindly run the tests? Thanks for all your help.

@smita-kamath, can you explain why it does not work with AVX1? If it really requires AVX2 then you should just disable F16C for (AVX < 2) instead of current (AVX < 1) in vm_version_x86.cpp. And you would not need to modify .ad file and test.

smita-kamath · 2022-12-07T22:44:14Z

@vnkozlov you are right. It should work with AVX=1. I will make the changes. Thank you for your comment.

smita-kamath · 2022-12-07T23:36:25Z

@vnkozlov I have updated the test case to work with AVX=1.

vnkozlov · 2022-12-07T23:54:50Z

@vnkozlov I have updated the test case to work with AVX=1.

Can you explain what was wrong with AVX1 and what change fixed the issue?
I see you renamed classes and addressed @fg1417 comment about opcode. It is not clear to me what fixed AVX1 issue.

sviswa7 · 2022-12-08T00:27:42Z

@vnkozlov The test was failing earlier with -XX:UseAVX=1 because the right implemented() check was not happening as Fei Gao explained. In vectornode.cpp, method VectorCastNode::implemented() was not getting the right vopc (VectorCastF2X, VectorCastS2X instead of VectorCastF2HF and VectorCastHF2F) after call to VectorCastNode::opcode() and so the Matcher::match_rule_supported_superword() was called with wrong vopc. This is now fixed as Smita has fixed the VectorCastNode::opcode() and VectorCastNode::implemented().

vnkozlov · 2022-12-08T00:36:22Z

Thank you @sviswa7 for explanation! Good.

vnkozlov · 2022-12-08T00:37:48Z

I started new testing after verifying locally that test passed with -XX:UseAVX=1.

jatin-bhateja · 2022-12-08T02:10:02Z

@vnkozlov The test was failing earlier with -XX:UseAVX=1 because the right implemented() check was not happening as Fei Gao explained. In vectornode.cpp, method VectorCastNode::implemented() was not getting the right vopc (VectorCastF2X, VectorCastS2X instead of VectorCastF2HF and VectorCastHF2F) after call to VectorCastNode::opcode() and so the Matcher::match_rule_supported_superword() was called with wrong vopc. This is now fixed as Smita has fixed the VectorCastNode::opcode() and VectorCastNode::implemented().

Also, the IR test was only enabled for avx512f earlier, which some how over shadowed the problem. Since VM features are queried using CPUID hence matcher will give up if both F16C and AVX512F are not present. Hi @smita-kamath , we should not explicitly disable the F16C in vm_version.

sviswa7 · 2022-12-08T02:58:33Z

@vnkozlov The test was failing earlier with -XX:UseAVX=1 because the right implemented() check was not happening as Fei Gao explained. In vectornode.cpp, method VectorCastNode::implemented() was not getting the right vopc (VectorCastF2X, VectorCastS2X instead of VectorCastF2HF and VectorCastHF2F) after call to VectorCastNode::opcode() and so the Matcher::match_rule_supported_superword() was called with wrong vopc. This is now fixed as Smita has fixed the VectorCastNode::opcode() and VectorCastNode::implemented().

Also, the IR test was only enabled for avx512f earlier, which some how over shadowed the problem. Since VM features are queried using CPUID hence matcher will give up if both F16C and AVX512F are not present. Hi @smita-kamath , we should not explicitly disable the F16C in vm_version.

@jatin-bhateja When User sets -XX:UseAVX=0 on command line F16C needs to be disabled explicitly (in vm_version) as it needs AVX support.

jatin-bhateja · 2022-12-08T03:15:07Z

@vnkozlov The test was failing earlier with -XX:UseAVX=1 because the right implemented() check was not happening as Fei Gao explained. In vectornode.cpp, method VectorCastNode::implemented() was not getting the right vopc (VectorCastF2X, VectorCastS2X instead of VectorCastF2HF and VectorCastHF2F) after call to VectorCastNode::opcode() and so the Matcher::match_rule_supported_superword() was called with wrong vopc. This is now fixed as Smita has fixed the VectorCastNode::opcode() and VectorCastNode::implemented().

Also, the IR test was only enabled for avx512f earlier, which some how over shadowed the problem. Since VM features are queried using CPUID hence matcher will give up if both F16C and AVX512F are not present. Hi @smita-kamath , we should not explicitly disable the F16C in vm_version.

@jatin-bhateja When User sets -XX:UseAVX=0 on command line F16C needs to be disabled explicitly (in vm_version) as it needs AVX support.

Thanks you @sviswa7 for explanation!

vnkozlov · 2022-12-08T05:11:16Z

Unfortunately I have to restart testing because JTREG version was update but I did not update my local repo which caused half of tests failed with "harness" error :^(

vnkozlov · 2022-12-08T05:13:45Z

Good news is the test passed in this testing (hotspot vector testing passed a whole).

smita-kamath · 2022-12-08T05:33:28Z

@vnkozlov, Thanks so much for running the tests. I really appreciate your help.

fg1417

Thanks for your update. The change involving superword and vectornode parts looks good to me now.

XiaohongGong

LGTM, thanks for the update!

vnkozlov

Latest testing results are good.

smita-kamath · 2022-12-08T09:06:51Z

@vnkozlov Thanks a lot for your review comments and for testing this patch.

smita-kamath · 2022-12-08T09:08:27Z

/integrate

openjdk · 2022-12-08T09:09:29Z

@smita-kamath
Your change (at version dc7d728) is now ready to be sponsored by a Committer.

jatin-bhateja · 2022-12-08T09:44:58Z

/sponsor

openjdk · 2022-12-08T09:47:18Z

Going to push as commit 073897c.
Since your change was applied there have been 43 commits pushed to the master branch:

46cd457: 8298341: Ensure heap growth in TestNativeMemoryUsageEvents.java
1166c8e: 8296896: Change virtual Thread.yield to use external submit
5175965: 8298323: trivial typo in JOptionPane.OK_OPTION
d5cf18e: 8296198: JFileChooser throws InternalError java.lang.InternalError with Windows shortcuts
74f346b: 8298075: RISC-V: Implement post-call NOPs
3aa4070: 8294047: HttpResponseInputStream swallows interrupts
af8fb7e: 8282578: AIOOBE in javax.sound.sampled.Clip
8b69a2e: 8298099: [JVMCI] decouple libgraal from JVMCI module at runtime
8a9911e: 8295803: Console should be usable in jshell and other environments
5d4c71c: 8281236: (D)TLS key exchange named groups
... and 33 more: https://git.openjdk.org/jdk/compare/336d230a39e41eeed8b9d2ce3fec42e9de1d11fe...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-12-08T09:47:35Z

@jatin-bhateja @smita-kamath Pushed as commit 073897c.

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

Auto vectorize half precision floating point conversion APIs

a102491

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Dec 2, 2022

smita-kamath marked this pull request as ready for review December 2, 2022 06:41

openjdk bot added the rfr Pull request is ready for review label Dec 2, 2022

jatin-bhateja reviewed Dec 2, 2022

View reviewed changes

sviswa7 reviewed Dec 2, 2022

View reviewed changes

smita-kamath added 3 commits December 5, 2022 17:24

Updated code as per review comments

8e7f884

Updated instruction definition

52cefb8

Merge master

a0b4f96

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Dec 6, 2022

sviswa7 reviewed Dec 6, 2022

View reviewed changes

Addressed review comment

09816f7

openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Dec 6, 2022

sviswa7 approved these changes Dec 6, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 6, 2022

Update test case

981ea9f

vnkozlov reviewed Dec 6, 2022

View reviewed changes

Updated test case

4b1e127

jatin-bhateja reviewed Dec 7, 2022

View reviewed changes

jatin-bhateja approved these changes Dec 7, 2022

View reviewed changes

Updated test case and updated code as per review comment

dc7d728

fg1417 approved these changes Dec 8, 2022

View reviewed changes

XiaohongGong approved these changes Dec 8, 2022

View reviewed changes

vnkozlov approved these changes Dec 8, 2022

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Dec 8, 2022

openjdk bot added the integrated Pull request has been integrated label Dec 8, 2022

openjdk bot closed this Dec 8, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Dec 8, 2022

		case Op_F2HFV:
		if (!VM_Version::supports_f16c() && !VM_Version::supports_avx512vl()) {

		void Assembler::vcvtph2ps(XMMRegister dst, XMMRegister src, int vector_len) {
		assert(VM_Version::supports_avx512vl() \|\| VM_Version::supports_f16c(), "");

Conversation

smita-kamath commented Dec 2, 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 Dec 2, 2022

Uh oh!

smita-kamath commented Dec 2, 2022

Uh oh!

openjdk bot commented Dec 2, 2022

Uh oh!

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

Uh oh!

Webrevs

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!

openjdk bot commented Dec 6, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sviswa7 commented Dec 6, 2022

Uh oh!

openjdk bot commented Dec 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vnkozlov 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!

jatin-bhateja Dec 7, 2022 • 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!

jatin-bhateja left a comment

Choose a reason for hiding this comment

Uh oh!

smita-kamath commented Dec 7, 2022

Uh oh!

vnkozlov commented Dec 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smita-kamath commented Dec 7, 2022

Uh oh!

smita-kamath commented Dec 7, 2022

Uh oh!

vnkozlov commented Dec 7, 2022

Uh oh!

sviswa7 commented Dec 8, 2022

Uh oh!

vnkozlov commented Dec 8, 2022

Uh oh!

smita-kamath commented Dec 2, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Dec 2, 2022 •

edited

Loading

openjdk bot commented Dec 6, 2022 •

edited

Loading

jatin-bhateja Dec 7, 2022 •

edited

Loading

vnkozlov commented Dec 7, 2022 •

edited

Loading

jatin-bhateja commented Dec 8, 2022 •

edited

Loading