Do not deopt when using AOT inline caches
Generating a deopt does not ensure that we will actually capture the new
types; and the danger is that we could be stuck in a loop with "forever"
deoptimizations. For example, if the method never becomes hot again, the
inline cache will not be updated and the AOT code will keep
deoptimizing.
This is a compromise because we will most likely never update the inline
cache (unless there's another reason to deopt). So we might be stuck
with a sub-optimal inline cache. As a TODO, we could be smarter when
capturing inline caches to mitigate this. (e.g. by having different
thresholds for new and old methods).
Delete test 644 which was testing a regression involving deopts which is
no longer applicable.
Bug: 38412648
Test: m test-art-host
Change-Id: Ib84559e84f07ec976feeb3a3120fa486d5bee3bf
diff --git a/compiler/optimizing/inliner.cc b/compiler/optimizing/inliner.cc
index 8674e72..ac5cbf9 100644
--- a/compiler/optimizing/inliner.cc
+++ b/compiler/optimizing/inliner.cc
@@ -470,6 +470,33 @@
return inline_cache;
}
+bool HInliner::UseOnlyPolymorphicInliningWithNoDeopt() {
+ // If we are compiling AOT or OSR, pretend the call using inline caches is polymorphic and
+ // do not generate a deopt.
+ //
+ // For AOT:
+ // Generating a deopt does not ensure that we will actually capture the new types;
+ // and the danger is that we could be stuck in a loop with "forever" deoptimizations.
+ // Take for example the following scenario:
+ // - we capture the inline cache in one run
+ // - the next run, we deoptimize because we miss a type check, but the method
+ // never becomes hot again
+ // In this case, the inline cache will not be updated in the profile and the AOT code
+ // will keep deoptimizing.
+ // Another scenario is if we use profile compilation for a process which is not allowed
+ // to JIT (e.g. system server). If we deoptimize we will run interpreted code for the
+ // rest of the lifetime.
+ // TODO(calin):
+ // This is a compromise because we will most likely never update the inline cache
+ // in the profile (unless there's another reason to deopt). So we might be stuck with
+ // a sub-optimal inline cache.
+ // We could be smarter when capturing inline caches to mitigate this.
+ // (e.g. by having different thresholds for new and old methods).
+ //
+ // For OSR:
+ // We may come from the interpreter and it may have seen different receiver types.
+ return Runtime::Current()->IsAotCompiler() || outermost_graph_->IsCompilingOsr();
+}
bool HInliner::TryInlineFromInlineCache(const DexFile& caller_dex_file,
HInvoke* invoke_instruction,
ArtMethod* resolved_method)
@@ -503,9 +530,7 @@
case kInlineCacheMonomorphic: {
MaybeRecordStat(kMonomorphicCall);
- if (outermost_graph_->IsCompilingOsr()) {
- // If we are compiling OSR, we pretend this call is polymorphic, as we may come from the
- // interpreter and it may have seen different receiver types.
+ if (UseOnlyPolymorphicInliningWithNoDeopt()) {
return TryInlinePolymorphicCall(invoke_instruction, resolved_method, inline_cache);
} else {
return TryInlineMonomorphicCall(invoke_instruction, resolved_method, inline_cache);
@@ -926,14 +951,11 @@
// If we have inlined all targets before, and this receiver is the last seen,
// we deoptimize instead of keeping the original invoke instruction.
- bool deoptimize = all_targets_inlined &&
+ bool deoptimize = !UseOnlyPolymorphicInliningWithNoDeopt() &&
+ all_targets_inlined &&
(i != InlineCache::kIndividualCacheSize - 1) &&
(classes->Get(i + 1) == nullptr);
- if (outermost_graph_->IsCompilingOsr()) {
- // We do not support HDeoptimize in OSR methods.
- deoptimize = false;
- }
HInstruction* compare = AddTypeGuard(receiver,
cursor,
bb_cursor,
diff --git a/compiler/optimizing/inliner.h b/compiler/optimizing/inliner.h
index 9e4685c..67476b6 100644
--- a/compiler/optimizing/inliner.h
+++ b/compiler/optimizing/inliner.h
@@ -180,6 +180,9 @@
Handle<mirror::ObjectArray<mirror::Class>> classes)
REQUIRES_SHARED(Locks::mutator_lock_);
+ // Returns whether or not we should use only polymorphic inlining with no deoptimizations.
+ bool UseOnlyPolymorphicInliningWithNoDeopt();
+
// Try CHA-based devirtualization to change virtual method calls into
// direct calls.
// Returns the actual method that resolved_method can be devirtualized to.
diff --git a/test/638-checker-inline-caches/src/Main.java b/test/638-checker-inline-caches/src/Main.java
index 680bd14..f104e6a 100644
--- a/test/638-checker-inline-caches/src/Main.java
+++ b/test/638-checker-inline-caches/src/Main.java
@@ -36,16 +36,17 @@
/// CHECK: InvokeVirtual method_name:Super.getValue
/// CHECK-START: int Main.inlineMonomorphicSubA(Super) inliner (after)
- /// CHECK-NOT: InvokeVirtual method_name:Super.getValue
-
- /// CHECK-START: int Main.inlineMonomorphicSubA(Super) inliner (after)
/// CHECK: <<SubARet:i\d+>> IntConstant 42
/// CHECK: <<Obj:l\d+>> NullCheck
/// CHECK: <<ObjClass:l\d+>> InstanceFieldGet [<<Obj>>] field_name:java.lang.Object.shadow$_klass_
/// CHECK: <<InlineClass:l\d+>> LoadClass class_name:SubA
/// CHECK: <<Test:z\d+>> NotEqual [<<InlineClass>>,<<ObjClass>>]
- /// CHECK: Deoptimize [<<Test>>,<<Obj>>]
- /// CHECK: Return [<<SubARet>>]
+ /// CHECK: <<DefaultRet:i\d+>> InvokeVirtual [<<Obj>>] method_name:Super.getValue
+
+ /// CHECK: <<Ret:i\d+>> Phi [<<SubARet>>,<<DefaultRet>>]
+ /// CHECK: Return [<<Ret>>]
+
+ /// CHECK-NOT: Deoptimize
public static int inlineMonomorphicSubA(Super a) {
return a.getValue();
}
@@ -53,27 +54,27 @@
/// CHECK-START: int Main.inlinePolymophicSubASubB(Super) inliner (before)
/// CHECK: InvokeVirtual method_name:Super.getValue
- /// CHECK-START: int Main.inlinePolymophicSubASubB(Super) inliner (after)
- /// CHECK-NOT: InvokeVirtual method_name:Super.getValue
-
// Note that the order in which the types are added to the inline cache in the profile matters.
/// CHECK-START: int Main.inlinePolymophicSubASubB(Super) inliner (after)
/// CHECK-DAG: <<SubARet:i\d+>> IntConstant 42
/// CHECK-DAG: <<SubBRet:i\d+>> IntConstant 38
- /// CHECK: <<Obj:l\d+>> NullCheck
- /// CHECK: <<ObjClassSubA:l\d+>> InstanceFieldGet [<<Obj>>] field_name:java.lang.Object.shadow$_klass_
- /// CHECK: <<InlineClassSubA:l\d+>> LoadClass class_name:SubA
- /// CHECK: <<TestSubA:z\d+>> NotEqual [<<InlineClassSubA>>,<<ObjClassSubA>>]
- /// CHECK: If [<<TestSubA>>]
+ /// CHECK-DAG: <<Obj:l\d+>> NullCheck
+ /// CHECK-DAG: <<ObjClassSubA:l\d+>> InstanceFieldGet [<<Obj>>] field_name:java.lang.Object.shadow$_klass_
+ /// CHECK-DAG: <<InlineClassSubA:l\d+>> LoadClass class_name:SubA
+ /// CHECK-DAG: <<TestSubA:z\d+>> NotEqual [<<InlineClassSubA>>,<<ObjClassSubA>>]
+ /// CHECK-DAG: If [<<TestSubA>>]
- /// CHECK: <<ObjClassSubB:l\d+>> InstanceFieldGet field_name:java.lang.Object.shadow$_klass_
- /// CHECK: <<InlineClassSubB:l\d+>> LoadClass class_name:SubB
- /// CHECK: <<TestSubB:z\d+>> NotEqual [<<InlineClassSubB>>,<<ObjClassSubB>>]
- /// CHECK: Deoptimize [<<TestSubB>>,<<Obj>>]
+ /// CHECK-DAG: <<ObjClassSubB:l\d+>> InstanceFieldGet field_name:java.lang.Object.shadow$_klass_
+ /// CHECK-DAG: <<InlineClassSubB:l\d+>> LoadClass class_name:SubB
+ /// CHECK-DAG: <<TestSubB:z\d+>> NotEqual [<<InlineClassSubB>>,<<ObjClassSubB>>]
+ /// CHECK-DAG: <<DefaultRet:i\d+>> InvokeVirtual [<<Obj>>] method_name:Super.getValue
- /// CHECK: <<Ret:i\d+>> Phi [<<SubARet>>,<<SubBRet>>]
- /// CHECK: Return [<<Ret>>]
+ /// CHECK-DAG: <<FirstMerge:i\d+>> Phi [<<SubBRet>>,<<DefaultRet>>]
+ /// CHECK-DAG: <<Ret:i\d+>> Phi [<<SubARet>>,<<FirstMerge>>]
+ /// CHECK-DAG: Return [<<Ret>>]
+
+ /// CHECK-NOT: Deoptimize
public static int inlinePolymophicSubASubB(Super a) {
return a.getValue();
}
@@ -81,27 +82,27 @@
/// CHECK-START: int Main.inlinePolymophicCrossDexSubASubC(Super) inliner (before)
/// CHECK: InvokeVirtual method_name:Super.getValue
- /// CHECK-START: int Main.inlinePolymophicCrossDexSubASubC(Super) inliner (after)
- /// CHECK-NOT: InvokeVirtual method_name:Super.getValue
-
// Note that the order in which the types are added to the inline cache in the profile matters.
/// CHECK-START: int Main.inlinePolymophicCrossDexSubASubC(Super) inliner (after)
/// CHECK-DAG: <<SubARet:i\d+>> IntConstant 42
/// CHECK-DAG: <<SubCRet:i\d+>> IntConstant 24
- /// CHECK: <<Obj:l\d+>> NullCheck
- /// CHECK: <<ObjClassSubA:l\d+>> InstanceFieldGet [<<Obj>>] field_name:java.lang.Object.shadow$_klass_
- /// CHECK: <<InlineClassSubA:l\d+>> LoadClass class_name:SubA
- /// CHECK: <<TestSubA:z\d+>> NotEqual [<<InlineClassSubA>>,<<ObjClassSubA>>]
- /// CHECK: If [<<TestSubA>>]
+ /// CHECK-DAG: <<Obj:l\d+>> NullCheck
+ /// CHECK-DAG: <<ObjClassSubA:l\d+>> InstanceFieldGet [<<Obj>>] field_name:java.lang.Object.shadow$_klass_
+ /// CHECK-DAG: <<InlineClassSubA:l\d+>> LoadClass class_name:SubA
+ /// CHECK-DAG: <<TestSubA:z\d+>> NotEqual [<<InlineClassSubA>>,<<ObjClassSubA>>]
+ /// CHECK-DAG: If [<<TestSubA>>]
- /// CHECK: <<ObjClassSubC:l\d+>> InstanceFieldGet field_name:java.lang.Object.shadow$_klass_
- /// CHECK: <<InlineClassSubC:l\d+>> LoadClass class_name:SubC
- /// CHECK: <<TestSubC:z\d+>> NotEqual [<<InlineClassSubC>>,<<ObjClassSubC>>]
- /// CHECK: Deoptimize [<<TestSubC>>,<<Obj>>]
+ /// CHECK-DAG: <<ObjClassSubC:l\d+>> InstanceFieldGet field_name:java.lang.Object.shadow$_klass_
+ /// CHECK-DAG: <<InlineClassSubC:l\d+>> LoadClass class_name:SubC
+ /// CHECK-DAG: <<TestSubC:z\d+>> NotEqual [<<InlineClassSubC>>,<<ObjClassSubC>>]
+ /// CHECK-DAG: <<DefaultRet:i\d+>> InvokeVirtual [<<Obj>>] method_name:Super.getValue
- /// CHECK: <<Ret:i\d+>> Phi [<<SubARet>>,<<SubCRet>>]
- /// CHECK: Return [<<Ret>>]
+ /// CHECK-DAG: <<FirstMerge:i\d+>> Phi [<<SubCRet>>,<<DefaultRet>>]
+ /// CHECK-DAG: <<Ret:i\d+>> Phi [<<SubARet>>,<<FirstMerge>>]
+ /// CHECK-DAG: Return [<<Ret>>]
+
+ /// CHECK-NOT: Deoptimize
public static int inlinePolymophicCrossDexSubASubC(Super a) {
return a.getValue();
}
diff --git a/test/644-checker-deopt/expected.txt b/test/644-checker-deopt/expected.txt
deleted file mode 100644
index e69de29..0000000
--- a/test/644-checker-deopt/expected.txt
+++ /dev/null
diff --git a/test/644-checker-deopt/info.txt b/test/644-checker-deopt/info.txt
deleted file mode 100644
index c5fb12c..0000000
--- a/test/644-checker-deopt/info.txt
+++ /dev/null
@@ -1,2 +0,0 @@
-Regression test for making sure HDeoptimize is executed before
-the code it should have prevented executing.
diff --git a/test/644-checker-deopt/profile b/test/644-checker-deopt/profile
deleted file mode 100644
index cb261cc..0000000
--- a/test/644-checker-deopt/profile
+++ /dev/null
@@ -1,2 +0,0 @@
-LMain;->inlineMonomorphic(LMain;)I+LMain;
-LMain;->inlinePolymorphic(LMain;)I+LMain;,LSubMain;
diff --git a/test/644-checker-deopt/run b/test/644-checker-deopt/run
deleted file mode 100644
index 146e180..0000000
--- a/test/644-checker-deopt/run
+++ /dev/null
@@ -1,17 +0,0 @@
-#!/bin/bash
-#
-# Copyright (C) 2017 The Android Open Source Project
-#
-# Licensed under the Apache License, Version 2.0 (the "License");
-# you may not use this file except in compliance with the License.
-# You may obtain a copy of the License at
-#
-# http://www.apache.org/licenses/LICENSE-2.0
-#
-# Unless required by applicable law or agreed to in writing, software
-# distributed under the License is distributed on an "AS IS" BASIS,
-# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-# See the License for the specific language governing permissions and
-# limitations under the License.
-
-exec ${RUN} $@ --profile -Xcompiler-option --compiler-filter=speed-profile
diff --git a/test/644-checker-deopt/src/Main.java b/test/644-checker-deopt/src/Main.java
deleted file mode 100644
index 17c80a6..0000000
--- a/test/644-checker-deopt/src/Main.java
+++ /dev/null
@@ -1,74 +0,0 @@
-/*
- * Copyright (C) 2017 The Android Open Source Project
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-public class Main {
-
- /// CHECK-START: int Main.inlineMonomorphic(Main) inliner (before)
- /// CHECK: InvokeVirtual method_name:Main.getValue
-
- /// CHECK-START: int Main.inlineMonomorphic(Main) inliner (after)
- /// CHECK-NOT: InvokeVirtual method_name:Main.getValue
-
- /// CHECK-START: int Main.inlineMonomorphic(Main) licm (before)
- /// CHECK: <<Deopt:l\d+>> Deoptimize
- /// CHECK: InstanceFieldGet [<<Deopt>>] field_name:Main.value
-
- /// CHECK-START: int Main.inlineMonomorphic(Main) licm (after)
- /// CHECK: <<Deopt:l\d+>> Deoptimize
- /// CHECK: InstanceFieldGet [<<Deopt>>] field_name:Main.value
-
- public static int inlineMonomorphic(Main a) {
- if (a == null) {
- return 42;
- }
- int i = 0;
- while (i < 100) {
- i += a.getValue();
- }
- return i;
- }
-
- /// CHECK-START: int Main.inlinePolymorphic(Main) inliner (before)
- /// CHECK: InvokeVirtual method_name:Main.getValue
-
- /// CHECK-START: int Main.inlinePolymorphic(Main) inliner (after)
- /// CHECK-NOT: InvokeVirtual method_name:Main.getValue
-
- /// CHECK-START: int Main.inlineMonomorphic(Main) licm (before)
- /// CHECK: <<Deopt:l\d+>> Deoptimize
- /// CHECK: InstanceFieldGet [<<Deopt>>] field_name:Main.value
-
- /// CHECK-START: int Main.inlineMonomorphic(Main) licm (after)
- /// CHECK: <<Deopt:l\d+>> Deoptimize
- /// CHECK: InstanceFieldGet [<<Deopt>>] field_name:Main.value
- public static int inlinePolymorphic(Main a) {
- return a.getValue();
- }
-
- public int getValue() {
- return value;
- }
-
- public static void main(String[] args) {
- inlineMonomorphic(new Main());
- }
-
- int value = 1;
-}
-
-// Add a subclass of 'Main' to write the polymorphic inline cache in the profile.
-class SubMain extends Main {
-}
diff --git a/test/knownfailures.json b/test/knownfailures.json
index 4b44df7..168b673 100644
--- a/test/knownfailures.json
+++ b/test/knownfailures.json
@@ -508,7 +508,6 @@
"640-checker-short-simd",
"641-checker-arraycopy",
"643-checker-bogus-ic",
- "644-checker-deopt",
"645-checker-abs-simd",
"706-checker-scheduler"],
"description": ["Checker tests are not compatible with jvmti."],