8138651: -XX:DisableIntrinsic matches intrinsics overly eagerly
Summary: Improve parsing of DisableIntrinsic flag.
Reviewed-by: kvn, shade, neliasso
--- a/hotspot/src/share/vm/compiler/compilerDirectives.cpp Fri Oct 23 10:57:41 2015 +0200
+++ b/hotspot/src/share/vm/compiler/compilerDirectives.cpp Thu Oct 29 09:24:00 2015 +0100
@@ -175,12 +175,38 @@
return NULL;
}
+// In the list of disabled intrinsics, the ID of the disabled intrinsics can separated:
+// - by ',' (if -XX:DisableIntrinsic is used once when invoking the VM) or
+// - by '\n' (if -XX:DisableIntrinsic is used multiple times when invoking the VM) or
+// - by ' ' (if DisableIntrinsic is used on a per-method level, e.g., with CompileCommand).
+//
+// To simplify the processing of the list, the canonicalize_disableintrinsic() method
+// returns a new copy of the list in which '\n' and ' ' is replaced with ','.
+ccstrlist DirectiveSet::canonicalize_disableintrinsic(ccstrlist option_value) {
+ char* canonicalized_list = NEW_C_HEAP_ARRAY(char, strlen(option_value) + 1, mtCompiler);
+ int i = 0;
+ char current;
+ while ((current = option_value[i]) != '\0') {
+ if (current == '\n' || current == ' ') {
+ canonicalized_list[i] = ',';
+ } else {
+ canonicalized_list[i] = current;
+ }
+ i++;
+ }
+ canonicalized_list[i] = '\0';
+ return canonicalized_list;
+}
+
DirectiveSet::DirectiveSet(CompilerDirectives* d) :_inlinematchers(NULL), _directive(d) {
#define init_defaults_definition(name, type, dvalue, compiler) this->name##Option = dvalue;
compilerdirectives_common_flags(init_defaults_definition)
compilerdirectives_c2_flags(init_defaults_definition)
compilerdirectives_c1_flags(init_defaults_definition)
memset(_modified, 0, sizeof _modified);
+
+ // Canonicalize DisableIntrinsic to contain only ',' as a separator.
+ this->DisableIntrinsicOption = canonicalize_disableintrinsic(DisableIntrinsic);
}
DirectiveSet::~DirectiveSet() {
@@ -192,12 +218,11 @@
tmp = next;
}
- // Free if modified, otherwise it just points to the global vm flag value
- // or to the Compile command option
- if (_modified[DisableIntrinsicIndex]) {
- assert(this->DisableIntrinsicOption != NULL, "");
- FREE_C_HEAP_ARRAY(char, (void *)this->DisableIntrinsicOption);
- }
+ // When constructed, DirectiveSet canonicalizes the DisableIntrinsic flag
+ // into a new string. Therefore, that string is deallocated when
+ // the DirectiveSet is destroyed.
+ assert(this->DisableIntrinsicOption != NULL, "");
+ FREE_C_HEAP_ARRAY(char, (void *)this->DisableIntrinsicOption);
}
// Backward compatibility for CompileCommands
@@ -253,6 +278,14 @@
compilerdirectives_c2_flags(init_default_cc)
compilerdirectives_c1_flags(init_default_cc)
+ // Canonicalize DisableIntrinsic to contain only ',' as a separator.
+ ccstrlist option_value;
+ if (!_modified[DisableIntrinsicIndex] &&
+ CompilerOracle::has_option_value(method, "DisableIntrinsic", option_value)) {
+ set->DisableIntrinsicOption = canonicalize_disableintrinsic(option_value);
+ }
+
+
if (!changed) {
// We didn't actually update anything, discard.
delete set;
@@ -352,8 +385,26 @@
vmIntrinsics::ID id = method->intrinsic_id();
assert(id != vmIntrinsics::_none, "must be a VM intrinsic");
- ccstr disable_intr = DisableIntrinsicOption;
- return ((disable_intr != '\0') && strstr(disable_intr, vmIntrinsics::name_at(id)) != NULL);
+ ResourceMark rm;
+
+ // Create a copy of the string that contains the list of disabled
+ // intrinsics. The copy is created because the string
+ // will be modified by strtok(). Then, the list is tokenized with
+ // ',' as a separator.
+ size_t length = strlen(DisableIntrinsicOption);
+ char* local_list = NEW_RESOURCE_ARRAY(char, length + 1);
+ strncpy(local_list, DisableIntrinsicOption, length + 1);
+
+ char* token = strtok(local_list, ",");
+ while (token != NULL) {
+ if (strcmp(token, vmIntrinsics::name_at(id)) == 0) {
+ return true;
+ } else {
+ token = strtok(NULL, ",");
+ }
+ }
+
+ return false;
}
DirectiveSet* DirectiveSet::clone(DirectiveSet const* src) {
@@ -371,15 +422,13 @@
compilerdirectives_c2_flags(copy_members_definition)
compilerdirectives_c1_flags(copy_members_definition)
- // Must duplicate ccstr option if it was modified, otherwise it is global.
- if (src->_modified[DisableIntrinsicIndex]) {
- assert(src->DisableIntrinsicOption != NULL, "");
- size_t len = strlen(src->DisableIntrinsicOption) + 1;
- char* s = NEW_C_HEAP_ARRAY(char, len, mtCompiler);
- strncpy(s, src->DisableIntrinsicOption, len);
- assert(s[len-1] == '\0', "");
- set->DisableIntrinsicOption = s;
- }
+ // Create a local copy of the DisableIntrinsicOption.
+ assert(src->DisableIntrinsicOption != NULL, "");
+ size_t len = strlen(src->DisableIntrinsicOption) + 1;
+ char* s = NEW_C_HEAP_ARRAY(char, len, mtCompiler);
+ strncpy(s, src->DisableIntrinsicOption, len);
+ assert(s[len-1] == '\0', "");
+ set->DisableIntrinsicOption = s;
return set;
}
--- a/hotspot/src/share/vm/compiler/compilerDirectives.hpp Fri Oct 23 10:57:41 2015 +0200
+++ b/hotspot/src/share/vm/compiler/compilerDirectives.hpp Thu Oct 29 09:24:00 2015 +0100
@@ -47,7 +47,7 @@
cflags(DumpReplay, bool, false, DumpReplay) \
cflags(DumpInline, bool, false, DumpInline) \
cflags(CompilerDirectivesIgnoreCompileCommands, bool, CompilerDirectivesIgnoreCompileCommands, X) \
- cflags(DisableIntrinsic, ccstr, DisableIntrinsic, DisableIntrinsic)
+ cflags(DisableIntrinsic, ccstrlist, DisableIntrinsic, DisableIntrinsic)
#ifdef COMPILER1
#define compilerdirectives_c1_flags(cflags)
@@ -100,6 +100,8 @@
InlineMatcher* _inlinematchers;
CompilerDirectives* _directive;
+ static ccstrlist canonicalize_disableintrinsic(ccstrlist option_value);
+
public:
DirectiveSet(CompilerDirectives* directive);
~DirectiveSet();
@@ -141,6 +143,7 @@
void print_bool(outputStream* st, ccstr n, bool v, bool mod) { if (mod) { st->print("%s:%s ", n, v ? "true" : "false"); } }
void print_double(outputStream* st, ccstr n, double v, bool mod) { if (mod) { st->print("%s:%f ", n, v); } }
void print_ccstr(outputStream* st, ccstr n, ccstr v, bool mod) { if (mod) { st->print("%s:%s ", n, v); } }
+ void print_ccstrlist(outputStream* st, ccstr n, ccstr v, bool mod) { print_ccstr(st, n, v, mod); }
void print(outputStream* st) {
print_inline(st);
--- a/hotspot/src/share/vm/compiler/directivesParser.cpp Fri Oct 23 10:57:41 2015 +0200
+++ b/hotspot/src/share/vm/compiler/directivesParser.cpp Thu Oct 29 09:24:00 2015 +0100
@@ -288,7 +288,7 @@
break;
case JSON_STRING:
- if (option_key->flag_type != ccstrFlag) {
+ if (option_key->flag_type != ccstrFlag && option_key->flag_type != ccstrlistFlag) {
error(VALUE_ERROR, "Cannot use string value for a %s flag", flag_type_names[option_key->flag_type]);
return false;
} else {
--- a/hotspot/src/share/vm/compiler/directivesParser.hpp Fri Oct 23 10:57:41 2015 +0200
+++ b/hotspot/src/share/vm/compiler/directivesParser.hpp Thu Oct 29 09:24:00 2015 +0100
@@ -33,6 +33,7 @@
intxFlag,
doubleFlag,
ccstrFlag,
+ ccstrlistFlag,
UnknownFlagType
};
@@ -41,6 +42,7 @@
"int",
"double",
"string",
+ "string list",
"unknown"
};
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/hotspot/test/compiler/intrinsics/IntrinsicDisabledTest.java Thu Oct 29 09:24:00 2015 +0100
@@ -0,0 +1,211 @@
+/*
+ * Copyright (c) 2015, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * @test
+ * @bug 8138651
+ * @library /testlibrary /../../test/lib
+ * @build IntrinsicDisabledTest
+ * @run main ClassFileInstaller sun.hotspot.WhiteBox
+ * sun.hotspot.WhiteBox$WhiteBoxPermission
+ * @run main/othervm -Xbootclasspath/a:.
+ * -XX:+UnlockDiagnosticVMOptions
+ * -XX:+WhiteBoxAPI
+ * -XX:DisableIntrinsic=_putCharVolatile,_putInt
+ * -XX:DisableIntrinsic=_putIntVolatile
+ * -XX:CompileCommand=option,sun.misc.Unsafe::putChar,ccstrlist,DisableIntrinsic,_getCharVolatile,_getInt
+ * -XX:CompileCommand=option,sun.misc.Unsafe::putCharVolatile,ccstrlist,DisableIntrinsic,_getIntVolatile
+ * IntrinsicDisabledTest
+ */
+
+import java.lang.reflect.Executable;
+import java.util.Objects;
+
+import sun.hotspot.WhiteBox;
+
+import jdk.test.lib.Platform;
+
+public class IntrinsicDisabledTest {
+
+ private static final WhiteBox wb = WhiteBox.getWhiteBox();
+
+ /* Compilation level corresponding to C1. */
+ private static final int COMP_LEVEL_SIMPLE = 1;
+
+ /* Compilation level corresponding to C2. */
+ private static final int COMP_LEVEL_FULL_OPTIMIZATION = 4;
+
+ /* Determine if tiered compilation is enabled. */
+ private static boolean isTieredCompilationEnabled() {
+ return Boolean.valueOf(Objects.toString(wb.getVMFlag("TieredCompilation")));
+ }
+
+ /* This test uses several methods from sun.misc.Unsafe. The method
+ * getMethod() returns a different Executable for each different
+ * combination of its input parameters. There are eight possible
+ * combinations, getMethod can return an Executable representing
+ * the following methods: putChar, putCharVolatile, getChar,
+ * getCharVolatile, putInt, putIntVolatile, getInt,
+ * getIntVolatile. These methods were selected because they can
+ * be intrinsified by both the C1 and the C2 compiler.
+ */
+ static Executable getMethod(boolean isChar, boolean isPut, boolean isVolatile) throws RuntimeException {
+ Executable aMethod;
+ String methodTypeName = isChar ? "Char" : "Int";
+
+ try {
+ Class aClass = Class.forName("sun.misc.Unsafe");
+ if (isPut) {
+ aMethod = aClass.getDeclaredMethod("put" + methodTypeName + (isVolatile ? "Volatile" : ""),
+ Object.class,
+ long.class,
+ isChar ? char.class : int.class);
+ } else {
+ aMethod = aClass.getDeclaredMethod("get" + methodTypeName + (isVolatile ? "Volatile" : ""),
+ Object.class,
+ long.class);
+ }
+ } catch (NoSuchMethodException e) {
+ throw new RuntimeException("Test bug, method is unavailable. " + e);
+ } catch (ClassNotFoundException e) {
+ throw new RuntimeException("Test bug, class is unavailable. " + e);
+ }
+
+ return aMethod;
+ }
+
+ public static void test(int compLevel) {
+
+ Executable putChar = getMethod(true, /*isPut =*/ true, /*isVolatile = */ false);
+ Executable getChar = getMethod(true, /*isPut =*/ false, /*isVolatile = */ false);
+ Executable putCharVolatile = getMethod(true, /*isPut =*/ true, /*isVolatile = */ true);
+ Executable getCharVolatile = getMethod(true, /*isPut =*/ false, /*isVolatile = */ true);
+ Executable putInt = getMethod(false, /*isPut =*/ true, /*isVolatile = */ false);
+ Executable getInt = getMethod(false, /*isPut =*/ false, /*isVolatile = */ false);
+ Executable putIntVolatile = getMethod(false, /*isPut =*/ true, /*isVolatile = */ true);
+ Executable getIntVolatile = getMethod(false, /*isPut =*/ false, /*isVolatile = */ true);
+
+ /* Test if globally disabling intrinsics works. */
+ if (!wb.isIntrinsicAvailable(putChar, compLevel)) {
+ throw new RuntimeException("Intrinsic for [" + putChar.toGenericString() +
+ "] is not available globally although it should be.");
+ }
+
+ if (wb.isIntrinsicAvailable(putCharVolatile, compLevel)) {
+ throw new RuntimeException("Intrinsic for [" + putCharVolatile.toGenericString() +
+ "] is available globally although it should not be.");
+ }
+
+ if (wb.isIntrinsicAvailable(putInt, compLevel)) {
+ throw new RuntimeException("Intrinsic for [" + putInt.toGenericString() +
+ "] is available globally although it should not be.");
+ }
+
+ if (wb.isIntrinsicAvailable(putIntVolatile, compLevel)) {
+ throw new RuntimeException("Intrinsic for [" + putIntVolatile.toGenericString() +
+ "] is available globally although it should not be.");
+ }
+
+ /* Test if disabling intrinsics on a per-method level
+ * works. The method for which intrinsics are
+ * disabled (the compilation context) is putChar. */
+ if (!wb.isIntrinsicAvailable(getChar, putChar, compLevel)) {
+ throw new RuntimeException("Intrinsic for [" + getChar.toGenericString() +
+ "] is not available for intrinsification in [" +
+ putChar.toGenericString() + "] although it should be.");
+ }
+
+ if (wb.isIntrinsicAvailable(getCharVolatile, putChar, compLevel)) {
+ throw new RuntimeException("Intrinsic for [" + getCharVolatile.toGenericString() +
+ "] is available for intrinsification in [" +
+ putChar.toGenericString() + "] although it should not be.");
+ }
+
+ if (wb.isIntrinsicAvailable(getInt, putChar, compLevel)) {
+ throw new RuntimeException("Intrinsic for [" + getInt.toGenericString() +
+ "] is available for intrinsification in [" +
+ putChar.toGenericString() + "] although it should not be.");
+ }
+
+ if (wb.isIntrinsicAvailable(getIntVolatile, putCharVolatile, compLevel)) {
+ throw new RuntimeException("Intrinsic for [" + getIntVolatile.toGenericString() +
+ "] is available for intrinsification in [" +
+ putCharVolatile.toGenericString() + "] although it should not be.");
+ }
+
+ /* Test if disabling intrinsics on a per-method level
+ * leaves those intrinsics enabled globally. */
+ if (!wb.isIntrinsicAvailable(getCharVolatile, compLevel)) {
+ throw new RuntimeException("Intrinsic for [" + getCharVolatile.toGenericString() +
+ "] is not available globally although it should be.");
+ }
+
+ if (!wb.isIntrinsicAvailable(getInt, compLevel)) {
+ throw new RuntimeException("Intrinsic for [" + getInt.toGenericString() +
+ "] is not available globally although it should be.");
+ }
+
+
+ if (!wb.isIntrinsicAvailable(getIntVolatile, compLevel)) {
+ throw new RuntimeException("Intrinsic for [" + getIntVolatile.toGenericString() +
+ "] is not available globally although it should be.");
+ }
+
+ /* Test if disabling an intrinsic globally disables it on a
+ * per-method level as well. */
+ if (!wb.isIntrinsicAvailable(putChar, getChar, compLevel)) {
+ throw new RuntimeException("Intrinsic for [" + putChar.toGenericString() +
+ "] is not available for intrinsification in [" +
+ getChar.toGenericString() + "] although it should be.");
+ }
+
+ if (wb.isIntrinsicAvailable(putCharVolatile, getChar, compLevel)) {
+ throw new RuntimeException("Intrinsic for [" + putCharVolatile.toGenericString() +
+ "] is available for intrinsification in [" +
+ getChar.toGenericString() + "] although it should not be.");
+ }
+
+ if (wb.isIntrinsicAvailable(putInt, getChar, compLevel)) {
+ throw new RuntimeException("Intrinsic for [" + putInt.toGenericString() +
+ "] is available for intrinsification in [" +
+ getChar.toGenericString() + "] although it should not be.");
+ }
+
+ if (wb.isIntrinsicAvailable(putIntVolatile, getChar, compLevel)) {
+ throw new RuntimeException("Intrinsic for [" + putIntVolatile.toGenericString() +
+ "] is available for intrinsification in [" +
+ getChar.toGenericString() + "] although it should not be.");
+ }
+ }
+
+ public static void main(String args[]) {
+ if (Platform.isServer()) {
+ if (isTieredCompilationEnabled()) {
+ test(COMP_LEVEL_SIMPLE);
+ }
+ test(COMP_LEVEL_FULL_OPTIMIZATION);
+ } else {
+ test(COMP_LEVEL_SIMPLE);
+ }
+ }
+}