Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix vectorized toLowerCase and toUppercase intrinsics on Z #4688

Merged

Conversation

fjeremic
Copy link
Contributor

@fjeremic fjeremic commented Feb 11, 2019

In #4597 we identified cases on which toUpperCase fails on Linux on
Z. This is due to some faulty logic in the evaluator for these
intrinsics on Z. Bugs seem to have been introduced in #2318 for certain
characters. Namely the test in #4597 fails to convert the \u00FF
character. The bug is obvious, since toUpperCase of such a character
cannot be handled and the JIT evaluator determines the invalid
character range by checking if the character is greater than 0xFF, but
it should be checking if greater than or equal to.

In addition the compressed String evaluator also fails on 0xB5 as
determined by the unit tests. Both issues are fixed.

Fixes: #4597

@fjeremic
Copy link
Contributor Author

I've improved the tests by including an exhaustive list of tests for uppercasing and lowercasing. There were two issues:

  1. The previous testing never tested all characters so some conversions were wrong and uncaught
  2. The whole point of Test_JITHelpers is to compile the intrinsics and we should be testing with -Xjit:count=0 as a variation

Both issues will be fixed. There will be a subsequent commit fixing the issues, but before I add it I'd like to kick off sanity testing with the new tests and ensure the failures are caught on the farm. In addition to the problem observed in #4597 we have one additional character we're not converting properly (0xB5). Hopefully both failures show up on the builds.

Jenkins test sanity xlinux,zlinux JDK8,JDK11

@fjeremic fjeremic changed the title Fix vectorized toLowerCase and toUppercase intrinsics in Z Fix vectorized toLowerCase and toUppercase intrinsics on Z Feb 11, 2019
It seems the previous tests that were added were only testing certain
buffer lengths against certain subsets of characters. While ok, these
tests are not exhaustive and do not test the entire range of characters
which can be uppercased or lowercased.

In this commit we add exhaustive testing for both intrinsic APIs by
pre-generating a table of the uppercase and lowercase mapping. We use
this map to compare the result of calling the intrinsic.

Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>
@fjeremic
Copy link
Contributor Author

As expected the tests failed as follows [1]:

FAILED: test_toUpperIntrinsicLatin1
java.lang.AssertionError: Failed to correctly uppercase character 0xb5, actual = 0xb5 expected = 0x39c expected [true] but found [false]
	at org.testng.Assert.fail(Assert.java:96)
	at org.testng.Assert.failNotEquals(Assert.java:776)
	at org.testng.Assert.assertTrue(Assert.java:44)
	at org.openj9.test.com.ibm.jit.Test_JITHelpers.test_toUpperIntrinsicLatin1(Test_JITHelpers.java:297)
...
FAILED: test_toUpperIntrinsicUTF16
java.lang.AssertionError: Failed to correctly uppercase character 0xff, actual = 0xff expected = 0x178 expected [true] but found [false]
	at org.testng.Assert.fail(Assert.java:96)
	at org.testng.Assert.failNotEquals(Assert.java:776)
	at org.testng.Assert.assertTrue(Assert.java:44)
	at org.openj9.test.com.ibm.jit.Test_JITHelpers.test_toUpperIntrinsicUTF16(Test_JITHelpers.java:261)

The second failure is the one observed in #4597. I'll test my fix locally tomorrow morning and attach it as a new commit to this PR.

[1] https://ci.eclipse.org/openj9/job/PullRequest-Sanity-JDK11-linux_390-64_cmprssptrs-OpenJ9/198/tapResults/

In eclipse-openj9#4597 we identified cases on which `toUpperCase` fails on Linux on
Z. This is due to some faulty login in the evaluator for these
intrinsics on Z. Bugs seem to have been introduced in eclipse-openj9#2318 for certain
characters. Namely the test in eclipse-openj9#4597 fails to convert the `\u00FF`
character. The bug is obvious, since `toUpperCase` of such a character
cannot be handled and the JIT evaluator determines the invalid
character range by checking if the character is greater than 0xFF, but
it should be checking if greater than or equal to.

In addition the compressed String evaluator also fails on 0xB5 as
determined by the unit tests. Both issues are fixed.

Fixes: eclipse-openj9#4597

Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>
@fjeremic fjeremic force-pushed the fix-case-conversion-unit-test branch from a6db4fd to 38f9961 Compare February 12, 2019 16:41
@fjeremic
Copy link
Contributor Author

And the fix is in. I've tested manually the tests pass but for good measure we'll test on the farm as well. Requesting reviews from one of @smlambert / @llxia for the test piece of the PR and one of @NigelYiboYu / @dhong44 for the Z codegen piece.

Jenkins test sanity xlinux,zlinux JDK8,JDK11

@fjeremic
Copy link
Contributor Author

@andrewcraik @0dvictor FYI we'll need to take a look at these x86 failures as my unit tests have caught what are likely bugs in the x86 toUpperCase and toLowerCase intrinsics. The output seems correct however so perhaps something else went wrong on the x86 side.

@andrewcraik
Copy link
Contributor

per discussion the issues on x86 are that we need to truncate the return value of the helper to be 0 or 1 per recent spec updates, ensure we check the disableHWAcceleratedStringCaseConv flag. With that the functional issues should be resolved and a remaining perf item will be to expand the number of characters converted by the Latin1 helpers which are currently limited to 0-127 (eg core ASCII). @0dvictor is having a look and coordinating with @fjeremic directly.

