Fix inlining loops in OSR mode.
When compiling a method in OSR mode and the method does not
contain a loop (arguably, a very odd case) but we inline
another method with a loop and then the final DCE re-runs
the loop identification, the inlined loop would previously
be marked as irreducible. However, the SSA liveness analysis
expects irreducible loop to have extra loop Phis which were
already eliminated from the loop before the inner graph was
inlined to the outer graph, so we would fail a DCHECK().
We fix this by not marking inlined loops as irreducible when
compiling in OSR mode.
Bug: 28210356
(cherry picked from commit fd66c50d64c38e40bafde83b4872e27bbff7546d)
Change-Id: I149273b766d1c713c571baad6033c5f70e6dd960
diff --git a/compiler/optimizing/nodes.cc b/compiler/optimizing/nodes.cc
index 1afa36a..5091c7b 100644
--- a/compiler/optimizing/nodes.cc
+++ b/compiler/optimizing/nodes.cc
@@ -618,7 +618,24 @@
}
}
- if (is_irreducible_loop || graph->IsCompilingOsr()) {
+ if (!is_irreducible_loop && graph->IsCompilingOsr()) {
+ // When compiling in OSR mode, all loops in the compiled method may be entered
+ // from the interpreter. We treat this OSR entry point just like an extra entry
+ // to an irreducible loop, so we need to mark the method's loops as irreducible.
+ // This does not apply to inlined loops which do not act as OSR entry points.
+ if (suspend_check_ == nullptr) {
+ // Just building the graph in OSR mode, this loop is not inlined. We never build an
+ // inner graph in OSR mode as we can do OSR transition only from the outer method.
+ is_irreducible_loop = true;
+ } else {
+ // Look at the suspend check's environment to determine if the loop was inlined.
+ DCHECK(suspend_check_->HasEnvironment());
+ if (!suspend_check_->GetEnvironment()->IsFromInlinedInvoke()) {
+ is_irreducible_loop = true;
+ }
+ }
+ }
+ if (is_irreducible_loop) {
irreducible_ = true;
graph->SetHasIrreducibleLoops(true);
}
diff --git a/compiler/optimizing/ssa_liveness_analysis.cc b/compiler/optimizing/ssa_liveness_analysis.cc
index 83e9dac..134c1f6 100644
--- a/compiler/optimizing/ssa_liveness_analysis.cc
+++ b/compiler/optimizing/ssa_liveness_analysis.cc
@@ -318,7 +318,7 @@
for (uint32_t idx : live_in->Indexes()) {
HInstruction* instruction = GetInstructionFromSsaIndex(idx);
DCHECK(instruction->GetBlock()->IsEntryBlock()) << instruction->DebugName();
- DCHECK(!instruction->IsParameterValue()) << instruction->DebugName();
+ DCHECK(!instruction->IsParameterValue());
DCHECK(instruction->IsCurrentMethod() || instruction->IsConstant())
<< instruction->DebugName();
}
diff --git a/test/570-checker-osr/expected.txt b/test/570-checker-osr/expected.txt
index 25fb220..65447be 100644
--- a/test/570-checker-osr/expected.txt
+++ b/test/570-checker-osr/expected.txt
@@ -3,3 +3,4 @@
200000
300000
400000
+b28210356 passed.
diff --git a/test/570-checker-osr/osr.cc b/test/570-checker-osr/osr.cc
index 09e97ea..ac2e0ce 100644
--- a/test/570-checker-osr/osr.cc
+++ b/test/570-checker-osr/osr.cc
@@ -20,15 +20,17 @@
#include "jit/profiling_info.h"
#include "oat_quick_method_header.h"
#include "scoped_thread_state_change.h"
+#include "ScopedUtfChars.h"
#include "stack_map.h"
namespace art {
class OsrVisitor : public StackVisitor {
public:
- explicit OsrVisitor(Thread* thread)
+ explicit OsrVisitor(Thread* thread, const char* method_name)
SHARED_REQUIRES(Locks::mutator_lock_)
: StackVisitor(thread, nullptr, StackVisitor::StackWalkKind::kIncludeInlinedFrames),
+ method_name_(method_name),
in_osr_method_(false),
in_interpreter_(false) {}
@@ -36,13 +38,7 @@
ArtMethod* m = GetMethod();
std::string m_name(m->GetName());
- if ((m_name.compare("$noinline$returnInt") == 0) ||
- (m_name.compare("$noinline$returnFloat") == 0) ||
- (m_name.compare("$noinline$returnDouble") == 0) ||
- (m_name.compare("$noinline$returnLong") == 0) ||
- (m_name.compare("$noinline$deopt") == 0) ||
- (m_name.compare("$noinline$inlineCache") == 0) ||
- (m_name.compare("$noinline$stackOverflow") == 0)) {
+ if (m_name.compare(method_name_) == 0) {
const OatQuickMethodHeader* header =
Runtime::Current()->GetJit()->GetCodeCache()->LookupOsrMethodHeader(m);
if (header != nullptr && header == GetCurrentOatQuickMethodHeader()) {
@@ -55,29 +51,38 @@
return true;
}
+ const char* const method_name_;
bool in_osr_method_;
bool in_interpreter_;
};
-extern "C" JNIEXPORT jboolean JNICALL Java_Main_ensureInOsrCode(JNIEnv*, jclass) {
+extern "C" JNIEXPORT jboolean JNICALL Java_Main_isInOsrCode(JNIEnv* env,
+ jclass,
+ jstring method_name) {
jit::Jit* jit = Runtime::Current()->GetJit();
if (jit == nullptr) {
// Just return true for non-jit configurations to stop the infinite loop.
return JNI_TRUE;
}
+ ScopedUtfChars chars(env, method_name);
+ CHECK(chars.c_str() != nullptr);
ScopedObjectAccess soa(Thread::Current());
- OsrVisitor visitor(soa.Self());
+ OsrVisitor visitor(soa.Self(), chars.c_str());
visitor.WalkStack();
return visitor.in_osr_method_;
}
-extern "C" JNIEXPORT jboolean JNICALL Java_Main_ensureInInterpreter(JNIEnv*, jclass) {
+extern "C" JNIEXPORT jboolean JNICALL Java_Main_isInInterpreter(JNIEnv* env,
+ jclass,
+ jstring method_name) {
if (!Runtime::Current()->UseJit()) {
// The return value is irrelevant if we're not using JIT.
return false;
}
+ ScopedUtfChars chars(env, method_name);
+ CHECK(chars.c_str() != nullptr);
ScopedObjectAccess soa(Thread::Current());
- OsrVisitor visitor(soa.Self());
+ OsrVisitor visitor(soa.Self(), chars.c_str());
visitor.WalkStack();
return visitor.in_interpreter_;
}
@@ -112,17 +117,17 @@
class OsrCheckVisitor : public StackVisitor {
public:
- explicit OsrCheckVisitor(Thread* thread)
+ OsrCheckVisitor(Thread* thread, const char* const method_name)
SHARED_REQUIRES(Locks::mutator_lock_)
- : StackVisitor(thread, nullptr, StackVisitor::StackWalkKind::kIncludeInlinedFrames) {}
+ : StackVisitor(thread, nullptr, StackVisitor::StackWalkKind::kIncludeInlinedFrames),
+ method_name_(method_name) {}
bool VisitFrame() SHARED_REQUIRES(Locks::mutator_lock_) {
ArtMethod* m = GetMethod();
std::string m_name(m->GetName());
jit::Jit* jit = Runtime::Current()->GetJit();
- if ((m_name.compare("$noinline$inlineCache") == 0) ||
- (m_name.compare("$noinline$stackOverflow") == 0)) {
+ if (m_name.compare(method_name_) == 0) {
while (jit->GetCodeCache()->LookupOsrMethodHeader(m) == nullptr) {
// Sleep to yield to the compiler thread.
sleep(0);
@@ -133,14 +138,20 @@
}
return true;
}
+
+ const char* const method_name_;
};
-extern "C" JNIEXPORT void JNICALL Java_Main_ensureHasOsrCode(JNIEnv*, jclass) {
+extern "C" JNIEXPORT void JNICALL Java_Main_ensureHasOsrCode(JNIEnv* env,
+ jclass,
+ jstring method_name) {
if (!Runtime::Current()->UseJit()) {
return;
}
+ ScopedUtfChars chars(env, method_name);
+ CHECK(chars.c_str() != nullptr);
ScopedObjectAccess soa(Thread::Current());
- OsrCheckVisitor visitor(soa.Self());
+ OsrCheckVisitor visitor(soa.Self(), chars.c_str());
visitor.WalkStack();
}
diff --git a/test/570-checker-osr/src/Main.java b/test/570-checker-osr/src/Main.java
index 6514334..f22a0c1 100644
--- a/test/570-checker-osr/src/Main.java
+++ b/test/570-checker-osr/src/Main.java
@@ -63,6 +63,9 @@
$noinline$stackOverflow(new Main(), /* isSecondInvocation */ false);
$noinline$stackOverflow(new SubMain(), /* isSecondInvocation */ true);
+
+ $opt$noinline$testOsrInlineLoop(null);
+ System.out.println("b28210356 passed.");
}
public static int $noinline$returnInt() {
@@ -70,7 +73,7 @@
int i = 0;
for (; i < 100000; ++i) {
}
- while (!ensureInOsrCode()) {}
+ while (!isInOsrCode("$noinline$returnInt")) {}
System.out.println(i);
return 53;
}
@@ -80,7 +83,7 @@
int i = 0;
for (; i < 200000; ++i) {
}
- while (!ensureInOsrCode()) {}
+ while (!isInOsrCode("$noinline$returnFloat")) {}
System.out.println(i);
return 42.2f;
}
@@ -90,7 +93,7 @@
int i = 0;
for (; i < 300000; ++i) {
}
- while (!ensureInOsrCode()) {}
+ while (!isInOsrCode("$noinline$returnDouble")) {}
System.out.println(i);
return Double.longBitsToDouble(0xF000000000001111L);
}
@@ -100,7 +103,7 @@
int i = 0;
for (; i < 400000; ++i) {
}
- while (!ensureInOsrCode()) {}
+ while (!isInOsrCode("$noinline$returnLong")) {}
System.out.println(i);
return 0xFFFF000000001111L;
}
@@ -110,14 +113,14 @@
int i = 0;
for (; i < 100000; ++i) {
}
- while (!ensureInOsrCode()) {}
+ while (!isInOsrCode("$noinline$deopt")) {}
DeoptimizationController.startDeoptimization();
}
public static Class $noinline$inlineCache(Main m, boolean isSecondInvocation) {
// If we are running in non-JIT mode, or were unlucky enough to get this method
// already JITted, just return the expected value.
- if (!ensureInInterpreter()) {
+ if (!isInInterpreter("$noinline$inlineCache")) {
return SubMain.class;
}
@@ -125,7 +128,7 @@
// Ensure that we have OSR code to jump to.
if (isSecondInvocation) {
- ensureHasOsrCode();
+ ensureHasOsrCode("$noinline$inlineCache");
}
// This call will be optimized in the OSR compiled code
@@ -137,7 +140,7 @@
// code we are jumping to will have wrongly optimize other as being a
// 'Main'.
if (isSecondInvocation) {
- while (!ensureInOsrCode()) {}
+ while (!isInOsrCode("$noinline$inlineCache")) {}
}
// We used to wrongly optimize this call and assume 'other' was a 'Main'.
@@ -159,7 +162,7 @@
public static void $noinline$stackOverflow(Main m, boolean isSecondInvocation) {
// If we are running in non-JIT mode, or were unlucky enough to get this method
// already JITted, just return the expected value.
- if (!ensureInInterpreter()) {
+ if (!isInInterpreter("$noinline$stackOverflow")) {
return;
}
@@ -168,7 +171,7 @@
if (isSecondInvocation) {
// Ensure we have an OSR code and we jump to it.
- while (!ensureInOsrCode()) {}
+ while (!isInOsrCode("$noinline$stackOverflow")) {}
}
for (int i = 0; i < (isSecondInvocation ? 10000000 : 1); ++i) {
@@ -179,10 +182,45 @@
}
}
- public static native boolean ensureInInterpreter();
- public static native boolean ensureInOsrCode();
+ public static void $opt$noinline$testOsrInlineLoop(String[] args) {
+ // Regression test for inlining a method with a loop to a method without a loop in OSR mode.
+ if (doThrow) throw new Error();
+ assertIntEquals(12, $opt$inline$testRemoveSuspendCheck(12, 5));
+ // Since we cannot have a loop directly in this method, we need to force the OSR
+ // compilation from native code.
+ ensureHasOsrCode("$opt$noinline$testOsrInlineLoop");
+ }
+
+ public static int $opt$inline$testRemoveSuspendCheck(int x, int y) {
+ // For this test we need an inlined loop and have DCE re-run loop analysis
+ // after inlining.
+ while (y > 0) {
+ while ($opt$inline$inlineFalse() || !$opt$inline$inlineTrue()) {
+ x++;
+ }
+ y--;
+ }
+ return x;
+ }
+
+ public static boolean $opt$inline$inlineTrue() {
+ return true;
+ }
+
+ public static boolean $opt$inline$inlineFalse() {
+ return false;
+ }
+
+ public static void assertIntEquals(int expected, int result) {
+ if (expected != result) {
+ throw new Error("Expected: " + expected + ", found: " + result);
+ }
+ }
+
+ public static native boolean isInOsrCode(String methodName);
+ public static native boolean isInInterpreter(String methodName);
public static native void ensureHasProfilingInfo();
- public static native void ensureHasOsrCode();
+ public static native void ensureHasOsrCode(String methodName);
public static boolean doThrow = false;
}
