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.

Sign up for GitHub

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

Re-enable toLowerCase/toUpperCase acceleration #2098

Merged
merged 2 commits into from
Jul 4, 2018
Merged

Re-enable toLowerCase/toUpperCase acceleration #2098

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b89ef07
Re-enable toLowerCase/toUpperCase acceleration
dhong44 Jun 7, 2018
635a9ad
Remove unnecessary String constructors for toUpper/toLower
dhong44 Jun 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 94 additions & 0 deletions jcl/src/java.base/share/classes/com/ibm/jit/JITHelpers.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,100 @@ public int getJ9ClassFromObject32(Object obj) {
}


/*
* To be recognized by the JIT and returns true if the hardware supports SIMD case conversion.
*/
public boolean supportsIntrinsicCaseConversion() {
return false;
}

/**
* To be used by the JIT when performing SIMD upper case conversion with Latin 1 strings.
* @param value the underlying array for the source string.
* @param output a new array which will be used for the converted string.
* @param length the number of bytes used to represent the string.
* @return True if intrinsic conversion was successful, false if fallback conversion must be used.
*/
public boolean toUpperIntrinsicLatin1(byte[] value, byte[] output, int length) {
return false;
}

/**
* To be used by the JIT when performing SIMD lower case conversion with Latin 1 strings.
* @param value the underlying array for the source string.
* @param output a new array which will be used for the converted string.
* @param length the number of bytes used to represent the string.
* @return True if intrinsic conversion was successful, false if fallback conversion must be used.
*/
public boolean toLowerIntrinsicLatin1(byte[] value, byte[] output, int length) {
return false;
}

/**
* To be used by the JIT when performing SIMD upper case conversion with UTF16 strings.
* @param value the underlying array for the source string.
* @param output a new array which will be used for the converted string.
* @param length the number of bytes used to represent the string.
* @return True if intrinsic conversion was successful, false if fallback conversion must be used.
*/
public boolean toUpperIntrinsicUTF16(byte[] value, byte[] output, int length) {
return false;
}

/**
* To be used by the JIT when performing SIMD lower case conversion with UTF16 strings.
* @param value the underlying array for the source string.
* @param output a new array which will be used for the converted string.
* @param length the number of bytes used to represent the string.
* @return True if intrinsic conversion was successful, false if fallback conversion must be used.
*/
public boolean toLowerIntrinsicUTF16(byte[] value, byte[] output, int length) {
return false;
}
/**
* To be used by the JIT when performing SIMD upper case conversion with Latin 1 strings.
* @param value the underlying array for the source string.
* @param output a new array which will be used for the converted string.
* @param length the number of bytes used to represent the string.
* @return True if intrinsic conversion was successful, false if fallback conversion must be used.
*/
public boolean toUpperIntrinsicLatin1(char[] value, char[] output, int length) {
return false;
}

/**
* To be used by the JIT when performing SIMD lower case conversion with Latin 1 strings.
* @param value the underlying array for the source string.
* @param output a new array which will be used for the converted string.
* @param length the number of bytes used to represent the string.
* @return True if intrinsic conversion was successful, false if fallback conversion must be used.
*/
public boolean toLowerIntrinsicLatin1(char[] value, char[] output, int length) {
return false;
}

/**
* To be used by the JIT when performing SIMD upper case conversion with UTF16 strings.
* @param value the underlying array for the source string.
* @param output a new array which will be used for the converted string.
* @param length the number of bytes used to represent the string.
* @return True if intrinsic conversion was successful, false if fallback conversion must be used.
*/
public boolean toUpperIntrinsicUTF16(char[] value, char[] output, int length) {
return false;
}

/**
* To be used by the JIT when performing SIMD lower case conversion with UTF16 strings.
* @param value the underlying array for the source string.
* @param output a new array which will be used for the converted string.
* @param length the number of bytes used to represent the string.
* @return True if intrinsic conversion was successful, false if fallback conversion must be used.
*/
public boolean toLowerIntrinsicUTF16(char[] value, char[] output, int length) {
return false;
}

