JDK-8325037: x86: enable and fix hotspot/jtreg/compiler/vectorization/TestRoundVectFloat.java #17649

Hamlin-Li · 2024-01-31T10:09:16Z

Hi,
Can you help to review this simple patch to fix test TestRoundVectFloat.java?
Thanks!

FYI: This test is not actually tested, need to fix the test applying filter and IR matching rule.

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-8325037: x86: enable and fix hotspot/jtreg/compiler/vectorization/TestRoundVectFloat.java (Bug - P5)

Reviewers

Tobias Hartmann (@TobiHartmann - Reviewer)
Vladimir Kozlov (@vnkozlov - Reviewer)
Jatin Bhateja (@jatin-bhateja - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17649

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-01-31T10:09:46Z

👋 Welcome back mli! 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-01-31T10:11:22Z

@Hamlin-Li 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-01-31T10:14:10Z

Webrevs

00: Full (971fff04)

TobiHartmann · 2024-01-31T11:34:16Z

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

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2022, 2023, Oracle and/or its affiliates. All rights reserved.


Suggested change

* Copyright (c) 2022, 2023, Oracle and/or its affiliates. All rights reserved.

* Copyright (c) 2022, 2024, Oracle and/or its affiliates. All rights reserved.

TobiHartmann · 2024-01-31T11:37:03Z

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

                                 "-XX:CompileThresholdScaling=0.3");
      System.out.println("PASSED");
  }

  @Test
-  @IR(applyIf = {"UseAVX", " > 1"}, counts = {"RoundVF" , " > 0 "})
+  @IR(applyIf = {"UseAVX", " > 1"}, counts = {IRNode.ROUND_VF , " > 0 "})


Isn't that node only available if UseAVX >= 2?

jdk/src/hotspot/cpu/x86/x86.ad

Lines 1501 to 1504 in f0bae79

case Op_RoundVF:

if (UseAVX < 2) { // enabled for AVX2 only

return false;

}

Sorry, misread the code. Looks good.

FTR, backend implementation of rounding first adds a broadcasted 0.5 value to incoming floating point vector lanes, register only flavor of broadcast operation is only supported by AVX2 targets. We can broadcast 0.5 into constant table emitted prior to method body to lift this limitation. Another concern is around different max vector size for floating point and integral vectors on AVX1 targets, 256 bit float vector vs 128 bit integral vector. Thus, a sharper check in match_rule_supported_vector (size_in_bits > 128 && UseAVX < 2) should be added to support this for AVX1 targets as we are emitting an integral compare in special case handling for NaN and -/+0.0 / Inf.

But, this can be taken up separately, if need be, current fix for PR looks ok to me.

TobiHartmann · 2024-01-31T12:55:15Z

I'll run this through our testing and report back once it passed.

openjdk · 2024-01-31T12:55:53Z

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

8325037: x86: enable and fix hotspot/jtreg/compiler/vectorization/TestRoundVectFloat.java

Reviewed-by: thartmann, kvn, 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 41 new commits pushed to the master branch:

d9331bf: 8324845: management.properties text "interface name" is misleading
68206b5: 8324585: JVM native memory leak in PCKS11-NSS security provider
1aba78f: 8324937: GHA: Avoid multiple test suites per job
a663248: 8324668: JDWP process management needs more efficient file descriptor handling
a2229b1: 8324838: test_nmt_locationprinting.cpp broken in the gcc windows build
432756b: 8325024: java/security/cert/CertPathValidator/OCSP/OCSPTimeout.java incorrect comment information
fe78c0f: 8325022: Incorrect error message on client authentication
5b9b176: 8324174: assert(m->is_entered(current)) failed: invariant
0cc8e5b: 8325042: remove unused JVMDITools test files
f292053: 8323621: JDK build should exclude snippet class in java.lang.foreign
... and 31 more: https://git.openjdk.org/jdk/compare/4df04f0ec910525cdef1dea40a3c2d184213ea3a...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.

vnkozlov

Good.

Hamlin-Li · 2024-02-01T11:00:55Z

I'll run this through our testing and report back once it passed.

Thanks for testing, I'll wait for your feedback.

Hamlin-Li · 2024-02-04T09:45:08Z

@TobiHartmann @vnkozlov @jatin-bhateja Thanks for your revewiing!

/integrate

openjdk · 2024-02-04T09:46:17Z

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

