8315024: Vector API FP reduction tests should not test for exact equality by gergo- · Pull Request #16024 · openjdk/jdk

gergo- · 2023-10-03T07:53:05Z

Certain floating point reduction operations in the Vector API are allowed to introduce rounding errors. Adjust the corresponding tests to allow a small relative error when comparing the operation's result to the expected value. Also, add a new generator double[i / 10.0 + 0.1] to test floating point operations with somewhat more interesting input data.

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-8315024: Vector API FP reduction tests should not test for exact equality (Bug - P5)

Reviewers

Emanuel Peter (@eme64 - Reviewer)
Tobias Hartmann (@TobiHartmann - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16024

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

Using diff file

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

Webrev

Link to Webrev Comment

…lity

bridgekeeper · 2023-10-03T07:53:54Z

👋 Welcome back gbarany! 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 · 2023-10-03T07:56:10Z

@gergo- To determine the appropriate audience for reviewing this pull request, one or more labels corresponding to different subsystems will normally be applied automatically. However, no automatic labelling rule matches the changes in this pull request. In order to have an "RFR" email sent to the correct mailing list, you will need to add one or more applicable labels manually using the /label pull request command.

Applicable Labels

build
client
compiler
core-libs
graal
hotspot
hotspot-compiler
hotspot-gc
hotspot-jfr
hotspot-runtime
i18n
ide-support
javadoc
jdk
jmx
kulla
net
nio
security
serviceability
shenandoah

gergo- · 2023-10-03T08:26:07Z

/label add hotspot-compiler

openjdk · 2023-10-03T08:27:41Z

@gergo-
The hotspot-compiler label was successfully added.

mlbridge · 2023-10-03T08:31:52Z

Webrevs

01: Full - Incremental (710cbae7)
00: Full (0c4b903e)

eme64

Generally looks good, thanks for looking into this.
I left a few comments below.

Another concern I have, which I ran into by writing tests for the auto-vectorizer:
Are we making sure the float/double reductions do not degenerate to either zero or infinity? Because if they do degenerate, then we have only a very weak test.
I'm especially worried about all the values that depend on i, and then get multiplied. Don't the multiplications hit the maximal float value very quickly?

eme64 · 2023-10-04T09:25:49Z

test/jdk/jdk/incubator/vector/Double128VectorTests.java

It seems a bit large. Have you tried to make it smaller? Or what is your justification for this value?

With the current version of the test, a value as small as 0.000001 seems to be fine, one more zero is too small. This will probably have to be adjusted in the future for larger tests.

I'm just worried that 1% is very much for the addition tests. Basically we might be dropping off a whole element, and would not notice it.

eme64 · 2023-10-04T09:32:57Z

test/jdk/jdk/incubator/vector/Double128VectorTests.java

+            Assert.assertEquals(r[i], f.apply(a, i), Math.abs(r[i] * relativeError), "at index #" + i);
+        }
+    }
+


Optional: reduce code duplication. Call assertReductionArraysEquals with relativeError = 0 in pre-existing method.

Good idea, thanks.

eme64 · 2023-10-04T09:35:44Z

test/jdk/jdk/incubator/vector/Double128VectorTests.java

+            withToString("double[i / 10.0 + 0.1]", (int s) -> {
+                return fill(s * BUFFER_REPS,
+                            i -> (double)(i / (double) 10.0 + 0.1));
+            }),


Did this generate rounding issues for addition?
Another option would be random values that make sure to fill the whole mantissa with random information.
Of course the tricky part is to keep them within reasonable bounds so that on multiplication they do not degenerate to zero or infinity.
Also: it would be nice to have some cases with extreme values (infinity, NaN, etc).

Yes, this generator generates a rounding issue for addition:

test FloatMaxVectorTests.ADDReduceFloatMaxVectorTests(float[i / 10.0 + 0.1]): failure java.lang.AssertionError: at index #16 expected [39.2] but found [39.199997]

