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 (file)
--- a/compiler/optimizing/nodes.cc
+++ b/compiler/optimizing/nodes.cc
@@ -618,7 +618,24 @@ void HLoopInformation::Populate() {
     }
   }
 
-  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 (file)
--- a/compiler/optimizing/ssa_liveness_analysis.cc
+++ b/compiler/optimizing/ssa_liveness_analysis.cc
@@ -318,7 +318,7 @@ void SsaLivenessAnalysis::ComputeLiveRanges() {
         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 (file)
--- a/test/570-checker-osr/expected.txt
+++ b/test/570-checker-osr/expected.txt
@@ -3,3 +3,4 @@ JNI_OnLoad called
 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 (file)
--- 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 @@ class OsrVisitor : public StackVisitor {
     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 @@ class OsrVisitor : public StackVisitor {
     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 @@ extern "C" JNIEXPORT void JNICALL Java_Main_ensureHasProfilingInfo(JNIEnv*, jcla
 
 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 @@ class OsrCheckVisitor : public StackVisitor {
     }
     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 (file)
--- a/test/570-checker-osr/src/Main.java
+++ b/test/570-checker-osr/src/Main.java
@@ -63,6 +63,9 @@ public class Main {
 
     $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 @@ public class Main {
     int i = 0;
     for (; i < 100000; ++i) {
     }
-    while (!ensureInOsrCode()) {}
+    while (!isInOsrCode("$noinline$returnInt")) {}
     System.out.println(i);
     return 53;
   }
@@ -80,7 +83,7 @@ public class Main {
     int i = 0;
     for (; i < 200000; ++i) {
     }
-    while (!ensureInOsrCode()) {}
+    while (!isInOsrCode("$noinline$returnFloat")) {}
     System.out.println(i);
     return 42.2f;
   }
@@ -90,7 +93,7 @@ public class Main {
     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 @@ public class Main {
     int i = 0;
     for (; i < 400000; ++i) {
     }
-    while (!ensureInOsrCode()) {}
+    while (!isInOsrCode("$noinline$returnLong")) {}
     System.out.println(i);
     return 0xFFFF000000001111L;
   }
@@ -110,14 +113,14 @@ public class Main {
     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 @@ public class Main {
 
     // 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 @@ public class Main {
     // 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 class Main {
   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 @@ public class Main {
 
     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 class Main {
     }
   }
 
-  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;
 }