8315024: Vector API FP reduction tests should not test for exact equality by amosshi · Pull Request #2768 · openjdk/jdk17u-dev

amosshi · 2024-08-01T17:10:40Z

This PR has two commits
- commit 1: the git apply result, which is clean
- commit 2: manually merge the following two .rej files
  - test/jdk/jdk/incubator/vector/DoubleMaxVectorTests.java.rej
  - test/jdk/jdk/incubator/vector/FloatMaxVectorTests.java.rej
- commit 3,4: Fix code postion
- commit 5: Add file Unit-header.template

Content of test/jdk/jdk/incubator/vector/DoubleMaxVectorTests.java.rej

@@ -668,15 +670,21 @@
 
     static void assertReductionArraysEquals(double[] r, double rc, double[] a,
                                             FReductionOp f, FReductionAllOp fa) {
+        assertReductionArraysEquals(r, rc, a, f, fa, (double)0.0);
+    }
+
+    static void assertReductionArraysEquals(double[] r, double rc, double[] a,
+                                            FReductionOp f, FReductionAllOp fa,
+                                            double relativeError) {
         int i = 0;
         try {
-            Assert.assertEquals(rc, fa.apply(a));
+            Assert.assertEquals(rc, fa.apply(a), Math.abs(rc * relativeError));
             for (; i < a.length; i += SPECIES.length()) {
-                Assert.assertEquals(r[i], f.apply(a, i));
+                Assert.assertEquals(r[i], f.apply(a, i), Math.abs(r[i] * relativeError));
             }
         } catch (AssertionError e) {
-            Assert.assertEquals(rc, fa.apply(a), "Final result is incorrect!");
-            Assert.assertEquals(r[i], f.apply(a, i), "at index #" + i);
+            Assert.assertEquals(rc, fa.apply(a), Math.abs(rc * relativeError), "Final result is incorrect!");
+            Assert.assertEquals(r[i], f.apply(a, i), Math.abs(r[i] * relativeError), "at index #" + i);
         }
     }
 
@@ -690,15 +698,22 @@
 
     static void assertReductionArraysEqualsMasked(double[] r, double rc, double[] a, boolean[] mask,
                                             FReductionMaskedOp f, FReductionAllMaskedOp fa) {
+        assertReductionArraysEqualsMasked(r, rc, a, mask, f, fa, (double)0.0);
+    }
+
+    static void assertReductionArraysEqualsMasked(double[] r, double rc, double[] a, boolean[] mask,
+                                            FReductionMaskedOp f, FReductionAllMaskedOp fa,
+                                            double relativeError) {
         int i = 0;
         try {
-            Assert.assertEquals(rc, fa.apply(a, mask));
+            Assert.assertEquals(rc, fa.apply(a, mask), Math.abs(rc * relativeError));
             for (; i < a.length; i += SPECIES.length()) {
-                Assert.assertEquals(r[i], f.apply(a, i, mask));
+                Assert.assertEquals(r[i], f.apply(a, i, mask), Math.abs(r[i] *
+relativeError));
             }
         } catch (AssertionError e) {
-            Assert.assertEquals(rc, fa.apply(a, mask), "Final result is incorrect!");
-            Assert.assertEquals(r[i], f.apply(a, i, mask), "at index #" + i);
+            Assert.assertEquals(rc, fa.apply(a, mask), Math.abs(rc * relativeError), "Final result is incorrect!");
+            Assert.assertEquals(r[i], f.apply(a, i, mask), Math.abs(r[i] * relativeError), "at index #" + i);
         }
     }

Content of test/jdk/jdk/incubator/vector/FloatMaxVectorTests.java.rej