There are two functional issues with the String case conversion
intrinsics on x86:

1. The `getSupportsInlineStringCaseConversion` API is not respected and
hence there is no way to disable the specific intrinsic
2. The evaluator will return a non binary value for a truthful result
while it should only return a binary value (0 or 1)

We fix both issues here.

Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>
@fjeremic
Copy link
Contributor Author

Jenkins test sanity xlinux,zlinux JDK8,JDK11

The edge case tests needed to store a byte into a `char[]`, but this
has to be performed in an endian aware manner because the intrinsics
act on bytes and we subsequently do an array comparison which acts on
chars. We risk the high order byte of the char being different on
little endian vs. big endian following the conversion which makes the
tests faulty.

The logical and with 0xFF00 was written with big endian in mind. Here
we make it endian aware.

Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>
@fjeremic
Copy link
Contributor Author

Jenkins test sanity xlinux,zlinux JDK8,JDK11

@fjeremic
Copy link
Contributor Author

All issues have been fixed. This is ready for final review from committers.

@0xdaryl
Copy link
Contributor

0xdaryl commented Mar 4, 2019

@andrewcraik, can you give your +1 if you're fine with the x86 changes please?

@0xdaryl 0xdaryl self-assigned this Mar 4, 2019
@0xdaryl 0xdaryl merged commit 2f17a23 into eclipse-openj9:master Mar 4, 2019
@fjeremic fjeremic deleted the fix-case-conversion-unit-test branch March 4, 2019 16:53
@0xdaryl 0xdaryl added arch:z and removed comp:jit:x labels Jun 24, 2019
smlambert pushed a commit to adoptium/aqa-tests that referenced this pull request Jul 22, 2019
… list (#1230)

* PR to remove the UnicodeCasingTest from the problem list. Original issue has been patched for JDK11 eclipse-openj9/openj9#4688
mw918 pushed a commit to mw918/openjdk-tests that referenced this pull request Jul 24, 2019
… list (adoptium#1230)

* PR to remove the UnicodeCasingTest from the problem list. Original issue has been patched for JDK11 eclipse-openj9/openj9#4688
mw918 pushed a commit to mw918/openjdk-tests that referenced this pull request Jul 25, 2019
… list (adoptium#1230)

* PR to remove the UnicodeCasingTest from the problem list. Original issue has been patched for JDK11 eclipse-openj9/openj9#4688
karianna pushed a commit to adoptium/aqa-tests that referenced this pull request Aug 19, 2019
* Un-excluded java/lang/String/UnicodeCasingTest.java from problem list

* Updated issue links

Unicode casing test deleted as [#4688](eclipse-openj9/openj9#4688) fixes the issue

* Updated weblinks

* Updated issue links
smlambert pushed a commit to adoptium/aqa-tests that referenced this pull request Aug 22, 2019
* Un-excluded java/lang/String/UnicodeCasingTest.java from problem list

Unicode casing test deleted as [#4688](eclipse-openj9/openj9#4688) fixes the issue

* Links to permanent failures
smlambert pushed a commit to adoptium/aqa-tests that referenced this pull request Sep 6, 2019
* Un-excluded java/lang/String/UnicodeCasingTest.java from problem list
* Updated issue links
Unicode casing test deleted as [#4688](eclipse-openj9/openj9#4688) fixes the issue
smlambert pushed a commit to adoptium/aqa-tests that referenced this pull request Sep 19, 2019
* Un-excluded java/lang/String/UnicodeCasingTest.java from problem list

* Updated issue links

Unicode casing test deleted as [#4688](eclipse-openj9/openj9#4688) fixes the issue

* Updated weblinks

* Updated issue links

* Issue links updated

* Updated problem list weblinks

* Updated issue links

* Ricochet.java passes

java/lang/invoke/RicochetTest.java is now passing on nightly build's`11.0.5+6-201909111831` and upwards
```
openjdk version "11.0.5" 2019-10-15
OpenJDK Runtime Environment AdoptOpenJDK (build 11.0.5+6-201909111831)
Eclipse OpenJ9 VM AdoptOpenJDK (build master-d65742671, JRE 11 Linux amd64-64-Bit Compressed References 20190911_331 (JIT enabled, AOT enabled)
OpenJ9   - d65742671
OMR      - 7e9584ea
JCL      - 8057a0754d based on jdk-11.0.5+6)
```

* CustomizedLambdaForm has been fixed as per issue eclipse-openj9/openj9#7080

* perm excludes point to #1297
smlambert pushed a commit to adoptium/aqa-tests that referenced this pull request Sep 23, 2019
* Un-excluded java/lang/String/UnicodeCasingTest.java from problem list
* Updated issue links

Unicode casing test deleted as [#4688](eclipse-openj9/openj9#4688) fixes the issue

* Ricochet.java passes

java/lang/invoke/RicochetTest.java is now passing on nightly build's`11.0.5+6-201909111831` and upwards
```
* CustomizedLambdaForm has been fixed as per issue eclipse-openj9/openj9#7080

* perm excludes point to #1297

* Perm exclude SpecialInterfaceCall

* JDK11 Prob list links

* AutomaticModulesTest is now passing

* Reinclude currently failing tests

* Reincluded failing tests, PR#1359 already does this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JTReg test fail - Linux s390 : java/lang/String/UnicodeCasingTest.java
5 participants