Allow code to sink right before the TryBoundary
Instructions before TryBoundary entry are not considered to be inside
of a try. Therefore, we can allow to sink instructions at that level.
Bug: 226143661
Test: art/test/testrunner/testrunner.py --host --64 --optimizing -b
Change-Id: If29a53cfd94e1d0a7afb79d0e9fb7e76b337c493
diff --git a/compiler/optimizing/code_sinking.cc b/compiler/optimizing/code_sinking.cc
index c14ad9c..766bb01 100644
--- a/compiler/optimizing/code_sinking.cc
+++ b/compiler/optimizing/code_sinking.cc
@@ -17,7 +17,9 @@
#include "code_sinking.h"
#include "base/arena_bit_vector.h"
+#include "base/array_ref.h"
#include "base/bit_vector-inl.h"
+#include "base/logging.h"
#include "base/scoped_arena_allocator.h"
#include "base/scoped_arena_containers.h"
#include "common_dominator.h"
@@ -216,22 +218,49 @@
target_block = target_block->GetDominator();
DCHECK(target_block != nullptr);
}
+ const bool was_in_loop = target_block->IsInLoop();
- // Bail if the instruction would throw into a catch block.
- if (instruction->CanThrow() && target_block->IsTryBlock()) {
- // TODO(solanes): Here we could do something similar to the loop above and move to the first
- // dominator, which is not a try block, instead of just returning nullptr. If we do so, we have
- // to also make sure we are not in a loop.
-
- if (instruction->GetBlock()->IsTryBlock() &&
- instruction->GetBlock()->GetTryCatchInformation()->GetTryEntry().GetId() ==
- target_block->GetTryCatchInformation()->GetTryEntry().GetId()) {
- // Sink within the same try block is allowed.
+ // For throwing instructions we can move them into:
+ // * Blocks that are not part of a try
+ // * Catch blocks are suitable as well, as long as they are not part of an outer try.
+ // * Blocks that are part of the same try that the instrucion was already in.
+ //
+ // We cannot move an instruction that can throw into a try that said instruction is not a part of
+ // already, as that would mean it will throw into a different catch block. If we detect that
+ // `target_block` is not a valid block to move `instruction` to, we traverse up the dominator tree
+ // to find if we have a suitable block.
+ while (instruction->CanThrow() && target_block->GetTryCatchInformation() != nullptr) {
+ if (target_block->IsCatchBlock()) {
+ // If the catch block has an xhandler, it means it is inside of an outer try.
+ const bool inside_of_another_try_catch = target_block->GetSuccessors().size() != 1;
+ if (!inside_of_another_try_catch) {
+ // If we have a catch block, it's okay to sink as long as that catch is not inside of
+ // another try catch.
+ break;
+ }
} else {
+ DCHECK(target_block->IsTryBlock());
+ if (instruction->GetBlock()->IsTryBlock() &&
+ instruction->GetBlock()->GetTryCatchInformation()->GetTryEntry().GetId() ==
+ target_block->GetTryCatchInformation()->GetTryEntry().GetId()) {
+ // Sink within the same try block is allowed.
+ break;
+ }
+ }
+ // We are now in the case where we would be moving to a different try. Since we don't want
+ // that, traverse up the dominator tree to find a suitable block.
+ if (!post_dominated.IsBitSet(target_block->GetDominator()->GetBlockId())) {
+ // We couldn't find a suitable block.
return nullptr;
}
+ target_block = target_block->GetDominator();
+ DCHECK(target_block != nullptr);
}
+ // We shouldn't land in a loop if we weren't in one before traversing up the dominator tree
+ // regarding try catches.
+ DCHECK_IMPLIES(target_block->IsInLoop(), was_in_loop);
+
// Find insertion position. No need to filter anymore, as we have found a
// target block.
HInstruction* insert_pos = nullptr;
@@ -296,7 +325,37 @@
// We currently bail for loops.
is_post_dominated = false;
} else {
- for (HBasicBlock* successor : block->GetSuccessors()) {
+ // BasicBlock that are try entries look like this:
+ // BasicBlock i:
+ // instr 1
+ // ...
+ // instr N
+ // TryBoundary kind:entry ---Try begins here---
+ //
+ // Due to how our BasicBlocks are structured, BasicBlock i will have an xhandler successor
+ // since we are starting a try. If we use `GetSuccessors` for this case, we will check if
+ // the catch block is post_dominated.
+ //
+ // However, this catch block doesn't matter: when we sink the instruction into that
+ // BasicBlock i, we do it before the TryBoundary (i.e. outside of the try and outside the
+ // catch's domain). We can ignore catch blocks using `GetNormalSuccessors` to sink code
+ // right before the start of a try block.
+ //
+ // On the other side of the coin, BasicBlock that are try exits look like this:
+ // BasicBlock j:
+ // instr 1
+ // ...
+ // instr N
+ // TryBoundary kind:exit ---Try ends here---
+ //
+ // If we sink to these basic blocks we would be sinking inside of the try so we would like
+ // to check the catch block for post dominance.
+ const bool ends_with_try_boundary_entry =
+ block->EndsWithTryBoundary() && block->GetLastInstruction()->AsTryBoundary()->IsEntry();
+ ArrayRef<HBasicBlock* const> successors =
+ ends_with_try_boundary_entry ? block->GetNormalSuccessors() :
+ ArrayRef<HBasicBlock* const>(block->GetSuccessors());
+ for (HBasicBlock* successor : successors) {
if (!post_dominated.IsBitSet(successor->GetBlockId())) {
is_post_dominated = false;
break;
diff --git a/test/639-checker-code-sinking/src/Main.java b/test/639-checker-code-sinking/src/Main.java
index bd50fa6..4becc11 100644
--- a/test/639-checker-code-sinking/src/Main.java
+++ b/test/639-checker-code-sinking/src/Main.java
@@ -396,6 +396,9 @@
assertEquals(456, testDoNotSinkToTry());
assertEquals(456, testDoNotSinkToCatchInsideTry());
assertEquals(456, testSinkWithinTryBlock());
+ assertEquals(456, testSinkRightBeforeTryBlock());
+ assertEquals(456, testSinkToSecondCatch());
+ assertEquals(456, testDoNotSinkToCatchInsideTryWithMoreThings(false, false));
}
/// CHECK-START: int Main.testSinkToCatchBlock() code_sinking (before)
@@ -510,6 +513,103 @@
return 456;
}
+ /// CHECK-START: int Main.testSinkRightBeforeTryBlock() code_sinking (before)
+ /// CHECK: <<ObjLoadClass:l\d+>> LoadClass class_name:java.lang.Object
+ /// CHECK: NewInstance [<<ObjLoadClass>>]
+ /// CHECK: If
+ /// CHECK: TryBoundary kind:entry
+
+ /// CHECK-START: int Main.testSinkRightBeforeTryBlock() code_sinking (after)
+ /// CHECK: If
+ /// CHECK: <<ObjLoadClass:l\d+>> LoadClass class_name:java.lang.Object
+ /// CHECK: NewInstance [<<ObjLoadClass>>]
+ /// CHECK: TryBoundary kind:entry
+ private static int testSinkRightBeforeTryBlock() {
+ Object o = new Object();
+ if (doEarlyReturn) {
+ try {
+ throw new Error(o.toString());
+ } catch (Error e) {
+ return 123;
+ }
+ }
+ return 456;
+ }
+
+ /// CHECK-START: int Main.testSinkToSecondCatch() code_sinking (before)
+ /// CHECK: <<ObjLoadClass:l\d+>> LoadClass class_name:java.lang.Object
+ /// CHECK: NewInstance [<<ObjLoadClass>>]
+ /// CHECK: TryBoundary kind:entry
+ /// CHECK: TryBoundary kind:entry
+
+ /// CHECK-START: int Main.testSinkToSecondCatch() code_sinking (after)
+ /// CHECK: TryBoundary kind:entry
+ /// CHECK: TryBoundary kind:entry
+ /// CHECK: <<ObjLoadClass:l\d+>> LoadClass class_name:java.lang.Object
+ /// CHECK: NewInstance [<<ObjLoadClass>>]
+
+ // Consistency check to make sure there's exactly two entry TryBoundary.
+ /// CHECK-START: int Main.testSinkToSecondCatch() code_sinking (after)
+ /// CHECK: TryBoundary kind:entry
+ /// CHECK: TryBoundary kind:entry
+ /// CHECK-NOT: TryBoundary kind:entry
+ private static int testSinkToSecondCatch() {
+ Object o = new Object();
+ try {
+ if (doEarlyReturn) {
+ return 123;
+ }
+ } catch (Error e) {
+ throw new Error();
+ }
+
+ try {
+ // We need a different boolean to the one above, so that the compiler cannot optimize this
+ // return away.
+ if (doOtherEarlyReturn) {
+ return 789;
+ }
+ } catch (Error e) {
+ throw new Error(o.toString());
+ }
+
+ return 456;
+ }
+
+ /// CHECK-START: int Main.testDoNotSinkToCatchInsideTryWithMoreThings(boolean, boolean) code_sinking (before)
+ /// CHECK-NOT: TryBoundary kind:entry
+ /// CHECK: <<ObjLoadClass:l\d+>> LoadClass class_name:java.lang.Object
+ /// CHECK: NewInstance [<<ObjLoadClass>>]
+
+ /// CHECK-START: int Main.testDoNotSinkToCatchInsideTryWithMoreThings(boolean, boolean) code_sinking (after)
+ /// CHECK-NOT: TryBoundary kind:entry
+ /// CHECK: <<ObjLoadClass:l\d+>> LoadClass class_name:java.lang.Object
+ /// CHECK: NewInstance [<<ObjLoadClass>>]
+
+ // Tests that we don't sink the Object creation into a catch handler surrounded by try/catch, even
+ // when that inner catch is not at the boundary of the outer try catch.
+ private static int testDoNotSinkToCatchInsideTryWithMoreThings(boolean a, boolean b) {
+ Object o = new Object();
+ try {
+ if (a) {
+ System.out.println(a);
+ }
+ try {
+ if (doEarlyReturn) {
+ return 123;
+ }
+ } catch (Error e) {
+ throw new Error(o.toString());
+ }
+ if (b) {
+ System.out.println(b);
+ }
+ } catch (Error e) {
+ throw new Error();
+ }
+ return 456;
+ }
+
private static void assertEquals(int expected, int actual) {
if (expected != actual) {
throw new AssertionError("Expected: " + expected + ", Actual: " + actual);
@@ -523,6 +623,7 @@
static boolean doThrow;
static boolean doLoop;
static boolean doEarlyReturn;
+ static boolean doOtherEarlyReturn;
static Main mainField = new Main();
static Object obj = new Object();
}