@@ -668,15 +670,21 @@
 
     static void assertReductionArraysEquals(float[] r, float rc, float[] a,
                                             FReductionOp f, FReductionAllOp fa) {
+        assertReductionArraysEquals(r, rc, a, f, fa, (float)0.0);
+    }
+
+    static void assertReductionArraysEquals(float[] r, float rc, float[] a,
+                                            FReductionOp f, FReductionAllOp fa,
+                                            float relativeError) {
         int i = 0;
         try {
-            Assert.assertEquals(rc, fa.apply(a));
+            Assert.assertEquals(rc, fa.apply(a), Math.abs(rc * relativeError));
             for (; i < a.length; i += SPECIES.length()) {
-                Assert.assertEquals(r[i], f.apply(a, i));
+                Assert.assertEquals(r[i], f.apply(a, i), Math.abs(r[i] * relativeError));
             }
         } catch (AssertionError e) {
-            Assert.assertEquals(rc, fa.apply(a), "Final result is incorrect!");
-            Assert.assertEquals(r[i], f.apply(a, i), "at index #" + i);
+            Assert.assertEquals(rc, fa.apply(a), Math.abs(rc * relativeError), "Final result is incorrect!");
+            Assert.assertEquals(r[i], f.apply(a, i), Math.abs(r[i] * relativeError), "at index #" + i);
         }
     }
 
@@ -690,15 +698,22 @@
 
     static void assertReductionArraysEqualsMasked(float[] r, float rc, float[] a, boolean[] mask,
                                             FReductionMaskedOp f, FReductionAllMaskedOp fa) {
+        assertReductionArraysEqualsMasked(r, rc, a, mask, f, fa, (float)0.0);
+    }
+
+    static void assertReductionArraysEqualsMasked(float[] r, float rc, float[] a, boolean[] mask,
+                                            FReductionMaskedOp f, FReductionAllMaskedOp fa,
+                                            float relativeError) {
         int i = 0;
         try {
-            Assert.assertEquals(rc, fa.apply(a, mask));
+            Assert.assertEquals(rc, fa.apply(a, mask), Math.abs(rc * relativeError));
             for (; i < a.length; i += SPECIES.length()) {
-                Assert.assertEquals(r[i], f.apply(a, i, mask));
+                Assert.assertEquals(r[i], f.apply(a, i, mask), Math.abs(r[i] *
+relativeError));
             }
         } catch (AssertionError e) {
-            Assert.assertEquals(rc, fa.apply(a, mask), "Final result is incorrect!");
-            Assert.assertEquals(r[i], f.apply(a, i, mask), "at index #" + i);
+            Assert.assertEquals(rc, fa.apply(a, mask), Math.abs(rc * relativeError), "Final result is incorrect!");
+            Assert.assertEquals(r[i], f.apply(a, i, mask), Math.abs(r[i] * relativeError), "at index #" + i);
         }
     }

Testing

Local: Test passed on MacOS 14.5 with Apple M1 Max
- Double128VectorTests.java: Test results: passed: 1
- Double256VectorTests.java: Test results: passed: 1
- Double512VectorTests.java: Test results: passed: 1
- Double64VectorTests.java: Test results: passed: 1
- DoubleMaxVectorTests.java: Test results: passed: 1
- Float128VectorTests.java: Test results: passed: 1
- Float256VectorTests.java: Test results: passed: 1
- Float512VectorTests.java: Test results: passed: 1
- Float64VectorTests.java: Test results: passed: 1
- FloatMaxVectorTests.java: Test results: passed: 1
Pipeline:
- Linux, Windows - Passed
- MacOS - Failed with message: This request was automatically failed because there were no enabled runners online to process the request for more than 1 days.
Testing Machine: SAP nightlies SUCCESSFUL on 2024-08-17
- Automated jtreg test: jtreg_jdk_tier3, Started at 2024-08-16 22:45:27+01:00
- jdk/incubator/vector/Double128VectorTests.java: SUCCESSFUL GitHub 📊 - [22:55:08.718 -> 61,687 msec]
- jdk/incubator/vector/Double256VectorTests.java: SUCCESSFUL GitHub 📊 - [22:55:09.720 -> 52,033 msec]
- jdk/incubator/vector/Double512VectorTests.java: SUCCESSFUL GitHub 📊 - [22:55:25.860 -> 47,865 msec]
- jdk/incubator/vector/Double64VectorTests.java: SUCCESSFUL GitHub 📊 - [22:55:31.432 -> 79,579 msec]
- jdk/incubator/vector/DoubleMaxVectorTests.java: SUCCESSFUL GitHub 📊 - [22:55:40.700 -> 44,241 msec]
- jdk/incubator/vector/Float128VectorTests.java: SUCCESSFUL GitHub 📊 - [22:55:55.119 -> 51,644 msec]
- jdk/incubator/vector/Float256VectorTests.java: SUCCESSFUL GitHub 📊 - [22:56:00.304 -> 46,476 msec]
- jdk/incubator/vector/Float512VectorTests.java: SUCCESSFUL GitHub 📊 - [22:56:01.753 -> 49,030 msec]
- jdk/incubator/vector/Float64VectorTests.java: SUCCESSFUL GitHub 📊 - [22:56:03.088 -> 75,969 msec]
- jdk/incubator/vector/FloatMaxVectorTests.java: SUCCESSFUL GitHub 📊 - [22:56:10.405 -> 45,711 msec]

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
JDK-8315024 needs maintainer approval

