diff options
| -rw-r--r-- | compiler/optimizing/code_sinking.cc | 83 | ||||
| -rw-r--r-- | test/639-checker-code-sinking/src/Main.java | 101 |
2 files changed, 172 insertions, 12 deletions
diff --git a/compiler/optimizing/code_sinking.cc b/compiler/optimizing/code_sinking.cc index c14ad9c95e..766bb01978 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 @@ static HInstruction* FindIdealPosition(HInstruction* instruction, target_block = target_block->GetDominator(); DCHECK(target_block != nullptr); } - - // 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. + const bool was_in_loop = target_block->IsInLoop(); + + // 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 @@ void CodeSinking::SinkCodeToUncommonBranch(HBasicBlock* end_block) { // 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 bd50fa639f..4becc11845 100644 --- a/test/639-checker-code-sinking/src/Main.java +++ b/test/639-checker-code-sinking/src/Main.java @@ -396,6 +396,9 @@ public class Main { 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 @@ public class Main { 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 @@ public class Main { static boolean doThrow; static boolean doLoop; static boolean doEarlyReturn; + static boolean doOtherEarlyReturn; static Main mainField = new Main(); static Object obj = new Object(); } |