-
Notifications
You must be signed in to change notification settings - Fork 734
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
Accelerate ArraysSupport.vectorizedMismatch in IL #16662
Accelerate ArraysSupport.vectorizedMismatch in IL #16662
Conversation
53e32e6
to
256dd74
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Spencer-Comin, Can you upload a sample log file printing trees before and after this particular optimization?
|
ff8662b
to
693be7e
Compare
693be7e
to
3fbd0ea
Compare
@@ -134,6 +134,12 @@ J9::X86::CodeGenerator::initialize() | |||
cg->setSupportsBDLLHardwareOverflowCheck(); | |||
} | |||
|
|||
static char *disableInlineVectorizedMismatch = feGetEnv("TR_disableInlineVectorizedMismatch"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@0xdaryl I've only considered arraycmp
support in enabling this acceleration on x. Is there anything else to take into consideration here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes looks OK to me apart from couple of minor nitpicks in comments. @0xdaryl Can I request you to give your review as well to this change?
3fbd0ea
to
f48cfcc
Compare
@Spencer-Comin Do we have any functional tests for these changes ? OR we should contribute one ? |
Nothing I see in OpenJ9. There are JDK tests that use $ grep -rnw `pwd` -e 'Arrays\.compare'
jdk11/test/jdk/java/net/httpclient/http2/server/Http2TestServerConnection.java:309: if (Arrays.compare(clientPreface, bytes) != 0) {
jdk11/test/jdk/java/util/Arrays/ArraysEqCmpTest.java:755: Assert.assertTrue(Arrays.compare(a, b) < 0);
jdk11/test/jdk/java/util/Arrays/ArraysEqCmpTest.java:756: Assert.assertTrue(Arrays.compare(b, a) > 0);
jdk11/test/jdk/java/util/Arrays/ArraysEqCmpTest.java:1037: testNPE(() -> Arrays.compare(o1, o2, o3));
jdk11/test/jdk/java/util/Arrays/ArraysEqCmpTest.java:1042: testNPE(() -> Arrays.compare(o1, 0, 0, o2, 0, 0, o3));
jdk11/test/jdk/java/nio/file/Files/ReadWriteString.java:218: assertTrue((Arrays.compare(bytes, bytes2) == 0), "The bytes should be the same");
jdk11/test/jdk/com/sun/crypto/provider/Cipher/DES/DESKeyCleanupTest.java:70: } while (Arrays.compare(zeros, array) != 0);
jdk11/test/jdk/com/sun/crypto/provider/Cipher/PBE/PBEKeyCleanupTest.java:94: } while (Arrays.compare(zeros, array) != 0);
jdk11/test/lib/jdk/test/lib/OSVersion.java:89: return Arrays.compare(this.versionTokens, o.versionTokens);
jdk11/test/lib/jdk/test/lib/containers/cgroup/MetricsTesterCgroupV1.java:434: if (Arrays.compare(oldVal, newVal) != 0) {
jdk11/test/lib/jdk/test/lib/containers/cgroup/MetricsTesterCgroupV1.java:446: if (Arrays.compare(oldVal, newVal) != 0) {
jdk11/test/lib/jdk/test/lib/containers/cgroup/MetricsTesterCgroupV1.java:456: if (Arrays.compare(oldVal, newVal) != 0) {
jdk11/test/lib/jdk/test/lib/containers/cgroup/MetricsTesterCgroupV1.java:468: if (Arrays.compare(oldVal, newVal) != 0) {
jdk11/test/lib/jdk/test/lib/containers/cgroup/MetricsTesterCgroupV2.java:357: if (Arrays.compare(oldVal, newVal) != 0) {
jdk11/test/lib/jdk/test/lib/containers/cgroup/MetricsTesterCgroupV2.java:367: if (Arrays.compare(oldVal, newVal) != 0) {
jdk11/test/lib/jdk/test/lib/containers/cgroup/MetricsTesterCgroupV2.java:377: if (Arrays.compare(oldVal, newVal) != 0) {
jdk11/test/lib/jdk/test/lib/containers/cgroup/MetricsTesterCgroupV2.java:387: if (Arrays.compare(oldVal, newVal) != 0) {
$ grep -rnw `pwd` -e 'Arrays\.mismatch'
jdk11/test/jdk/tools/jmod/hashes/HashesOrderTest.java:150: int i = Arrays.mismatch(buffer1, 0, nRead1, buffer2, 0, nRead2);
jdk11/test/jdk/tools/jlink/JLinkReproducibleTest.java:134: int i = Arrays.mismatch(buffer1, 0, nRead1, buffer2, 0, nRead2);
jdk11/test/jdk/java/util/Arrays/ArraysEqCmpTest.java:1038: testNPE(() -> Arrays.mismatch(o1, o2, o3));
jdk11/test/jdk/java/util/Arrays/ArraysEqCmpTest.java:1043: testNPE(() -> Arrays.mismatch(o1, 0, 0, o2, 0, 0, o3));
jdk11/test/jdk/java/rmi/testlibrary/TestSocketFactory.java:832: long index = Arrays.mismatch(actual, expected);
jdk11/test/jdk/javax/xml/crypto/dsig/Versions.java:56: int i = Arrays.mismatch(buffer1, 0, nRead1, buffer2, 0, nRead2);
jdk11/test/hotspot/jtreg/compiler/arraycopy/stress/StressBooleanArrayCopy.java:41: int m = Arrays.mismatch(test, 0, size,
jdk11/test/hotspot/jtreg/compiler/arraycopy/stress/StressBooleanArrayCopy.java:53: int m = Arrays.mismatch(test, r, r+len,
jdk11/test/hotspot/jtreg/compiler/arraycopy/stress/StressBooleanArrayCopy.java:62: int m = Arrays.mismatch(test, 0, r,
jdk11/test/hotspot/jtreg/compiler/arraycopy/stress/StressBooleanArrayCopy.java:69: int m = Arrays.mismatch(test, r + len, size,
jdk11/test/hotspot/jtreg/compiler/arraycopy/stress/StressIntArrayCopy.java:41: int m = Arrays.mismatch(test, 0, size,
jdk11/test/hotspot/jtreg/compiler/arraycopy/stress/StressIntArrayCopy.java:53: int m = Arrays.mismatch(test, r, r+len,
jdk11/test/hotspot/jtreg/compiler/arraycopy/stress/StressIntArrayCopy.java:62: int m = Arrays.mismatch(test, 0, r,
jdk11/test/hotspot/jtreg/compiler/arraycopy/stress/StressIntArrayCopy.java:69: int m = Arrays.mismatch(test, r + len, size,
jdk11/test/hotspot/jtreg/compiler/arraycopy/stress/StressLongArrayCopy.java:41: int m = Arrays.mismatch(test, 0, size,
jdk11/test/hotspot/jtreg/compiler/arraycopy/stress/StressLongArrayCopy.java:53: int m = Arrays.mismatch(test, r, r+len,
jdk11/test/hotspot/jtreg/compiler/arraycopy/stress/StressLongArrayCopy.java:62: int m = Arrays.mismatch(test, 0, r,
jdk11/test/hotspot/jtreg/compiler/arraycopy/stress/StressLongArrayCopy.java:69: int m = Arrays.mismatch(test, r + len, size,
jdk11/test/hotspot/jtreg/compiler/arraycopy/stress/StressCharArrayCopy.java:41: int m = Arrays.mismatch(test, 0, size,
jdk11/test/hotspot/jtreg/compiler/arraycopy/stress/StressCharArrayCopy.java:53: int m = Arrays.mismatch(test, r, r+len,
jdk11/test/hotspot/jtreg/compiler/arraycopy/stress/StressCharArrayCopy.java:62: int m = Arrays.mismatch(test, 0, r,
jdk11/test/hotspot/jtreg/compiler/arraycopy/stress/StressCharArrayCopy.java:69: int m = Arrays.mismatch(test, r + len, size,
jdk11/test/hotspot/jtreg/compiler/arraycopy/stress/StressObjectArrayCopy.java:41: int m = Arrays.mismatch(test, 0, size,
jdk11/test/hotspot/jtreg/compiler/arraycopy/stress/StressObjectArrayCopy.java:53: int m = Arrays.mismatch(test, r, r+len,
jdk11/test/hotspot/jtreg/compiler/arraycopy/stress/StressObjectArrayCopy.java:62: int m = Arrays.mismatch(test, 0, r,
jdk11/test/hotspot/jtreg/compiler/arraycopy/stress/StressObjectArrayCopy.java:69: int m = Arrays.mismatch(test, r + len, size,
jdk11/test/hotspot/jtreg/compiler/arraycopy/stress/StressByteArrayCopy.java:41: int m = Arrays.mismatch(test, 0, size,
jdk11/test/hotspot/jtreg/compiler/arraycopy/stress/StressByteArrayCopy.java:53: int m = Arrays.mismatch(test, r, r+len,
jdk11/test/hotspot/jtreg/compiler/arraycopy/stress/StressByteArrayCopy.java:62: int m = Arrays.mismatch(test, 0, r,
jdk11/test/hotspot/jtreg/compiler/arraycopy/stress/StressByteArrayCopy.java:69: int m = Arrays.mismatch(test, r + len, size,
jdk11/test/hotspot/jtreg/compiler/arraycopy/stress/StressFloatArrayCopy.java:41: int m = Arrays.mismatch(test, 0, size,
jdk11/test/hotspot/jtreg/compiler/arraycopy/stress/StressFloatArrayCopy.java:53: int m = Arrays.mismatch(test, r, r+len,
jdk11/test/hotspot/jtreg/compiler/arraycopy/stress/StressFloatArrayCopy.java:62: int m = Arrays.mismatch(test, 0, r,
jdk11/test/hotspot/jtreg/compiler/arraycopy/stress/StressFloatArrayCopy.java:69: int m = Arrays.mismatch(test, r + len, size,
jdk11/test/hotspot/jtreg/compiler/arraycopy/stress/StressShortArrayCopy.java:41: int m = Arrays.mismatch(test, 0, size,
jdk11/test/hotspot/jtreg/compiler/arraycopy/stress/StressShortArrayCopy.java:53: int m = Arrays.mismatch(test, r, r+len,
jdk11/test/hotspot/jtreg/compiler/arraycopy/stress/StressShortArrayCopy.java:62: int m = Arrays.mismatch(test, 0, r,
jdk11/test/hotspot/jtreg/compiler/arraycopy/stress/StressShortArrayCopy.java:69: int m = Arrays.mismatch(test, r + len, size,
jdk11/test/hotspot/jtreg/compiler/arraycopy/stress/StressDoubleArrayCopy.java:41: int m = Arrays.mismatch(test, 0, size,
jdk11/test/hotspot/jtreg/compiler/arraycopy/stress/StressDoubleArrayCopy.java:53: int m = Arrays.mismatch(test, r, r+len,
jdk11/test/hotspot/jtreg/compiler/arraycopy/stress/StressDoubleArrayCopy.java:62: int m = Arrays.mismatch(test, 0, r,
jdk11/test/hotspot/jtreg/compiler/arraycopy/stress/StressDoubleArrayCopy.java:69: int m = Arrays.mismatch(test, r + len, size, |
@0xdaryl Can we get your review on these changes? |
@0xdaryl / @jdmpapin Can I request your review on this change from @Spencer-Comin ? |
f48cfcc
to
c27af73
Compare
With aarch64 now supporting |
@Spencer-Comin Thanks. Your change looks good to me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of minor nits. Generally looks ok.
c27af73
to
5b0c361
Compare
Jenkins test sanity all jdk17 |
c4373c8
to
5049d1e
Compare
Converting this to draft while we resolve the size restriction on |
5049d1e
to
1ff5372
Compare
I've rewritten this to use |
1ff5372
to
dd490e9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already reviewed this, so just went through again, looks good to me. @jdmpapin as the PR for arrayCmpLen will be merged in coming days, requesting your review on this one.
@@ -124,6 +124,12 @@ J9::Z::CodeGenerator::initialize() | |||
cg->setSupportsInlineEncodeASCII(); | |||
} | |||
|
|||
static bool disableInlineVectorizedMismatch = feGetEnv("TR_disableInlineVectorizedMismatch") != NULL; | |||
if (cg->getSupportsArrayCmpLen() && !disableInlineVectorizedMismatch) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getSupportsArrayCmpLen would be disabled for arraylets and off-heap, right? Just confirming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eclipse-omr/omr#6983 enables arraycmplen
in the same way arraycmp
is enabled on each platform:
- AArch64 - disabled for arraylets, also disabled by environment variable
TR_aarch64DisableArrayCmpLen
- Power - always enabled
- x86 - disabled for arraylets
- Z - always enabled
arraycmplen
should probably be uniformly disabled for arraylets, and maybe by a single environment variable for all platforms. Same for arraycmp
. I'll add a commit to eclipse-omr/omr#6983 to make . Edit: To not put another obstacle to the beleaguered arraycmp
enablement more uniform and amend the commit introducing arraycmplen
arraycmplen
PRs, I'll make this change in its own PR (eclipse-omr/omr#7108).
Not sure how off-heap will affect arraycmp(len)
enablement, @VermaSh any input?
dd490e9
to
517f288
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, @jdmpapin Would appreciate if you can give your review on these changes as well and if OK, merge it.
@Spencer-Comin seems like there has been merge conflicts. Can you resolve them ? @jdmpapin As arraycmp opcode changes have been merged, Can we get your review on these changes? |
517f288
to
de7ee82
Compare
de7ee82
to
dcc15e3
Compare
Thanks! I suppose I should have also mentioned the places in the comment that show 64-bit shift amounts, since those are now inconsistent with the implementation |
dcc15e3
to
b83f994
Compare
This patch adds ArraysSupport.vectorizedMismatch as a recognized method, and adds a SupportsInlineVectorizedMismatch flag to the code generator. This flag is set in Z, Power, aarch64 and x86 code generator initialization if the arraycmp opcode is supported and the TR_disableInlineVectorizedMismatch environment variable is not set. If the flag is set, vectorizedMismatch call nodes are transformed to a functionally equivalent tree that uses arraycmp. Fixes: eclipse-openj9#15204 Signed-off-by: Spencer Comin <[email protected]>
b83f994
to
0636989
Compare
I've resolved the conflict, @jdmpapin is there anything left to do before this PR can begin the merge process? |
Due to the state of PR builds, I'm coordinating with Spencer to test this PR privately |
This has now passed JDK17 sanity.functional on Linux with x86-64, PPC64LE, z, and AArch64, with the exception of a number of unrelated CRIU test failures caused by machine configuration. Merging |
This patch adds
ArraysSupport.vectorizedMismatch
as a recognized method. If thearraycmp
IL opcode is supported,vectorizedMismatch
call nodes are transformed to a functionally equivalent tree usingarraycmp
.vectorizedMismatch
is functionally equivalent to:Therefore the call nodes can be translated like so:
becomes
ArraysSupport.vectorizedMismatch
is used in the implementations ofArrays.mismatch
,Arrays.compare
, andArrays.equals
for primitive types.Benchmarking
Arrays.compare
on x86, Power, and Z for 128, 4096, and 8192 element arrays of all primitive types shows a speedup of around 4.2× on x86, 2.7× on Power, and 4.1× on Z. Benchmark arrays of 1 or 2 elements shows no significant slowdown, and in some cases a speedup (notably, I saw a speedup of 3× comparing two element arrays oflong
).Full benchmarking results here, for the curious: arraycompare-results.zip
Fixes: #15204