Issue

JDK-8315024: Vector API FP reduction tests should not test for exact equality (Bug - P5 - Approved)

Reviewers

Martin Doerr (@TheRealMDoerr - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk17u-dev.git pull/2768/head:pull/2768
$ git checkout pull/2768

Update a local copy of the PR:
$ git checkout pull/2768
$ git pull https://git.openjdk.org/jdk17u-dev.git pull/2768/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2768

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk17u-dev/pull/2768.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-08-01T17:12:07Z

👋 Welcome back ashi! 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-08-01T17:12:38Z

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

8315024: Vector API FP reduction tests should not test for exact equality

Reviewed-by: mdoerr

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

be4f913: 8312049: runtime/logging/ClassLoadUnloadTest can be improved
8dbd677: 8315576: compiler/codecache/CodeCacheFullCountTest.java fails after JDK-8314837
c103409: 8308891: TestCDSVMCrash.java needs @requires vm.cds
d12077d: 8284316: Support accessibility ManualTestFrame.java for non SwingSet tests
6cbedd9: 8273216: JCMD does not work across container boundaries with Podman
d5d26be: 8315936: Parallelize gc/stress/TestStressG1Humongous.java test
2e3ca31: 8303891: Speed up Zip64SizeTest using a small ZIP64 file
27c92b6: 8299813: java/nio/channels/DatagramChannel/Disconnect.java fails with jtreg test timeout due to lost datagram
61e0a11: 8315684: Parallelize sun/security/util/math/TestIntegerModuloP.java
698f0e3: 8313674: (fc) java/nio/channels/FileChannel/BlockDeviceSize.java should test for more block devices
... and 235 more: https://git.openjdk.org/jdk17u-dev/compare/d953de6b2cb0572ea24b9e02952aa5aedee6cc64...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-08-01T17:12:55Z

This backport pull request has now been updated with issue from the original commit.

mlbridge · 2024-08-01T17:16:13Z

Webrevs

MBaesken · 2024-08-09T08:27:05Z

In the original commit I see in test/jdk/jdk/incubator/vector/DoubleMaxVectorTests.java

openjdk/jdk@e6f23a9
private static final double RELATIVE_ROUNDING_ERROR = (double)0.000001;

Is it intentional that it is not in the backport ?

amosshi · 2024-08-16T05:03:18Z

Hi @MBaesken,

Somehow the git patch command has put these two lines to the test/jdk/jdk/incubator/vector/FloatMaxVectorTests.java file at line 625-626

+ 625    // for floating point reduction ops that may introduce rounding errors
+ 626    private static final double RELATIVE_ROUNDING_ERROR = (double)0.000001;

I will move them to earlier lines in respect to the original commit.

Regards
Amos
A Happy Developer

amosshi · 2024-08-16T06:32:01Z

Update

Sending to test server to test again

TheRealMDoerr

Looks basically good. I only found a couple of formatting differences which should get removed.

test/jdk/jdk/incubator/vector/DoubleMaxVectorTests.java

test/jdk/jdk/incubator/vector/FloatMaxVectorTests.java

test/jdk/jdk/incubator/vector/templates/Unit-header.template

TheRealMDoerr

Thanks for the update!

openjdk · 2024-08-18T18:54:01Z

⚠️ @amosshi This change is now ready for you to apply for maintainer approval. This can be done directly in each associated issue or by using the /approval command.

amosshi · 2024-08-22T00:07:41Z

/approval request "Backporting for parity with 17.0.13-oracle. No risk, test fix. Un-Clean backport. Test passes and SAP nightlies pass on 2024-08-17”

openjdk · 2024-08-22T00:07:57Z

@amosshi
8315024: The approval request has been created successfully.

amosshi · 2024-08-26T18:48:02Z

/integrate

openjdk · 2024-08-26T18:50:09Z

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

803c2c2: 8302800: Augment NaN handling tests of FDLIBM methods
cd0bb4d: 8310683: Refactor StandardCharset/standard.java to use JUnit
1887a80: 8332248: (fc) java/nio/channels/FileChannel/BlockDeviceSize.java failed with RuntimeException
64ab2fa: 8303920: Avoid calling out to python in DataDescriptorSignatureMissing test
97e4a93: 8316193: jdk/jfr/event/oldobject/TestListenerLeak.java java.lang.Exception: Could not find leak
be4f913: 8312049: runtime/logging/ClassLoadUnloadTest can be improved
8dbd677: 8315576: compiler/codecache/CodeCacheFullCountTest.java fails after JDK-8314837
c103409: 8308891: TestCDSVMCrash.java needs @requires vm.cds
d12077d: 8284316: Support accessibility ManualTestFrame.java for non SwingSet tests
6cbedd9: 8273216: JCMD does not work across container boundaries with Podman
... and 240 more: https://git.openjdk.org/jdk17u-dev/compare/d953de6b2cb0572ea24b9e02952aa5aedee6cc64...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-08-26T18:50:15Z

@amosshi Pushed as commit 03cbfaa.

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

amosshi added 2 commits August 1, 2024 10:03

Backport e6f23a90d4a53339a3c9c2b76fc5d317940e4472

207cf1d

Backport e6f23a90d4a53339a3c9c2b76fc5d317940e4472 Manual Merge

678979a

openjdk bot changed the title ~~Backport e6f23a90d4a53339a3c9c2b76fc5d317940e4472~~ 8315024: Vector API FP reduction tests should not test for exact equality Aug 1, 2024

openjdk bot added backport Port of a pull request already in a different code base rfr Pull request is ready for review labels Aug 1, 2024

amosshi added 3 commits August 15, 2024 22:04

Fix based on review: move 2 lines to correct location

23f5de9

Fix based on review: move 2 lines to correct location

7ffeb08

Add file Unit-header.template

1b6851e

TheRealMDoerr reviewed Aug 16, 2024

View reviewed changes

amosshi added 2 commits August 16, 2024 23:55

Fix based on review: space and new line

a7ff773

Fix based on review: space and new line

2b9e3b3

amosshi requested a review from TheRealMDoerr August 17, 2024 07:07

TheRealMDoerr approved these changes Aug 18, 2024

View reviewed changes

openjdk bot added approval Requires approval; will be removed when approval is received ready Pull request is ready to be integrated and removed approval Requires approval; will be removed when approval is received labels Aug 22, 2024

openjdk bot added the integrated Pull request has been integrated label Aug 26, 2024

openjdk bot closed this Aug 26, 2024

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

Conversation

amosshi commented Aug 1, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Aug 1, 2024

Uh oh!

openjdk bot commented Aug 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Aug 1, 2024

Uh oh!

mlbridge bot commented Aug 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

MBaesken commented Aug 9, 2024

Uh oh!

amosshi commented Aug 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amosshi commented Aug 16, 2024

Update

Uh oh!

TheRealMDoerr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheRealMDoerr left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Aug 18, 2024

Uh oh!

amosshi commented Aug 22, 2024

Uh oh!

openjdk bot commented Aug 22, 2024

Uh oh!

amosshi commented Aug 26, 2024

Uh oh!

openjdk bot commented Aug 26, 2024

Uh oh!

openjdk bot commented Aug 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

Comments

amosshi commented Aug 1, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Aug 1, 2024 •

edited

Loading

mlbridge bot commented Aug 1, 2024 •

edited

Loading

amosshi commented Aug 16, 2024 •

edited

Loading