8338126 : C2 SuperWord: VectorCastF2HF / vcvtps2ph produces wrong results for vector length 2 #21480

sviswa7 · 2024-10-11T23:27:35Z

When Float.floatToFloat16 is vectorized using a 2-element vector width due to dependencies, we incorrectly generate a 4-element vcvtps2ph with memory as destination storing 8 bytes instead of desired 4 bytes. This issue is fixed in this PR by limiting the memory version of match rule to 4-element vector and above.
Also a regression test case is added accordingly.

Best Regards,
Sandhya

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-8338126: C2 SuperWord: VectorCastF2HF / vcvtps2ph produces wrong results for vector length 2 (Bug - P3)

Reviewers

Tobias Hartmann (@TobiHartmann - Reviewer)
Emanuel Peter (@eme64 - Reviewer) Review applies to ed299327
Jatin Bhateja (@jatin-bhateja - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21480

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

Using diff file

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

Webrev

Link to Webrev Comment

…ults for vector length 2

bridgekeeper · 2024-10-11T23:28:29Z

👋 Welcome back sviswanathan! 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 · 2024-10-11T23:29:10Z

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

8338126: C2 SuperWord: VectorCastF2HF / vcvtps2ph produces wrong results for vector length 2

Reviewed-by: thartmann, jbhateja, epeter

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

c51a086: 8339694: ciTypeFlow does not correctly handle unresolved constant dynamic of array type
7f4ed50: 8341020: Error handler crashes when Metaspace is not fully initialized
f50bd0d: 8341513: Remove the unused thread_type field from OSThread
7a16906: 8341134: Deprecate for removal the jrunscript tool
ffe6091: 8173970: jar tool should have a way to extract to a directory
2b03dbd: 8311530: Deprecate jdk.jsobject module for removal
f7e8f50: 8341292: Open some TextArea awt tests 3
fe83b7d: 8339341: SurfaceManager cacheMap retains strong references
12551ae: 8339975: Open some dialog awt tests 2
60af907: 8340494: Open some dialog awt tests 4
... and 123 more: https://git.openjdk.org/jdk/compare/593c27e69703875115e6db5843a3743ba9bd8c18...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.

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

openjdk · 2024-10-11T23:29:47Z

@sviswa7 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 · 2024-10-11T23:35:06Z

Webrevs

TobiHartmann

That looks good to me. @eme64 should have a look as well.

I submitted testing and will report back once it passed.

eme64 · 2024-10-14T12:18:30Z

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

+    public void test_float_float16_short_vector(short[] sout, float[] finp) {
+        for (int i = 0; i < finp.length - 1; i++) {
+            sout[i+0] = Float.floatToFloat16(finp[i+0]);
+            sout[i+1] = Float.floatToFloat16(finp[i+1]);
+        }
+    }


Your test looks different than the one that I added on JIRA. Can you please add that one as well?

Thanks for pointing that out. I have modified the contents of the loop kernel to match your testcase loop kernel now. I also verified that it fails before the fix and passes after the fix.

Before the fix the test fails:
Test results: failed: 1

And the jtr file shows the following:

Custom Run Test: @Run: kernel_test_float_float16 - @Tests: {test_float_float16,test_float_float16_strided,test_float_float16_short_vector}: compiler.lib.ir_framework.shared.TestRunException: There was an error while invoking @Run method public void compiler.vectorization.TestFloatConversionsVector.kernel_test_float_float16() at compiler.lib.ir_framework.test.CustomRunTest.invokeTest(CustomRunTest.java:162) at compiler.lib.ir_framework.test.CustomRunTest.run(CustomRunTest.java:87) at compiler.lib.ir_framework.test.TestVM.runTests(TestVM.java:861) at compiler.lib.ir_framework.test.TestVM.start(TestVM.java:252) at compiler.lib.ir_framework.test.TestVM.main(TestVM.java:165) Caused by: java.lang.reflect.InvocationTargetException at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:119) at java.base/java.lang.reflect.Method.invoke(Method.java:573) at compiler.lib.ir_framework.test.CustomRunTest.invokeTest(CustomRunTest.java:159) ... 4 more Caused by: java.lang.RuntimeException: assertEquals expected: 18483 but was: 0 at jdk.test.lib.Asserts.fail(Asserts.java:691) at jdk.test.lib.Asserts.assertEquals(Asserts.java:204) at jdk.test.lib.Asserts.assertEquals(Asserts.java:191) at compiler.vectorization.TestFloatConversionsVector.kernel_test_float_float16(TestFloatConversionsVector.java:112) at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104) ... 6 more