/*
* sun.misc.Unsafe.get* and put* have to generate internal control flow for correctness due to different object shapes. The JIT emitted sequences
* checks for and handles: 1) standard fields in normal objects 2) static fields from classes 3) entries from arrays This sequence is branchy and
Expand Down
161 changes: 60 additions & 101 deletions jcl/src/java.base/share/classes/java/lang/String.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,6 @@ private void checkLastChar(char lastChar) {
/*[IF Sidecar19-SE]*/
// DO NOT CHANGE OR MOVE THIS LINE
// IT MUST BE THE FIRST THING IN THE INITIALIZATION
private static final boolean STRING_OPT_IN_HW = StrCheckHWAvailable();
private static final long serialVersionUID = -6849794470754667710L;

/**
Expand Down Expand Up @@ -878,17 +877,6 @@ public String(String string) {
hashCode = string.hashCode;
}

/**
* Creates a new string from the with specified length
*
* @param numChars
* length of new string
*/
private String(int numChars) {
value = new byte[numChars * 2];
coder = UTF16;
}

/**
* Creates a string from the contents of a StringBuffer.
*
Expand Down Expand Up @@ -2709,15 +2697,26 @@ public String toLowerCase(Locale locale) {
return this;
}

if (StrHWAvailable() && language == "en") { //$NON-NLS-1$
String output = new String(lengthInternal());
if (toLowerHWOptimized(output))
return output;
}
int sLength = lengthInternal();

if (enableCompression && (null == compressionFlag || coder == LATIN1)) {

if (helpers.supportsIntrinsicCaseConversion() && language == "en") { //$NON-NLS-1$
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The interpreter will never execute this code which means it will appear as a cold path due to no interpreter profiling data. Does the intrinsic recognition overcome this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe so. The interpreter will enter the if block immediately above on line 2728 so we will have profiling data for that path. The JIT will fold away the call to helpers.supportsIntrinsicCaseConversion() as a constant at compile time and the flow algorithm in the JIT should propagate the block frequencies to the if at line 2725 to be equal to that of the preceding if.

Just to be cautious though @dhong44 can we collect a log and verify the block frequencies we compute for the block in which we inline the intrinsic? Given your expertise @andrewcraik is my reasoning here valid?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The key question is when the supportsIntrinsicCaseConversion is folded. It looks to be in ilgen so the flow for IProfiling will have a chance to happen - other profilers do not do a flow. The key thing is to check the frequency on the blocks with the custom implementation after cold block marking. I think a log is the best way to verify this is working right.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

n1398n    BBStart <block_125> (freq 5002)                                                     [     0x3ff4315a4b0] bci=[10,116,2664] rc=0 vc=257 vn=- li=- udi=- nc=0
n1432n    treetop                                                                             [     0x3ff4315af50] bci=[10,122,2664] rc=0 vc=257 vn=- li=- udi=- nc=1
n1431n      newarray  jitNewArray[#176  helper Method] [flags 0x400 0x0 ] (skipZeroInit sharedMemory )  [     0x3ff4315af00] bci=[10,122,2664] rc=2 vc=257 vn=- li=- udi=- nc=2 flg=0x8000
n1429n        ishl                                                                            [     0x3ff4315ae60] bci=[10,121,2664] rc=1 vc=257 vn=- li=- udi=- nc=2
n1426n          iload  <auto slot 3>[#796  Auto] [flags 0x3 0x0 ]                             [     0x3ff4315ad70] bci=[10,116,2664] rc=1 vc=257 vn=- li=- udi=- nc=0
n1428n          iloadi  java/lang/String.coder B[#802  final Shadow +8] [flags 0xa0601 0x0 ]  [     0x3ff4315ae10] bci=[10,118,2664] rc=1 vc=257 vn=- li=- udi=- nc=1
n1427n            aload  <temp slot 3>[#786  Auto] [flags 0x7 0x0 ] (X!=0 sharedMemory )      [     0x3ff4315adc0] bci=[10,117,2664] rc=1 vc=257 vn=- li=- udi=- nc=0 flg=0x4
n1430n        iconst 8   ; array type is byte                                                 [     0x3ff4315aeb0] bci=[10,122,2664] rc=1 vc=257 vn=- li=- udi=- nc=0
n1433n    astore  <auto slot 4>[#810  Auto] [flags 0x7 0x0 ]                                  [     0x3ff4315afa0] bci=[10,124,2664] rc=0 vc=257 vn=- li=- udi=- nc=1
n1431n      ==>newarray
n1438n    compressedRefs                                                                      [     0x3ff4315b130] bci=[10,130,2666] rc=0 vc=257 vn=- li=- udi=- nc=2
n1436n      aloadi  java/lang/String.value [B[#804  final Shadow +4] [flags 0x400a0607 0x0 ]  [     0x3ff4315b090] bci=[10,130,2666] rc=2 vc=257 vn=- li=- udi=- nc=1
n1435n        aload  <temp slot 3>[#786  Auto] [flags 0x7 0x0 ] (X!=0 sharedMemory )          [     0x3ff4315b040] bci=[10,129,2666] rc=1 vc=257 vn=- li=- udi=- nc=0 flg=0x4
n1437n      lconst 0                                                                          [     0x3ff4315b0e0] bci=[10,130,2666] rc=1 vc=257 vn=- li=- udi=- nc=0
n1444n    NULLCHK on n1434n [#30]                                                             [     0x3ff4315b310] bci=[10,138,2666] rc=0 vc=257 vn=- li=- udi=- nc=1
n1443n      icall  com/ibm/jit/JITHelpers.toLowerIntrinsicUTF16([B[BI)Z[#815  final virtual Method -136] [flags 0x20500 0x0 ] ()  [     0x3ff4315b2c0] bci=[10,138,2666] rc=3 vc=257 vn=- li=- udi=- nc=4 flg=0x20
n1434n        aload  java/lang/String.helpers Lcom/ibm/jit/JITHelpers;[#799  final Static] [flags 0xa0307 0x0 ]  [     0x3ff4315aff0] bci=[10,126,2666] rc=1 vc=257 vn=- li=- udi=- nc=0
n1436n        ==>aloadi
n1439n        aload  <auto slot 4>[#810  Auto] [flags 0x7 0x0 ]                               [     0x3ff4315b180] bci=[10,133,2666] rc=1 vc=257 vn=- li=- udi=- nc=0
n1442n        imul                                                                            [     0x3ff4315b270] bci=[10,137,2666] rc=1 vc=257 vn=- li=- udi=- nc=2
n1440n          iload  <auto slot 3>[#796  Auto] [flags 0x3 0x0 ]                             [     0x3ff4315b1d0] bci=[10,135,2666] rc=1 vc=257 vn=- li=- udi=- nc=0
n1441n          iconst 2                                                                      [     0x3ff4315b220] bci=[10,136,2666] rc=1 vc=257 vn=- li=- udi=- nc=0
n1445n    istore  <pending push temp 0>[#793  Auto] [flags 0x3 0x800 ]                        [     0x3ff4315b360] bci=[10,138,2666] rc=0 vc=257 vn=- li=- udi=- nc=1

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be working as intended. I validated the log with @dhong44. We seem to have good frequencies on the blocks containing the intrinsic calls.

byte[] output = new byte[sLength << coder];

if (helpers.toLowerIntrinsicLatin1(value, output, sLength)) {
return new String(output, LATIN1);
}
}
return StringLatin1.toLowerCase(this, value, locale);
} else {
if (helpers.supportsIntrinsicCaseConversion() && language == "en") { //$NON-NLS-1$
byte[] output = new byte[sLength << coder];

if (helpers.toLowerIntrinsicUTF16(value, output, sLength * 2)) {
return new String(output, UTF16);
}
}
return StringUTF16.toLowerCase(this, value, locale);
}
}
Expand Down Expand Up @@ -2755,15 +2754,26 @@ public String toUpperCase(Locale locale) {
return this;
}

if (StrHWAvailable() && language == "en") { //$NON-NLS-1$
String output = new String(lengthInternal());
if (toUpperHWOptimized(output))
return output;
}
int sLength = lengthInternal();

if (enableCompression && (null == compressionFlag || coder == LATIN1)) {

if (helpers.supportsIntrinsicCaseConversion() && language == "en") { //$NON-NLS-1$
byte[] output = new byte[sLength << coder];

if (helpers.toUpperIntrinsicLatin1(value, output, sLength)) {
return new String(output, LATIN1);
}
}
return StringLatin1.toUpperCase(this, value, locale);
} else {
if (helpers.supportsIntrinsicCaseConversion() && language == "en") { //$NON-NLS-1$
byte[] output = new byte[sLength << coder];

if (helpers.toUpperIntrinsicUTF16(value, output, sLength * 2)) {
return new String(output, UTF16);
}
}
return StringUTF16.toUpperCase(this, value, locale);
}
}
Expand Down Expand Up @@ -3811,33 +3821,6 @@ public static String join(CharSequence delimiter, Iterable<? extends CharSequenc
return stringJoiner.toString();
}

// DO NOT MODIFY THIS METHOD
/*
* The method is only called once to setup the flag DFP_HW_AVAILABLE Return value true - when JIT compiled this method, replaces it with loadconst
* 1 if -Xjit:disableHWAcceleratedStringCaseConv hasn't been supplied false - if still interpreting this method or disabled by VM option
*/
private final static boolean StrCheckHWAvailable() {
return false;
}

