ART: Fix HSelectGenerator for instructions which can throw.
Make sure that HSelectGenerator doesn't hoist instructions which
can throw. Currently this doesn't happen due to
SideEffect::CanTriggerGC however this side effect is to be removed
for some instructions.
Test: select_generator_test.
Test: test-art-host, test-art-target.
Change-Id: I996f6cbdcee4987a36079d387a7b74b326881ab6
diff --git a/compiler/Android.bp b/compiler/Android.bp
index 11521e6..eff4955 100644
--- a/compiler/Android.bp
+++ b/compiler/Android.bp
@@ -346,6 +346,7 @@
"optimizing/parallel_move_test.cc",
"optimizing/pretty_printer_test.cc",
"optimizing/reference_type_propagation_test.cc",
+ "optimizing/select_generator_test.cc",
"optimizing/side_effects_test.cc",
"optimizing/ssa_liveness_analysis_test.cc",
"optimizing/ssa_test.cc",
diff --git a/compiler/optimizing/optimizing_unit_test.h b/compiler/optimizing/optimizing_unit_test.h
index a9bc566..f903f82 100644
--- a/compiler/optimizing/optimizing_unit_test.h
+++ b/compiler/optimizing/optimizing_unit_test.h
@@ -29,6 +29,7 @@
#include "dex/dex_instruction.h"
#include "dex/standard_dex_file.h"
#include "driver/dex_compilation_unit.h"
+#include "graph_checker.h"
#include "handle_scope-inl.h"
#include "mirror/class_loader.h"
#include "mirror/dex_cache.h"
@@ -187,6 +188,77 @@
class OptimizingUnitTest : public CommonCompilerTest, public OptimizingUnitTestHelper {};
+// OptimizingUnitTest with some handy functions to ease the graph creation.
+class ImprovedOptimizingUnitTest : public OptimizingUnitTest {
+ public:
+ ImprovedOptimizingUnitTest() : graph_(CreateGraph()),
+ entry_block_(nullptr),
+ return_block_(nullptr),
+ exit_block_(nullptr),
+ parameter_(nullptr) {}
+
+ virtual ~ImprovedOptimizingUnitTest() {}
+
+ void InitGraph() {
+ entry_block_ = new (GetAllocator()) HBasicBlock(graph_);
+ graph_->AddBlock(entry_block_);
+ graph_->SetEntryBlock(entry_block_);
+
+ return_block_ = new (GetAllocator()) HBasicBlock(graph_);
+ graph_->AddBlock(return_block_);
+
+ exit_block_ = new (GetAllocator()) HBasicBlock(graph_);
+ graph_->AddBlock(exit_block_);
+ graph_->SetExitBlock(exit_block_);
+
+ entry_block_->AddSuccessor(return_block_);
+ return_block_->AddSuccessor(exit_block_);
+
+ parameter_ = new (GetAllocator()) HParameterValue(graph_->GetDexFile(),
+ dex::TypeIndex(0),
+ 0,
+ DataType::Type::kInt32);
+ entry_block_->AddInstruction(parameter_);
+ return_block_->AddInstruction(new (GetAllocator()) HReturnVoid());
+ exit_block_->AddInstruction(new (GetAllocator()) HExit());
+ }
+
+ bool CheckGraph() {
+ GraphChecker checker(graph_);
+ checker.Run();
+ if (!checker.IsValid()) {
+ for (const std::string& error : checker.GetErrors()) {
+ std::cout << error << std::endl;
+ }
+ return false;
+ }
+ return true;
+ }
+
+ HEnvironment* ManuallyBuildEnvFor(HInstruction* instruction,
+ ArenaVector<HInstruction*>* current_locals) {
+ HEnvironment* environment = new (GetAllocator()) HEnvironment(
+ (GetAllocator()),
+ current_locals->size(),
+ graph_->GetArtMethod(),
+ instruction->GetDexPc(),
+ instruction);
+
+ environment->CopyFrom(ArrayRef<HInstruction* const>(*current_locals));
+ instruction->SetRawEnvironment(environment);
+ return environment;
+ }
+
+ protected:
+ HGraph* graph_;
+
+ HBasicBlock* entry_block_;
+ HBasicBlock* return_block_;
+ HBasicBlock* exit_block_;
+
+ HInstruction* parameter_;
+};
+
// Naive string diff data type.
typedef std::list<std::pair<std::string, std::string>> diff_t;
diff --git a/compiler/optimizing/select_generator.cc b/compiler/optimizing/select_generator.cc
index 0d0f7cc..dcc7f77 100644
--- a/compiler/optimizing/select_generator.cc
+++ b/compiler/optimizing/select_generator.cc
@@ -45,7 +45,9 @@
HInstruction* instruction = it.Current();
if (instruction->IsControlFlow()) {
return instruction->IsGoto() || instruction->IsReturn();
- } else if (instruction->CanBeMoved() && !instruction->HasSideEffects()) {
+ } else if (instruction->CanBeMoved() &&
+ !instruction->HasSideEffects() &&
+ !instruction->CanThrow()) {
if (instruction->IsSelect() &&
instruction->AsSelect()->GetCondition()->GetBlock() == block) {
// Count one HCondition and HSelect in the same block as a single instruction.
@@ -119,10 +121,14 @@
// TODO(dbrazdil): This puts an instruction between If and its condition.
// Implement moving of conditions to first users if possible.
while (!true_block->IsSingleGoto() && !true_block->IsSingleReturn()) {
- true_block->GetFirstInstruction()->MoveBefore(if_instruction);
+ HInstruction* instr = true_block->GetFirstInstruction();
+ DCHECK(!instr->CanThrow());
+ instr->MoveBefore(if_instruction);
}
while (!false_block->IsSingleGoto() && !false_block->IsSingleReturn()) {
- false_block->GetFirstInstruction()->MoveBefore(if_instruction);
+ HInstruction* instr = false_block->GetFirstInstruction();
+ DCHECK(!instr->CanThrow());
+ instr->MoveBefore(if_instruction);
}
DCHECK(true_block->IsSingleGoto() || true_block->IsSingleReturn());
DCHECK(false_block->IsSingleGoto() || false_block->IsSingleReturn());
diff --git a/compiler/optimizing/select_generator_test.cc b/compiler/optimizing/select_generator_test.cc
new file mode 100644
index 0000000..6e65497
--- /dev/null
+++ b/compiler/optimizing/select_generator_test.cc
@@ -0,0 +1,96 @@
+/*
+ * Copyright (C) 2018 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.
+ */
+
+#include "select_generator.h"
+
+#include "base/arena_allocator.h"
+#include "builder.h"
+#include "nodes.h"
+#include "optimizing_unit_test.h"
+#include "side_effects_analysis.h"
+
+namespace art {
+
+class SelectGeneratorTest : public ImprovedOptimizingUnitTest {
+ public:
+ void ConstructBasicGraphForSelect(HInstruction* instr) {
+ HBasicBlock* if_block = new (GetAllocator()) HBasicBlock(graph_);
+ HBasicBlock* then_block = new (GetAllocator()) HBasicBlock(graph_);
+ HBasicBlock* else_block = new (GetAllocator()) HBasicBlock(graph_);
+
+ graph_->AddBlock(if_block);
+ graph_->AddBlock(then_block);
+ graph_->AddBlock(else_block);
+
+ entry_block_->ReplaceSuccessor(return_block_, if_block);
+
+ if_block->AddSuccessor(then_block);
+ if_block->AddSuccessor(else_block);
+ then_block->AddSuccessor(return_block_);
+ else_block->AddSuccessor(return_block_);
+
+ HParameterValue* bool_param = new (GetAllocator()) HParameterValue(graph_->GetDexFile(),
+ dex::TypeIndex(0),
+ 1,
+ DataType::Type::kBool);
+ entry_block_->AddInstruction(bool_param);
+ HIntConstant* const1 = graph_->GetIntConstant(1);
+
+ if_block->AddInstruction(new (GetAllocator()) HIf(bool_param));
+
+ then_block->AddInstruction(instr);
+ then_block->AddInstruction(new (GetAllocator()) HGoto());
+
+ else_block->AddInstruction(new (GetAllocator()) HGoto());
+
+ HPhi* phi = new (GetAllocator()) HPhi(GetAllocator(), 0, 0, DataType::Type::kInt32);
+ return_block_->AddPhi(phi);
+ phi->AddInput(instr);
+ phi->AddInput(const1);
+ }
+
+ bool CheckGraphAndTrySelectGenerator() {
+ graph_->BuildDominatorTree();
+ EXPECT_TRUE(CheckGraph());
+
+ SideEffectsAnalysis side_effects(graph_);
+ side_effects.Run();
+ return HSelectGenerator(graph_, /*handles*/ nullptr, /*stats*/ nullptr).Run();
+ }
+};
+
+// HDivZeroCheck might throw and should not be hoisted from the conditional to an unconditional.
+TEST_F(SelectGeneratorTest, testZeroCheck) {
+ InitGraph();
+ HDivZeroCheck* instr = new (GetAllocator()) HDivZeroCheck(parameter_, 0);
+ ConstructBasicGraphForSelect(instr);
+
+ ArenaVector<HInstruction*> current_locals({parameter_, graph_->GetIntConstant(1)},
+ GetAllocator()->Adapter(kArenaAllocInstruction));
+ ManuallyBuildEnvFor(instr, ¤t_locals);
+
+ EXPECT_FALSE(CheckGraphAndTrySelectGenerator());
+}
+
+// Test that SelectGenerator succeeds with HAdd.
+TEST_F(SelectGeneratorTest, testAdd) {
+ InitGraph();
+ HAdd* instr = new (GetAllocator()) HAdd(DataType::Type::kInt32, parameter_, parameter_, 0);
+ ConstructBasicGraphForSelect(instr);
+ EXPECT_TRUE(CheckGraphAndTrySelectGenerator());
+}
+
+} // namespace art
diff --git a/compiler/optimizing/superblock_cloner_test.cc b/compiler/optimizing/superblock_cloner_test.cc
index 6f3bcda..31114b6 100644
--- a/compiler/optimizing/superblock_cloner_test.cc
+++ b/compiler/optimizing/superblock_cloner_test.cc
@@ -30,38 +30,8 @@
// This class provides methods and helpers for testing various cloning and copying routines:
// individual instruction cloning and cloning of the more coarse-grain structures.
-class SuperblockClonerTest : public OptimizingUnitTest {
+class SuperblockClonerTest : public ImprovedOptimizingUnitTest {
public:
- SuperblockClonerTest() : graph_(CreateGraph()),
- entry_block_(nullptr),
- return_block_(nullptr),
- exit_block_(nullptr),
- parameter_(nullptr) {}
-
- void InitGraph() {
- entry_block_ = new (GetAllocator()) HBasicBlock(graph_);
- graph_->AddBlock(entry_block_);
- graph_->SetEntryBlock(entry_block_);
-
- return_block_ = new (GetAllocator()) HBasicBlock(graph_);
- graph_->AddBlock(return_block_);
-
- exit_block_ = new (GetAllocator()) HBasicBlock(graph_);
- graph_->AddBlock(exit_block_);
- graph_->SetExitBlock(exit_block_);
-
- entry_block_->AddSuccessor(return_block_);
- return_block_->AddSuccessor(exit_block_);
-
- parameter_ = new (GetAllocator()) HParameterValue(graph_->GetDexFile(),
- dex::TypeIndex(0),
- 0,
- DataType::Type::kInt32);
- entry_block_->AddInstruction(parameter_);
- return_block_->AddInstruction(new (GetAllocator()) HReturnVoid());
- exit_block_->AddInstruction(new (GetAllocator()) HExit());
- }
-
void CreateBasicLoopControlFlow(HBasicBlock* position,
HBasicBlock* successor,
/* out */ HBasicBlock** header_p,
@@ -137,40 +107,6 @@
null_check->CopyEnvironmentFrom(env);
bounds_check->CopyEnvironmentFrom(env);
}
-
- HEnvironment* ManuallyBuildEnvFor(HInstruction* instruction,
- ArenaVector<HInstruction*>* current_locals) {
- HEnvironment* environment = new (GetAllocator()) HEnvironment(
- (GetAllocator()),
- current_locals->size(),
- graph_->GetArtMethod(),
- instruction->GetDexPc(),
- instruction);
-
- environment->CopyFrom(ArrayRef<HInstruction* const>(*current_locals));
- instruction->SetRawEnvironment(environment);
- return environment;
- }
-
- bool CheckGraph() {
- GraphChecker checker(graph_);
- checker.Run();
- if (!checker.IsValid()) {
- for (const std::string& error : checker.GetErrors()) {
- std::cout << error << std::endl;
- }
- return false;
- }
- return true;
- }
-
- HGraph* graph_;
-
- HBasicBlock* entry_block_;
- HBasicBlock* return_block_;
- HBasicBlock* exit_block_;
-
- HInstruction* parameter_;
};
TEST_F(SuperblockClonerTest, IndividualInstrCloner) {