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

Vectorized core logic of String.indexOf() on X86 #1681

Merged
merged 1 commit into from Jun 22, 2018

Conversation

@0dvictor
Copy link
Contributor

commented Apr 13, 2018

Core logic of String.indexOf() is separated into an
internal helper function, which is implemented with
SIMD instructions on X86.

Signed-off-by: Victor Ding dvictor@ca.ibm.com

}
}
return -1;
}

This comment has been minimized.

Copy link
@fjeremic

fjeremic Apr 13, 2018

Contributor

Would it be possible to have consistent coding style with the rest of the file?

This comment has been minimized.

Copy link
@0dvictor

0dvictor Apr 13, 2018

Author Contributor

For sure. Updated.

@0dvictor 0dvictor force-pushed the 0dvictor:indexof branch 4 times, most recently Apr 13, 2018

@0dvictor 0dvictor changed the title Vectorized core logic of String.indexOf() on X86 WIP: Vectorized core logic of String.indexOf() on X86 Apr 13, 2018

@0dvictor 0dvictor force-pushed the 0dvictor:indexof branch 2 times, most recently Apr 13, 2018

@0dvictor 0dvictor changed the title WIP: Vectorized core logic of String.indexOf() on X86 Vectorized core logic of String.indexOf() on X86 Apr 13, 2018

@0dvictor 0dvictor force-pushed the 0dvictor:indexof branch 2 times, most recently Apr 14, 2018

@0dvictor 0dvictor changed the title Vectorized core logic of String.indexOf() on X86 WIP: Vectorized core logic of String.indexOf() on X86 Apr 27, 2018

@0dvictor 0dvictor force-pushed the 0dvictor:indexof branch 2 times, most recently Apr 27, 2018

@0dvictor 0dvictor force-pushed the 0dvictor:indexof branch 2 times, most recently May 11, 2018

@fjeremic

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2018

@0dvictor I think something bad happen with a rebase here. There are a ton of foreign commits.

@0dvictor 0dvictor force-pushed the 0dvictor:indexof branch 3 times, most recently Jun 4, 2018

@0dvictor

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2018

Thank you for the reminder. A rebase went bad. I've fixed it.

return i;
}
}
return helpers.findElementFromArray(array, (byte)c, start, len);

This comment has been minimized.

Copy link
@fjeremic

fjeremic Jun 5, 2018

Contributor

There are two implementations of public int indexOf(int c, int start) in this file. One for Java 8 and one for Java 9+. Could we make the same change for the Java 9+ version?

This comment has been minimized.

Copy link
@0dvictor

0dvictor Jun 5, 2018

Author Contributor

Sorry I didn't realized that the two functions now are separated. I've updated both of them.

@0dvictor 0dvictor force-pushed the 0dvictor:indexof branch Jun 5, 2018

@andrewcraik

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2018

@0dvictor the change from @msqb has been merged - is this ready to go?

@0dvictor 0dvictor force-pushed the 0dvictor:indexof branch Jun 19, 2018

@0dvictor

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2018

I've rebased the change. I'm re-running all the test to satisfy myself that the change is still good after rebasing. Will remove WIP as soon as the tests clear.

@0dvictor

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2018

The code has been re-tested and therefore is ready for merging.

@0dvictor 0dvictor changed the title WIP: Vectorized core logic of String.indexOf() on X86 Vectorized core logic of String.indexOf() on X86 Jun 19, 2018

@0dvictor 0dvictor force-pushed the 0dvictor:indexof branch Jun 21, 2018

runtime/compiler/env/VMJ9.cpp Outdated
@@ -3244,6 +3244,7 @@ int TR_J9VMBase::checkInlineableTarget (TR_CallTarget* target, TR_CallSite* call
// Don't inline methods that are going to be reduced in ilgen or UnsafeFastPath
switch (rm)
{
case TR::com_ibm_jit_JITHelpers_findElementFromArray:

This comment has been minimized.

Copy link
@andrewcraik

andrewcraik Jun 21, 2018

Contributor

so this isn't reduced by ilgen or UnsafeFastPath so the comment above now seems to be misleading...

what happens on platforms that don't support this call in the codegen? I think there needs to be a query so that if this isn't supported in the codegen we add it to the always inline list. The danger being until other codegens support this helper they will see a perf degradation...

This comment has been minimized.

Copy link
@0dvictor

0dvictor Jun 21, 2018

Author Contributor

Good point, I've moved this into the CG specific query suppressInliningOfRecognizedMethod(), so that each CG can opt-in/opt-out this optimization.

This comment has been minimized.

Copy link
@andrewcraik

andrewcraik Jun 21, 2018

Contributor

Can we make sure that if the inlining isn't suppressed we put it into the list of always worth inlining to avoid regressions on the platforms which don't initially opt in?

This comment has been minimized.

Copy link
@0dvictor

0dvictor Jun 21, 2018

Author Contributor

In theory the Inliner should make better decision whether to inline findElementFromArray, but I see your point about affecting the existing code as little as possible to minimize surprises.
I've update that findElementFromArray is always inlined if CG does not request suppressing inlining of it.

@0dvictor 0dvictor force-pushed the 0dvictor:indexof branch 2 times, most recently Jun 21, 2018

Victor Ding
Vectorized core logic of String.indexOf() on X86
Core logic of String.indexOf() is separated into an
internal helper function, which is implemented with
SIMD instructions on X86.

Signed-off-by: Victor Ding <dvictor@ca.ibm.com>

@0dvictor 0dvictor force-pushed the 0dvictor:indexof branch to 9e493e2 Jun 21, 2018

@andrewcraik

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2018

Jenkins test sanity

@andrewcraik

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2018

win64 suffered a GIT failure, and win failed because of #2129. Others are clean. Change looks good - merging.

@andrewcraik andrewcraik merged commit f28a24d into eclipse:master Jun 22, 2018

16 of 18 checks passed

Sanity - JDK8 - win_x86 Build finished.
Details
Sanity - JDK8 - win_x86-64_cmprssptrs Build finished.
Details
Copyright Check Copyrights appear to be up to date
Details
Line Endings Check Line endings appear to be correct
Details
Sanity - JDK10 - aix_ppc-64_cmprssptrs Build finished.
Details
Sanity - JDK10 - linux_390-64_cmprssptrs Build finished.
Details
Sanity - JDK10 - linux_ppc-64_cmprssptrs_le Build finished.
Details
Sanity - JDK10 - linux_x86-64 Build finished.
Details
Sanity - JDK10 - linux_x86-64_cmprssptrs Build finished.
Details
Sanity - JDK10 - win_x86-64_cmprssptrs Build finished.
Details
Sanity - JDK8 - aix_ppc-64_cmprssptrs Build finished.
Details
Sanity - JDK8 - linux_390-64_cmprssptrs Build finished.
Details
Sanity - JDK8 - linux_ppc-64_cmprssptrs_le Build finished.
Details
Sanity - JDK8 - linux_x86-64 Build finished.
Details
Sanity - JDK8 - linux_x86-64_cmprssptrs Build finished.
Details
Signed-off-by Check All commits appear to have proper Sign-off
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
ip-validation
Details

@0dvictor 0dvictor deleted the 0dvictor:indexof branch Jun 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.