// DO NOT MODIFY THIS METHOD
/*
* Return value true - when JIT compiled this method, replaces it with loadconst 1 if -Xjit:disableHWAcceleratedStringCaseConv hasn't been supplied
* false - if still interpreting this method or disabled by VM option
*/
private final static boolean StrHWAvailable() {
return STRING_OPT_IN_HW;
}

// DO NOT CHANGE CONTENTS OF THESE METHODS
private final boolean toUpperHWOptimized(String input) {
return false;
}

private final boolean toLowerHWOptimized(String input) {
return false;
}

static void checkBoundsBeginEnd(int begin, int end, int length) {
if ((begin >= 0) && (begin <= end) && (end <= length)) {
return;
Expand Down Expand Up @@ -3946,7 +3929,6 @@ public String repeat(int count) {
/*[ELSE] Sidecar19-SE*/
// DO NOT CHANGE OR MOVE THIS LINE
// IT MUST BE THE FIRST THING IN THE INITIALIZATION
private static final boolean STRING_OPT_IN_HW = StrCheckHWAvailable();
private static final long serialVersionUID = -6849794470754667710L;

/**
Expand Down Expand Up @@ -4728,22 +4710,6 @@ public String(String string) {
hashCode = string.hashCode;
}

/**
* Creates a new string from the with specified length
*
* @param numChars
* length of new string
*/
private String(int numChars) {
if (enableCompression) {
value = new char[numChars];
count = numChars | uncompressedBit;
} else {
value = new char[numChars];
count = numChars;
}
}

/**
* Creates a string from the contents of a StringBuffer.
*
Expand Down Expand Up @@ -6745,10 +6711,20 @@ public String toLowerCase(Locale locale) {
return this;
}

if (StrHWAvailable() && language == "en") { //$NON-NLS-1$
String output = new String(lengthInternal());
if (toLowerHWOptimized(output))
return output;
if (helpers.supportsIntrinsicCaseConversion() && language == "en") { //$NON-NLS-1$
int sLength = lengthInternal();

if (enableCompression && (null == compressionFlag || count >= 0)) {
char[] output = new char[(sLength + 1) / 2];
if (helpers.toLowerIntrinsicLatin1(value, output, sLength)) {
return new String(output, 0, sLength, true);
}
} else {
char[] output = new char[sLength];
if (helpers.toLowerIntrinsicUTF16(value, output, sLength * 2)) {
return new String(output, 0, sLength, false);
}
}
}

return toLowerCaseCore(language);
Expand Down Expand Up @@ -7070,10 +7046,20 @@ public String toUpperCase(Locale locale) {
return this;
}

if (StrHWAvailable() && language == "en") { //$NON-NLS-1$
String output = new String(lengthInternal());
if (toUpperHWOptimized(output))
return output;
if (helpers.supportsIntrinsicCaseConversion() && language == "en") { //$NON-NLS-1$
int sLength = lengthInternal();

if (enableCompression && (null == compressionFlag || count >= 0)) {
char[] output = new char[(sLength + 1) / 2];
if (helpers.toUpperIntrinsicLatin1(value, output, sLength)){
return new String(output, 0, sLength, true);
}
} else {
char[] output = new char[sLength];
if (helpers.toUpperIntrinsicUTF16(value, output, sLength * 2)){
return new String(output, 0, sLength, false);
}
}
}

return toUpperCaseCore(language);
Expand Down Expand Up @@ -8270,32 +8256,5 @@ public static String join(CharSequence delimiter, Iterable<? extends CharSequenc
return stringJoiner.toString();
}

// DO NOT MODIFY THIS METHOD
/*
* The method is only called once to setup the flag DFP_HW_AVAILABLE Return value true - when JIT compiled this method, replaces it with loadconst
* 1 if -Xjit:disableHWAcceleratedStringCaseConv hasn't been supplied false - if still interpreting this method or disabled by VM option
*/
private final static boolean StrCheckHWAvailable() {
return false;
}

// DO NOT MODIFY THIS METHOD
/*
* Return value true - when JIT compiled this method, replaces it with loadconst 1 if -Xjit:disableHWAcceleratedStringCaseConv hasn't been supplied
* false - if still interpreting this method or disabled by VM option
*/
private final static boolean StrHWAvailable() {
return STRING_OPT_IN_HW;
}

// DO NOT CHANGE CONTENTS OF THESE METHODS
private final boolean toUpperHWOptimized(String input) {
return false;
}

private final boolean toLowerHWOptimized(String input) {
return false;
}

/*[ENDIF] Sidecar19-SE*/
}
13 changes: 5 additions & 8 deletions runtime/compiler/codegen/J9RecognizedMethodsEnum.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1122,15 +1122,12 @@
com_ibm_gpu_Kernel_syncThreads,

// Vectorized toUpper and toLowerCase from j.l.String
// Current only supported on z
// toUpper method for prototype so j.l.S.toUpper doesn't get messed up
java_lang_String_StrHWAvailable,
java_lang_String_toUpperHWOptimizedCompressed,
java_lang_String_toUpperHWOptimizedDecompressed,
java_lang_String_toUpperHWOptimized,
java_lang_String_toLowerHWOptimizedCompressed,
java_lang_String_toLowerHWOptimizedDecompressed,
java_lang_String_toLowerHWOptimized,
com_ibm_jit_JITHelpers_supportsIntrinsicCaseConversion,
com_ibm_jit_JITHelpers_toUpperIntrinsicLatin1,
com_ibm_jit_JITHelpers_toUpperIntrinsicUTF16,
com_ibm_jit_JITHelpers_toLowerIntrinsicLatin1,
com_ibm_jit_JITHelpers_toLowerIntrinsicUTF16,

// SIMD intrinsics built-in methods
com_ibm_simd_VectorBase_vectorHWAvailable,
Expand Down
4 changes: 0 additions & 4 deletions runtime/compiler/control/J9Options.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1922,10 +1922,6 @@ J9::Options::fePreProcess(void * base)
self()->setMaxOnsiteCacheSlotForInstanceOf(4);
#endif

// TODO: Disable string case conversion acceleration temporarily as the code generators need to be updated to
// support the new String object layout
self()->setOption(TR_DisableSIMDStringCaseConv);

// Process the deterministic mode
if (TR::Options::_deterministicMode == -1) // not yet set
{
Expand Down
Loading