String.compareTo intrinsic for amd64 #299

dsamersoff · 2018-02-17T18:50:19Z

Port of Chris Thalinger patch

dougxc · 2018-02-22T10:24:00Z

...mpiler.replacements.test/src/org/graalvm/compiler/replacements/test/StringCompareToTest.java

-     * Initialize variables
-     */
    public StringCompareToTest() {
+        if (Java8OrEarlier) {


Instead of this, use Assume.assumeFalse(Java8OrEarlier); (e.g.,

graal/compiler/src/org.graalvm.compiler.debug.test/src/org/graalvm/compiler/debug/test/DebugContextTest.java

Line 229 in bfe765f

public void testInvariantChecking() throws InterruptedException {

).

dougxc · 2018-02-22T10:26:08Z

...er/src/org.graalvm.compiler.asm.amd64/src/org/graalvm/compiler/asm/amd64/AMD64Assembler.java

+
+    private static class EvexTupleType {
+        public static final int EVEX_FV = 0;
+        // public static final int EVEX_HV = 4;


These bits of commented code need to be removed or uncommented when this PR is ready to merge.

Style check don't allow unused constants, but it's better to keep whole constant set for readability/future development.

You can solve this by making EvexTupleType non-private (package-protected should be sufficient to convince Checkstyle that it might be used).

tkrodriguez · 2018-02-28T07:13:23Z

...ements.amd64/src/org/graalvm/compiler/replacements/amd64/AMD64StringLatin1Substitutions.java

+
+    /**
+     * @param value is byte[]
+     * @param other is char[]


Shouldn't this be byte?

tkrodriguez · 2018-02-28T07:25:47Z

This looks ok to me. Can we use this implementation for JDK8? It seems like a pretty simple substitution for String.compareTo could use the same logic as the char/char variant but with length * 2 to compensate for the byte[] vs char[] difference. It would also give us more test exposure with the implementation since we don't really use JDK9. I'll run it through our internal gate and see if anything shows up.

tkrodriguez · 2018-02-28T08:32:47Z

...mpiler.replacements.test/src/org/graalvm/compiler/replacements/test/StringCompareToTest.java

+    };
+
+    public StringCompareToTest() {
+        Assume.assumeFalse(Java8OrEarlier);


Can you add an assumeTrue that the target arch is AMD64 as well?

tkrodriguez · 2018-02-28T21:50:18Z

I've gone ahead and fixed the issues I've mentioned as well as added a simple substitution for use in JDK8 along with a simple benchmark in StringBenchmark. If that's ok with you I'll push that through our gate.

dsamersoff · 2018-03-01T07:26:57Z

If that's ok with you I'll push that through our gate.

Tom, thank you for your support. Please, push the changes.

tkrodriguez · 2018-03-05T22:07:14Z

There was one actual bug in the code that I found in the gate. It had emission logic checking for SSE4.1 when it should have been checking for SSE4.2. It was found by assertion checking.

dsamersoff · 2018-03-06T07:42:55Z

Thank you!
Could you close #251 ?

christhalinger · 2018-03-09T06:26:19Z

Thanks for getting that in!

mur47x111 · 2018-03-14T23:45:40Z

...org.graalvm.compiler.lir.amd64/src/org/graalvm/compiler/lir/amd64/AMD64ArrayCompareToOp.java

+            masm.leaq(str1, new AMD64Address(str1, cnt2, scale1));
+            masm.leaq(str2, new AMD64Address(str2, cnt2, scale2));
+        }
+        masm.decrementl(cnt2);  // first character was compared already


This intrinsic causes segment fault transiently on Java 8. The problem occurs when cnt2 is 0 before this line. Then it decrements to 0xFFFFFFFF, and the next statement negates it to 0xFFFFFFFF00000001. This means on the entrance to this intrinsic min(cnt1, cnt2) is 1, and then gets right shifted to 0 at line 194. Given that we always double the length on Java 8, these two counters cannot be 1.

The crash log shows a corner case when we have String.compareTo in a loop, e.g., at TreeMap.getEntry. I suspect cnt1 is not reset within the loop but reuses the value from masm.pop(cnt1);. I am still working on reproducing it with a simple test case.

I finally managed to reproduce it on my local machine and have created a pull request.

dougxc · 2019-08-06T13:42:51Z

@dsamersoff there is a JDK bug (https://bugs.openjdk.java.net/browse/JDK-8228903) that seems to indicate a problem with String.compareTo intrinsic introduced by this PR.

dsamersoff · 2019-08-06T13:59:44Z

@dsamersoff there is a JDK bug (https://bugs.openjdk.java.net/browse/JDK-8228903) that seems to indicate a problem with String.compareTo intrinsic introduced by this PR.

I'll try to reproduce it in my setup and let you know if it succeeded. I also added myself to watchers of ojdk CR.

dougxc · 2019-08-12T07:36:12Z

@dsamersoff do you have access to a machine with capabilities that seem to be required for triggering this bug?

dsamersoff · 2019-08-12T07:49:58Z

No, I can't reproduce the issue on couple of machines I have access to

dsamersoff · 2019-08-12T07:56:47Z

@dougxc Could you try to disable the branch supportsAVX512VLBW inside AMD64ArrayCompareToOp:emitCode() and check whether it solves the problem or not?

dougxc · 2019-08-12T08:09:38Z

My Mac does not support AVX512VLBW. I'll find one of our test machines that does. In terms of a Graal unit test, does org.graalvm.compiler.replacements.test.StringCompareToTest provide the same coverage as TestStringIntrinsics?

dsamersoff · 2019-08-12T08:41:04Z

On brief check, StringCompareToTest from graal provides better coverage for compareTo, because it contains special sized strings to trigger tail comparison code.

dougxc · 2019-08-12T09:49:30Z

Interesting. We managed to reproduce the TestStringIntrinsics failure on an AVX512VLBW machine here. After applying the patch improved-error-message.patch attached to JDK-8229186, the compared values in the failure are:

"\u0000\u0001\u0002\u0003\u0004\u0005\u0006\u0007\u0008\u0009\n\u000B\u000C\r\u000E\u000F\u0010\u0011\u0012\u0013\u0014\u0015\u0016\u0017\u0018\u0019\u001A\u001B\u001C\u001D\u001E\u001F !\"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~\u007F\u0080\u0081\u0082\u0083\u0084\u0085\u0086\u0087\u0088\u0089\u008A\u008B\u008C\u008D\u008E\u008F\u0090\u0091\u0092\u0093\u0094\u0095\u0096\u0097\u0098\u0099\u009A\u009B\u009C\u009D\u009E\u009F\u00A0\u00A1\u00A2\u00A3\u00A4\u00A5\u00A6\u00A7\u00A8\u00A9\u00AA\u00AB\u00AC\u00AD\u00AE\u00AF\u00B0\u00B1\u00B2\u00B3\u00B4\u00B5\u00B6\u00B7\u00B8\u00B9\u00BA\u00BB\u00BC\u00BD\u00BE\u00BF\u00C0\u00C1\u00C2\u00C3\u00C4\u00C5\u00C6\u00C7\u00C8\u00C9\u00CA\u00CB\u00CC\u00CD\u00CE\u00CF\u00D0\u00D1\u00D2\u00D3\u00D4\u00D5\u00D6\u00D7\u00D8\u00D9\u00DA\u00DB\u00DC\u00DD\u00DE\u00DF\u00E0\u00E1\u00E2\u00E3\u00E4\u00E5\u00E6\u00E7\u00E8\u00E9\u00EA\u00EB\u00EC\u00ED\u00EE\u00EF\u00F0\u00F1\u00F2\u00F3\u00F4\u00F5\u00F6\u00F7\u00F8\u00F9\u00FA\u00FB\u00FC\u00FD\u00FE\u00FF"

and:

"\u0000\u0001\u0002\u0003\u0004\u0005\u0006\u0007\u0008\u0009\n\u000B\u000C\r\u000E\u000F\u0010\u0011\u0012\u0013\u0014\u0015\u0016\u0017\u0018\u0019\u001A\u001B\u001C\u001D\u001E\u001F !\"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~\u007F\u0080\u0081\u0082\u0083\u0084\u0085\u0086\u0087\u0088\u0089\u008A\u008B\u008C\u008D\u008E\u008F\u0090\u0091\u0092\u0093\u0094\u0095\u0096\u0097\u0098\u0099\u009A\u009B\u009C\u009D\u009E\u009F\u00A0\u00A1\u00A2\u00A3\u00A4\u00A5\u00A6\u00A7\u00A8\u00A9\u00AA\u00AB\u00AC\u00AD\u00AE\u00AF\u00B0\u00B1\u00B2\u00B3\u00B4\u00B5\u00B6\u00B7\u00B8\u00B9\u00BA\u00BB\u00BC\u00BD\u00BE\u00BF\u00C0\u00C1\u00C2\u00C3\u00C4\u00C5\u00C6\u00C7\u00C8\u00C9\u00CA\u00CB\u00CC\u00CD\u00CE\u00CF\u00D0\u00D1\u00D2\u00D3\u00D4\u00D5\u00D6\u00D7\u00D8\u00D9\u00DA\u00DB\u00DC\u00DD\u00DE\u00DF\u00E0\u00E1\u00E2\u00E3\u00E4\u00E5\u00E6\u00E7\u00E8\u04E9\u00EA\u00EB\u00EC\u00ED\u00EE\u00EF\u00F0\u00F1\u00F2\u00F3\u00F4\u00F5\u00F6\u00F7\u00F8\u00F9\u00FA\u00FB\u00FC\u00FD\u00FE\u00FF"

We added these to StringSubstitutionTestBase.testData but still StringCompareToTest passes.

dougxc · 2019-08-12T14:18:39Z

@mur47x111 is looking into this.

mur47x111 · 2019-08-12T16:44:26Z

...org.graalvm.compiler.lir.amd64/src/org/graalvm/compiler/lir/amd64/AMD64ArrayCompareToOp.java

+        // if (ae != StrIntrinsicNode::LL) {
+        if (kind1 == JavaKind.Byte && kind2 == JavaKind.Byte) {
+            stride2x2 = 0x20;
+        }


stride2x2 should be 0x40 in the case of comparing latin string with latin string. Indeed, ae != StrIntrinsicNode::LL should be !(kind1 == JavaKind.Byte && kind2 == JavaKind.Byte), right?

String.compareTo intrinsic for amd64

5983395

graalvmbot added the oca-signed label Feb 17, 2018

dsamersoff added 2 commits February 18, 2018 12:52

Comment out unused constants

b2efa9d

Fixed checkstyle errors, disable test for JDK8

9f69724

dougxc reviewed Feb 22, 2018

View reviewed changes

dsamersoff added 2 commits February 23, 2018 14:08

Skip test on jdk8 updated to use Assume

d5f8b0a

Change constants visibility to make checkstyle happy

1ef5d9d

tkrodriguez reviewed Feb 28, 2018

View reviewed changes

dougxc merged commit 1ef5d9d into oracle:master Mar 5, 2018

dougxc mentioned this pull request Mar 6, 2018

[AMD64] implement String.compareTo substitutions #251

Closed

dsamersoff deleted the string_compareTo_amd64 branch March 11, 2018 11:55

mur47x111 reviewed Mar 14, 2018

View reviewed changes

mur47x111 reviewed Aug 12, 2019

View reviewed changes

String.compareTo intrinsic for amd64 #299

String.compareTo intrinsic for amd64 #299

Uh oh!

Conversation

dsamersoff commented Feb 17, 2018

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!

tkrodriguez commented Feb 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tkrodriguez commented Feb 28, 2018

Uh oh!

dsamersoff commented Mar 1, 2018

Uh oh!

tkrodriguez commented Mar 5, 2018

Uh oh!

dsamersoff commented Mar 6, 2018

Uh oh!

christhalinger commented Mar 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougxc commented Aug 6, 2019

Uh oh!

dsamersoff commented Aug 6, 2019

Uh oh!

dougxc commented Aug 12, 2019

Uh oh!

dsamersoff commented Aug 12, 2019

Uh oh!

dsamersoff commented Aug 12, 2019

Uh oh!

dougxc commented Aug 12, 2019

Uh oh!

dsamersoff commented Aug 12, 2019

Uh oh!

dougxc commented Aug 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dougxc commented Aug 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dougxc commented Aug 12, 2019 •

edited

Loading