a18b03b: 8324635: (zipfs) Regression in Files.setPosixFilePermissions called on existing MSDOS entries
7476e29: 8323680: SA PointerFinder code can do a better job of leveraging existing code to determine if an address is in the TLAB
63cb1f8: 8321396: Retire test/jdk/java/util/zip/NoExtensionSignature.java
f613e13: 8313739: ZipOutputStream.close() should always close the wrapped stream
adc3604: 8325148: Enable restricted javac warning in java.base
1ae8513: 8324858: [vectorapi] Bounds checking issues when accessing memory segments
38c0197: 8318105: [jmh] the test java.security.HSS failed with 2 active threads
6787c4c: 8325055: Rename Injector.h
91d8dac: 8325137: com/sun/management/ThreadMXBean/ThreadCpuTimeArray.java can fail in Xcomp with out of expected range
783ae56: 8311893: Interactive component with ARIA role 'tabpanel' does not have a programmatically associated name
... and 52 more: https://git.openjdk.org/jdk/compare/4df04f0ec910525cdef1dea40a3c2d184213ea3a...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-02-04T09:46:22Z

@Hamlin-Li Pushed as commit ed06846.

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

Hamlin-Li added 2 commits January 31, 2024 10:00

Initial commit

97827b4

fix copyright

971fff0

openjdk bot added the rfr Pull request is ready for review label Jan 31, 2024

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Jan 31, 2024

TobiHartmann suggested changes Jan 31, 2024

View reviewed changes

TobiHartmann approved these changes Jan 31, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 31, 2024

vnkozlov approved these changes Jan 31, 2024

View reviewed changes

jatin-bhateja approved these changes Feb 1, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Feb 4, 2024

openjdk bot closed this Feb 4, 2024

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

Hamlin-Li deleted the x86-round-v-f branch February 27, 2024 08:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JDK-8325037: x86: enable and fix hotspot/jtreg/compiler/vectorization/TestRoundVectFloat.java #17649

JDK-8325037: x86: enable and fix hotspot/jtreg/compiler/vectorization/TestRoundVectFloat.java #17649

Hamlin-Li commented Jan 31, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 31, 2024

openjdk bot commented Jan 31, 2024

mlbridge bot commented Jan 31, 2024

TobiHartmann Jan 31, 2024

TobiHartmann Jan 31, 2024 •

edited

Loading

TobiHartmann Jan 31, 2024

jatin-bhateja Feb 1, 2024 •

edited

Loading

TobiHartmann commented Jan 31, 2024

openjdk bot commented Jan 31, 2024 •

edited

Loading

vnkozlov left a comment

Hamlin-Li commented Feb 1, 2024

Hamlin-Li commented Feb 4, 2024

openjdk bot commented Feb 4, 2024

openjdk bot commented Feb 4, 2024

	* Copyright (c) 2022, 2023, Oracle and/or its affiliates. All rights reserved.
	* Copyright (c) 2022, 2024, Oracle and/or its affiliates. All rights reserved.

	case Op_RoundVF:
	if (UseAVX < 2) { // enabled for AVX2 only
	return false;
	}

JDK-8325037: x86: enable and fix hotspot/jtreg/compiler/vectorization/TestRoundVectFloat.java #17649

JDK-8325037: x86: enable and fix hotspot/jtreg/compiler/vectorization/TestRoundVectFloat.java #17649

Conversation

Hamlin-Li commented Jan 31, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Jan 31, 2024

openjdk bot commented Jan 31, 2024

mlbridge bot commented Jan 31, 2024

Webrevs

TobiHartmann Jan 31, 2024

Choose a reason for hiding this comment

TobiHartmann Jan 31, 2024 • edited Loading

Choose a reason for hiding this comment

TobiHartmann Jan 31, 2024

Choose a reason for hiding this comment

jatin-bhateja Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

TobiHartmann commented Jan 31, 2024

openjdk bot commented Jan 31, 2024 • edited Loading

vnkozlov left a comment

Choose a reason for hiding this comment

Hamlin-Li commented Feb 1, 2024

Hamlin-Li commented Feb 4, 2024

openjdk bot commented Feb 4, 2024

openjdk bot commented Feb 4, 2024

Hamlin-Li commented Jan 31, 2024 •

edited by openjdk bot

Loading

TobiHartmann Jan 31, 2024 •

edited

Loading

jatin-bhateja Feb 1, 2024 •

edited

Loading

openjdk bot commented Jan 31, 2024 •

edited

Loading