I updated this generator to 0.01 + (i / (i + 1)), plus a variant that replaces some of the elements of this sequence with values from the cornerCaseValues generator.
This should address all of your concerns. Almost all values in this sequence are very close to 1, so they can be added and multiplied without overflow, up to about 2000 elements for floats. The mantissas have bits all over the place. The exponents are very limited, but a wider range of exponents is exercised by the other tests.

gergo- · 2023-10-05T14:00:44Z

Are we making sure the float/double reductions do not degenerate to either zero or infinity?

You're right. We're currently getting "lucky" here because we reduce individual vectors of 4, 8, or 16 elements inside a loop. So even if the array contains a zero, or the product of the entire array is infinite, individual blocks inside it will have finite, non-zero products. This will change when https://bugs.openjdk.org/browse/JDK-8309647 is addressed. I'll try to put together a generator that also works nicely for multiplications over a whole array.

gergo- · 2023-10-12T09:28:55Z

@eme64 would you have time to take another look at the changes I have made to this PR?

eme64 · 2023-10-13T10:25:55Z

@gergo- I just looked at it again. It looks better.

Still, I have a concern about cornerCaseValues: Does the result not always degenerate to zero / NaN now?
Every 17th value is a corner-case. And there, we mix in all of the special cases deterministically (zero, infty, min/max, NaN), depending on INT/FP type. But it seems that way we always have a NaN in the FP array - and therefore the reduction would always be NaN. Similar things happen with int-multiplication and zero.

I wonder if it would not be better to generate some data randomly, and throw in the special-cases with a very low probability. Maybe 50% that any show up, and then randomly pick one or more special case values. That way you can test the different special-cases separately. And their position could also be random.

BTW: is there any wiki about the template file format, and how to "compile" it to java? I might want to use it in the future myself :)

gergo- · 2023-10-13T13:03:20Z

@gergo- I just looked at it again. It looks better.

Thanks.

Still, I have a concern about cornerCaseValues: Does the result not always degenerate to zero / NaN now? Every 17th value is a corner-case. And there, we mix in all of the special cases deterministically (zero, infty, min/max, NaN), depending on INT/FP type. But it seems that way we always have a NaN in the FP array - and therefore the reduction would always be NaN. Similar things happen with int-multiplication and zero.

Currently there are reduction tests that reduce not across the whole input array but over individual vector-sized blocks, e.g.:

        for (int ic = 0; ic < INVOC_COUNT; ic++) {
            for (int i = 0; i < a.length; i += SPECIES.length()) {
                DoubleVector av = DoubleVector.fromArray(SPECIES, a, i);
                r[i] = av.reduceLanes(VectorOperators.MUL);
            }
        }

The largest vector length is 16 elements (32 bit float * 16 = 512 bits max vector size). Therefore at most every second block will contain one corner-case value, and all the other blocks will only contain normal values. No block mixes different corner case values.

I wonder if it would not be better to generate some data randomly, and throw in the special-cases with a very low probability. Maybe 50% that any show up, and then randomly pick one or more special case values. That way you can test the different special-cases separately. And their position could also be random.

I would say that this could be tackled more naturally as part of https://bugs.openjdk.org/browse/JDK-8309647 which concerns moving reductions out of loops and would require revisiting these tests anyway.

BTW: is there any wiki about the template file format, and how to "compile" it to java? I might want to use it in the future myself :)

I'm not aware of any docs, I learned to do this by doing. I just do bash gen-tests.sh to generate the Java code.

eme64

@gergo- Sounds ok. We don't have to do all in your change here. Thanks for the work you did!

openjdk · 2023-10-16T13:38:05Z

@gergo- 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: epeter, thartmann

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

fd332da: 8317289: javadoc fails with -sourcepath if module-info.java contains import statements
6d3cb45: 8318591: avoid leaks in loadlib_aix.cpp reload_table()
cb383c0: 8318587: refresh libraries cache on AIX in print_vm_info
4bfe226: 8310031: Parallel: Implement better work distribution for large object arrays in old gen
08f7914: 8305753: Allow JIT compilation for -Xshare:dump
728b858: 8318130: SocksSocketImpl needlessly encodes hostname for IPv6 addresses
eb59167: 8318691: runtime/CompressedOops/CompressedClassPointersEncodingScheme.java fails with release VMs
1b15011: 8318476: Add resource consumption note to BigInteger and BigDecimal
5ba9705: 8318485: Narrow klass shift should be zero if encoding range extends to 0x1_0000_0000
8d9a4b4: 8317678: Fix up hashCode() for ZipFile.Source.Key
... and 299 more: https://git.openjdk.org/jdk/compare/bd918f49d29bcbc699e07b4ef8d23cfe1abd32df...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 (@eme64, @TobiHartmann) 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).