After the fix the test passes with no failures:
Test results: passed: 1

Please let me know if this works or you would like to see any other change.

eme64 · 2024-10-14T12:23:25Z

src/hotspot/cpu/x86/x86.ad

@@ -3676,6 +3676,7 @@ instruct vconvF2HF(vec dst, vec src) %{
 %}

 instruct vconvF2HF_mem_reg(memory mem, vec src) %{
+  predicate(Matcher::vector_length_in_bytes(n->in(3)->in(1)) >= 16);


Ok, and so what alternative path is the matcher now going to take if we have vector_length = 8?

@eme64 It is now going to do the load from memory in register and use the register/register version for the conversion and then use the store to memory. Please see before and after code snippets below.

Generated code snippet for 2 element float vector to float16 vector conversion
Before:
vmovq 0x10(%rdx,%rbx,4),%xmm2 ; load 8 bytes from memory into xmm2 (correct)
vcvtps2ph $0x4,%xmm2,0x10(%rsi,%rbx,2) ; convert to float16 and store 8 bytes to memory (incorrect)

After:
vmovq 0x10(%rdx,%rbx,4),%xmm15 ; load 8 bytes from memory into xmm15 (correct)
vcvtps2ph $0x4,%xmm15,%xmm0 ; convert to float16 into register (correct)
vmovd %xmm0,0x10(%rsi,%rbx,2) ; store 4 byte into memory (correct)

Ah, I see. You are using a 4-element register-only vcvtps2ph instruction, but only use the first 2-elements of it. Great :)

eme64

Thanks for the updates! It looks good to me now.

I have one more wish:
Could you allow to run the test on all platforms please?
test/hotspot/jtreg/compiler/vectorization/TestFloatConversionsVector.java

Currently, it only runs on selected platforms, see @requires. We should really run this on all platforms and compilers. The IR rules can be limited to platforms and features. That ensures that we do not have similar bugs elsewhere, and our test-coverage would be increased.

jatin-bhateja · 2024-10-15T08:08:59Z

src/hotspot/cpu/x86/x86.ad

@@ -3676,6 +3676,7 @@ instruct vconvF2HF(vec dst, vec src) %{
 %}

 instruct vconvF2HF_mem_reg(memory mem, vec src) %{
+  predicate(Matcher::vector_length_in_bytes(n->in(3)->in(1)) >= 16);


You can add an eligant prediction check like following instead of accesing bare inputs.

n->as_StoreVector()->memory_size() >= 16.

We have used bare inputs at many places in the ad file in the predicate.

I think its ok to use safe cast if its available atleast for newly added code.

@jatin-bhateja I have made this change. Please take a look.

jatin-bhateja · 2024-10-15T08:15:41Z

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

+            test_float_float16_short_vector(sout, finp);
+        }
+
+        // Verifying the result


Since we are using IR framework, we can leverage existing @Check annotation for verification which works in conjunction with @test method, it will automatically invoke validation after test method execution. We may need little refactoring for this.

The added test follows the verification mechanism used already in the test. I would prefer not to get into refactoring.

Okay, we can do it separately.

sviswa7 · 2024-10-16T01:33:27Z

Thanks for the updates! It looks good to me now.

I have one more wish: Could you allow to run the test on all platforms please? test/hotspot/jtreg/compiler/vectorization/TestFloatConversionsVector.java

Currently, it only runs on selected platforms, see @requires. We should really run this on all platforms and compilers. The IR rules can be limited to platforms and features. That ensures that we do not have similar bugs elsewhere, and our test-coverage would be increased.

@eme64 I have attempted to update the test accordingly. Please take a look.

sviswa7 · 2024-10-17T16:22:57Z

That looks good to me. @eme64 should have a look as well.

I submitted testing and will report back once it passed.

@TobiHartmann Please do let me know if the testing passed.

TobiHartmann · 2024-10-18T06:55:37Z

Sorry for the delay. I re-submitted testing with the latest version and it all passed.

sviswa7 · 2024-10-21T14:57:54Z

Thanks a lot @TobiHartmann.

sviswa7 · 2024-10-21T14:58:20Z

/integrate

openjdk · 2024-10-21T14:58:43Z

Going to push as commit 153ad91.
Since your change was applied there have been 156 commits pushed to the master branch:

