From 3cd4fc8bbb40a57d2ffde85f543c124f53237c1d Mon Sep 17 00:00:00 2001 From: Calin Juravle Date: Thu, 14 May 2015 15:15:42 +0100 Subject: Eliminate redundant constructor barriers when inlining. Bug: 20410297 Change-Id: I2097743d00eb795d050d390b1918e38c7f41d506 --- compiler/optimizing/builder.cc | 14 ++-- compiler/optimizing/inliner.cc | 20 ++++++ compiler/optimizing/nodes.h | 10 +++ compiler/optimizing/optimizing_compiler.cc | 7 +- compiler/optimizing/optimizing_unit_test.h | 2 +- test/476-checker-ctor-memory-barrier/src/Main.java | 79 +++++++++++++++++++--- 6 files changed, 111 insertions(+), 21 deletions(-) diff --git a/compiler/optimizing/builder.cc b/compiler/optimizing/builder.cc index a5c6f23343..c4eaabf899 100644 --- a/compiler/optimizing/builder.cc +++ b/compiler/optimizing/builder.cc @@ -539,11 +539,6 @@ void HGraphBuilder::Binop_22b(const Instruction& instruction, bool reverse) { } static bool RequiresConstructorBarrier(const DexCompilationUnit* cu, const CompilerDriver& driver) { - // dex compilation unit is null only when unit testing. - if (cu == nullptr) { - return false; - } - Thread* self = Thread::Current(); return cu->IsConstructor() && driver.RequiresConstructorBarrier(self, cu->GetDexFile(), cu->GetClassDefIndex()); @@ -551,9 +546,12 @@ static bool RequiresConstructorBarrier(const DexCompilationUnit* cu, const Compi void HGraphBuilder::BuildReturn(const Instruction& instruction, Primitive::Type type) { if (type == Primitive::kPrimVoid) { - // Note that we might insert redundant barriers when inlining `super` calls. - // TODO: add a data flow analysis to get rid of duplicate barriers. - if (RequiresConstructorBarrier(dex_compilation_unit_, *compiler_driver_)) { + if (graph_->ShouldGenerateConstructorBarrier()) { + // The compilation unit is null during testing. + if (dex_compilation_unit_ != nullptr) { + DCHECK(RequiresConstructorBarrier(dex_compilation_unit_, *compiler_driver_)) + << "Inconsistent use of ShouldGenerateConstructorBarrier. Should not generate a barrier."; + } current_block_->AddInstruction(new (arena_) HMemoryBarrier(kStoreStore)); } current_block_->AddInstruction(new (arena_) HReturnVoid()); diff --git a/compiler/optimizing/inliner.cc b/compiler/optimizing/inliner.cc index afffc7ab4f..56d868f116 100644 --- a/compiler/optimizing/inliner.cc +++ b/compiler/optimizing/inliner.cc @@ -169,10 +169,30 @@ bool HInliner::TryBuildAndInline(Handle resolved_method, resolved_method->GetAccessFlags(), nullptr); + bool requires_ctor_barrier = false; + + if (dex_compilation_unit.IsConstructor()) { + // If it's a super invocation and we already generate a barrier there's no need + // to generate another one. + // We identify super calls by looking at the "this" pointer. If its value is the + // same as the local "this" pointer then we must have a super invocation. + bool is_super_invocation = invoke_instruction->InputAt(0)->IsParameterValue() + && invoke_instruction->InputAt(0)->AsParameterValue()->IsThis(); + if (is_super_invocation && graph_->ShouldGenerateConstructorBarrier()) { + requires_ctor_barrier = false; + } else { + Thread* self = Thread::Current(); + requires_ctor_barrier = compiler_driver_->RequiresConstructorBarrier(self, + dex_compilation_unit.GetDexFile(), + dex_compilation_unit.GetClassDefIndex()); + } + } + HGraph* callee_graph = new (graph_->GetArena()) HGraph( graph_->GetArena(), caller_dex_file, method_index, + requires_ctor_barrier, graph_->IsDebuggable(), graph_->GetCurrentInstructionId()); diff --git a/compiler/optimizing/nodes.h b/compiler/optimizing/nodes.h index cb2e5ccab4..0291992cab 100644 --- a/compiler/optimizing/nodes.h +++ b/compiler/optimizing/nodes.h @@ -120,6 +120,7 @@ class HGraph : public ArenaObject { HGraph(ArenaAllocator* arena, const DexFile& dex_file, uint32_t method_idx, + bool should_generate_constructor_barrier, bool debuggable = false, int start_instruction_id = 0) : arena_(arena), @@ -137,6 +138,7 @@ class HGraph : public ArenaObject { current_instruction_id_(start_instruction_id), dex_file_(dex_file), method_idx_(method_idx), + should_generate_constructor_barrier_(should_generate_constructor_barrier), cached_null_constant_(nullptr), cached_int_constants_(std::less(), arena->Adapter()), cached_float_constants_(std::less(), arena->Adapter()), @@ -247,6 +249,10 @@ class HGraph : public ArenaObject { has_bounds_checks_ = value; } + bool ShouldGenerateConstructorBarrier() const { + return should_generate_constructor_barrier_; + } + bool IsDebuggable() const { return debuggable_; } // Returns a constant of the given type and value. If it does not exist @@ -359,6 +365,8 @@ class HGraph : public ArenaObject { // The method index in the dex file. const uint32_t method_idx_; + const bool should_generate_constructor_barrier_; + // Cached constants. HNullConstant* cached_null_constant_; ArenaSafeMap cached_int_constants_; @@ -2902,6 +2910,8 @@ class HParameterValue : public HExpression<0> { bool CanBeNull() const OVERRIDE { return !is_this_; } + bool IsThis() const { return is_this_; } + DECLARE_INSTRUCTION(ParameterValue); private: diff --git a/compiler/optimizing/optimizing_compiler.cc b/compiler/optimizing/optimizing_compiler.cc index 8bb5d8ebae..be9a42425b 100644 --- a/compiler/optimizing/optimizing_compiler.cc +++ b/compiler/optimizing/optimizing_compiler.cc @@ -512,9 +512,14 @@ CompiledMethod* OptimizingCompiler::TryCompile(const DexFile::CodeItem* code_ite class_def_idx, method_idx, access_flags, compiler_driver->GetVerifiedMethod(&dex_file, method_idx)); + bool requires_barrier = dex_compilation_unit.IsConstructor() + && compiler_driver->RequiresConstructorBarrier(Thread::Current(), + dex_compilation_unit.GetDexFile(), + dex_compilation_unit.GetClassDefIndex()); ArenaAllocator arena(Runtime::Current()->GetArenaPool()); HGraph* graph = new (&arena) HGraph( - &arena, dex_file, method_idx, compiler_driver->GetCompilerOptions().GetDebuggable()); + &arena, dex_file, method_idx, requires_barrier, + compiler_driver->GetCompilerOptions().GetDebuggable()); // For testing purposes, we put a special marker on method names that should be compiled // with this compiler. This makes sure we're not regressing. diff --git a/compiler/optimizing/optimizing_unit_test.h b/compiler/optimizing/optimizing_unit_test.h index 4f8ec65e43..1fe9346b0f 100644 --- a/compiler/optimizing/optimizing_unit_test.h +++ b/compiler/optimizing/optimizing_unit_test.h @@ -74,7 +74,7 @@ void RemoveSuspendChecks(HGraph* graph) { inline HGraph* CreateGraph(ArenaAllocator* allocator) { return new (allocator) HGraph( - allocator, *reinterpret_cast(allocator->Alloc(sizeof(DexFile))), -1); + allocator, *reinterpret_cast(allocator->Alloc(sizeof(DexFile))), -1, false); } // Create a control-flow graph from Dex instructions. diff --git a/test/476-checker-ctor-memory-barrier/src/Main.java b/test/476-checker-ctor-memory-barrier/src/Main.java index 769ae208d9..9badc0f5e0 100644 --- a/test/476-checker-ctor-memory-barrier/src/Main.java +++ b/test/476-checker-ctor-memory-barrier/src/Main.java @@ -14,6 +14,7 @@ * limitations under the License. */ +// TODO: Add more tests after we can inline functions with calls. class ClassWithoutFinals { // CHECK-START: void ClassWithoutFinals.() register (after) @@ -80,6 +81,20 @@ class InheritFromClassWithFinals extends ClassWithFinals { super(cond); // should not inline the super constructor } + + // CHECK-START: void InheritFromClassWithFinals.(int) register (after) + // CHECK: MemoryBarrier kind:StoreStore + // CHECK: MemoryBarrier kind:StoreStore + // CHECK: ReturnVoid + + // CHECK-START: void InheritFromClassWithFinals.(int) register (after) + // CHECK-NOT: InvokeStaticOrDirect + public InheritFromClassWithFinals(int unused) { + // Should inline the super constructor and insert a memory barrier. + + // Should inline the new instance call and insert another memory barrier. + new InheritFromClassWithFinals(); + } } class HaveFinalsAndInheritFromClassWithFinals extends ClassWithFinals { @@ -87,14 +102,13 @@ class HaveFinalsAndInheritFromClassWithFinals extends ClassWithFinals { // CHECK-START: void HaveFinalsAndInheritFromClassWithFinals.() register (after) // CHECK: MemoryBarrier kind:StoreStore - // CHECK: MemoryBarrier kind:StoreStore // CHECK-NOT: {{.*}} // CHECK: ReturnVoid // CHECK-START: void HaveFinalsAndInheritFromClassWithFinals.() register (after) // CHECK-NOT: InvokeStaticOrDirect public HaveFinalsAndInheritFromClassWithFinals() { - // Should inline the super constructor. + // Should inline the super constructor and remove the memory barrier. y = 0; } @@ -108,6 +122,25 @@ class HaveFinalsAndInheritFromClassWithFinals extends ClassWithFinals { // should not inline the super constructor y = 0; } + + // CHECK-START: void HaveFinalsAndInheritFromClassWithFinals.(int) register (after) + // CHECK: MemoryBarrier kind:StoreStore + // CHECK: MemoryBarrier kind:StoreStore + // CHECK: MemoryBarrier kind:StoreStore + // CHECK-NOT: {{.*}} + // CHECK: ReturnVoid + + // CHECK-START: void HaveFinalsAndInheritFromClassWithFinals.(int) register (after) + // CHECK-NOT: InvokeStaticOrDirect + public HaveFinalsAndInheritFromClassWithFinals(int unused) { + // Should inline the super constructor and keep just one memory barrier. + y = 0; + + // Should inline new instance and keep one barrier. + new HaveFinalsAndInheritFromClassWithFinals(); + // Should inline new instance and keep one barrier. + new InheritFromClassWithFinals(); + } } public class Main { @@ -121,27 +154,51 @@ public class Main { return new ClassWithFinals(false); } - // CHECK-START: ClassWithFinals Main.inlineConstructorBarrier() register (after) + // CHECK-START: void Main.inlineNew() register (after) + // CHECK: MemoryBarrier kind:StoreStore + // CHECK-NOT: {{.*}} + // CHECK: Return + + // CHECK-START: void Main.inlineNew() register (after) + // CHECK-NOT: InvokeStaticOrDirect + public static void inlineNew() { + new ClassWithFinals(); + } + + // CHECK-START: void Main.inlineNew1() register (after) // CHECK: MemoryBarrier kind:StoreStore // CHECK-NOT: {{.*}} // CHECK: Return - // CHECK-START: ClassWithFinals Main.inlineConstructorBarrier() register (after) + // CHECK-START: void Main.inlineNew1() register (after) // CHECK-NOT: InvokeStaticOrDirect - public static ClassWithFinals inlineConstructorBarrier() { - return new ClassWithFinals(); + public static void inlineNew1() { + new InheritFromClassWithFinals(); } - // CHECK-START: InheritFromClassWithFinals Main.doubleInlineConstructorBarrier() register (after) + // CHECK-START: void Main.inlineNew2() register (after) + // CHECK: MemoryBarrier kind:StoreStore + // CHECK-NOT: {{.*}} + // CHECK: Return + + // CHECK-START: void Main.inlineNew2() register (after) + // CHECK-NOT: InvokeStaticOrDirect + public static void inlineNew2() { + new HaveFinalsAndInheritFromClassWithFinals(); + } + + // CHECK-START: void Main.inlineNew3() register (after) + // CHECK: MemoryBarrier kind:StoreStore // CHECK: MemoryBarrier kind:StoreStore // CHECK-NOT: {{.*}} // CHECK: Return - // CHECK-START: InheritFromClassWithFinals Main.doubleInlineConstructorBarrier() register (after) + // CHECK-START: void Main.inlineNew3() register (after) // CHECK-NOT: InvokeStaticOrDirect - public static InheritFromClassWithFinals doubleInlineConstructorBarrier() { - return new InheritFromClassWithFinals(); + public static void inlineNew3() { + new HaveFinalsAndInheritFromClassWithFinals(); + new HaveFinalsAndInheritFromClassWithFinals(); } - public static void main(String[] args) { } + public static void main(String[] args) {} } -- cgit v1.2.3-59-g8ed1b