gergo- · 2023-10-17T07:04:10Z

Thanks for the review @eme64 !

/integrate

openjdk · 2023-10-17T07:05:38Z

@gergo-
Your change (at version 710cbae) is now ready to be sponsored by a Committer.

eme64 · 2023-10-17T07:07:14Z

@gergo- You should only integrate once you have 2 reviewers (unless your changes are trivial, and this is not exactly trivial).
Nothing bad happened, since you need a sponsor anyway.

TobiHartmann

Looks reasonable to me.

TobiHartmann · 2023-10-24T08:57:27Z

/sponsor

openjdk · 2023-10-24T08:58:22Z

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

fd332da: 8317289: javadoc fails with -sourcepath if module-info.java contains import statements
6d3cb45: 8318591: avoid leaks in loadlib_aix.cpp reload_table()
cb383c0: 8318587: refresh libraries cache on AIX in print_vm_info
4bfe226: 8310031: Parallel: Implement better work distribution for large object arrays in old gen
08f7914: 8305753: Allow JIT compilation for -Xshare:dump
728b858: 8318130: SocksSocketImpl needlessly encodes hostname for IPv6 addresses
eb59167: 8318691: runtime/CompressedOops/CompressedClassPointersEncodingScheme.java fails with release VMs
1b15011: 8318476: Add resource consumption note to BigInteger and BigDecimal
5ba9705: 8318485: Narrow klass shift should be zero if encoding range extends to 0x1_0000_0000
8d9a4b4: 8317678: Fix up hashCode() for ZipFile.Source.Key
... and 299 more: https://git.openjdk.org/jdk/compare/bd918f49d29bcbc699e07b4ef8d23cfe1abd32df...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-10-24T08:58:34Z

@TobiHartmann @gergo- Pushed as commit e6f23a9.

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

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

0c4b903

…lity

openjdk bot added the rfr Pull request is ready for review label Oct 3, 2023

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Oct 3, 2023

eme64 suggested changes Oct 4, 2023

View reviewed changes

Address review comments

710cbae

eme64 approved these changes Oct 16, 2023

View reviewed changes

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

openjdk bot added the sponsor Pull request is ready to be sponsored label Oct 17, 2023

TobiHartmann approved these changes Oct 24, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Oct 24, 2023

openjdk bot closed this Oct 24, 2023

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 Oct 24, 2023

Conversation

gergo- commented Oct 3, 2023 • 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 Oct 3, 2023

Uh oh!

openjdk bot commented Oct 3, 2023

Uh oh!

gergo- commented Oct 3, 2023

Uh oh!

openjdk bot commented Oct 3, 2023

Uh oh!

mlbridge bot commented Oct 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

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

gergo- commented Oct 5, 2023

Uh oh!

gergo- commented Oct 12, 2023

Uh oh!

eme64 commented Oct 13, 2023

Uh oh!

gergo- commented Oct 13, 2023

Uh oh!

eme64 left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Oct 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gergo- commented Oct 17, 2023

Uh oh!

openjdk bot commented Oct 17, 2023

Uh oh!

eme64 commented Oct 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TobiHartmann left a comment

Choose a reason for hiding this comment

Uh oh!

TobiHartmann commented Oct 24, 2023

Uh oh!

openjdk bot commented Oct 24, 2023

Uh oh!

openjdk bot commented Oct 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

Comments

gergo- commented Oct 3, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Oct 3, 2023 •

edited

Loading

openjdk bot commented Oct 16, 2023 •

edited

Loading

eme64 commented Oct 17, 2023 •

edited

Loading