80ec552: 8328528: C2 should optimize long-typed parallel iv in an int counted loop
330f2b5: 8342295: compiler/jvmci/TestJVMCISavedProperties.java fails due to garbage in output
1f35748: 8342102: ZGC: Optimize copy constructors in ZPhysicalMemory
66ddaaa: 8340241: RISC-V: Returns mispredicted
07f550b: 8340818: Add a new jtreg test root to test the generated documentation
27ef6c9: 8341470: BigDecimal.stripTrailingZeros() optimization
5d5d88a: 8339570: Add Tidy build support for JDK tests
239d84a: 8342578: GHA: RISC-V: Bootstrap using Debian snapshot is still failing
aa060f2: 8342334: CDS: Scratch mirrors should not point to dead klasses
680dc5d: 8342496: C2/Shenandoah: SEGV in compiled code when running jcstress
... and 146 more: https://git.openjdk.org/jdk/compare/593c27e69703875115e6db5843a3743ba9bd8c18...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-10-21T14:58:55Z

@sviswa7 Pushed as commit 153ad91.

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

8338126 : C2 SuperWord: VectorCastF2HF / vcvtps2ph produces wrong res…

dedb4a0

…ults for vector length 2

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Oct 11, 2024

sviswa7 marked this pull request as ready for review October 11, 2024 23:30

openjdk bot added the rfr Pull request is ready for review label Oct 11, 2024

TobiHartmann approved these changes Oct 14, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 14, 2024

eme64 reviewed Oct 14, 2024

View reviewed changes

Update test case

ed29932

openjdk bot removed the ready Pull request is ready to be integrated label Oct 14, 2024

eme64 approved these changes Oct 15, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 15, 2024

jatin-bhateja reviewed Oct 15, 2024

View reviewed changes

Run test on all platforms

f298137

openjdk bot removed the ready Pull request is ready to be integrated label Oct 16, 2024

Review comments resolution

c876dc9

jatin-bhateja approved these changes Oct 16, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 16, 2024

TobiHartmann approved these changes Oct 18, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Oct 21, 2024

openjdk bot closed this Oct 21, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8338126 : C2 SuperWord: VectorCastF2HF / vcvtps2ph produces wrong results for vector length 2 #21480

8338126 : C2 SuperWord: VectorCastF2HF / vcvtps2ph produces wrong results for vector length 2 #21480

sviswa7 commented Oct 11, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Oct 11, 2024

openjdk bot commented Oct 11, 2024 •

edited

Loading

openjdk bot commented Oct 11, 2024

mlbridge bot commented Oct 11, 2024 •

edited

Loading

TobiHartmann left a comment

eme64 Oct 14, 2024

sviswa7 Oct 14, 2024

eme64 Oct 14, 2024

sviswa7 Oct 14, 2024

eme64 Oct 15, 2024

eme64 left a comment

jatin-bhateja Oct 15, 2024

sviswa7 Oct 16, 2024

jatin-bhateja Oct 16, 2024 •

edited

Loading

sviswa7 Oct 16, 2024

jatin-bhateja Oct 15, 2024 •

edited

Loading

sviswa7 Oct 16, 2024

jatin-bhateja Oct 16, 2024 •

edited

Loading

sviswa7 commented Oct 16, 2024

sviswa7 commented Oct 17, 2024

TobiHartmann commented Oct 18, 2024

sviswa7 commented Oct 21, 2024

sviswa7 commented Oct 21, 2024

openjdk bot commented Oct 21, 2024

openjdk bot commented Oct 21, 2024

8338126 : C2 SuperWord: VectorCastF2HF / vcvtps2ph produces wrong results for vector length 2 #21480

8338126 : C2 SuperWord: VectorCastF2HF / vcvtps2ph produces wrong results for vector length 2 #21480

Conversation

sviswa7 commented Oct 11, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Oct 11, 2024

openjdk bot commented Oct 11, 2024 • edited Loading

openjdk bot commented Oct 11, 2024

mlbridge bot commented Oct 11, 2024 • edited Loading

Webrevs

TobiHartmann 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

eme64 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jatin-bhateja Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jatin-bhateja Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jatin-bhateja Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

sviswa7 commented Oct 16, 2024

sviswa7 commented Oct 17, 2024

TobiHartmann commented Oct 18, 2024

sviswa7 commented Oct 21, 2024

sviswa7 commented Oct 21, 2024

openjdk bot commented Oct 21, 2024

openjdk bot commented Oct 21, 2024

sviswa7 commented Oct 11, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Oct 11, 2024 •

edited

Loading

mlbridge bot commented Oct 11, 2024 •

edited

Loading

jatin-bhateja Oct 16, 2024 •

edited

Loading

jatin-bhateja Oct 15, 2024 •

edited

Loading

jatin-bhateja Oct 16, 2024 •

edited

Loading