diff options
Diffstat (limited to 'compiler')
23 files changed, 339 insertions, 778 deletions
diff --git a/compiler/optimizing/code_generator.cc b/compiler/optimizing/code_generator.cc index 0e294b560a..b0e07e32ea 100644 --- a/compiler/optimizing/code_generator.cc +++ b/compiler/optimizing/code_generator.cc @@ -158,25 +158,6 @@ ReadBarrierOption CodeGenerator::GetCompilerReadBarrierOption() const { return EmitReadBarrier() ? kWithReadBarrier : kWithoutReadBarrier; } -bool CodeGenerator::ShouldCheckGCCard(DataType::Type type, - HInstruction* value, - WriteBarrierKind write_barrier_kind) const { - const CompilerOptions& options = GetCompilerOptions(); - const bool result = - // Check the GC card in debug mode, - options.EmitRunTimeChecksInDebugMode() && - // only for CC GC, - options.EmitReadBarrier() && - // and if we eliminated the write barrier in WBE. - !StoreNeedsWriteBarrier(type, value, write_barrier_kind) && - CodeGenerator::StoreNeedsWriteBarrier(type, value); - - DCHECK_IMPLIES(result, write_barrier_kind == WriteBarrierKind::kDontEmit); - DCHECK_IMPLIES(result, !GetGraph()->IsCompilingBaseline()); - - return result; -} - ScopedArenaAllocator* CodeGenerator::GetScopedAllocator() { DCHECK(code_generation_data_ != nullptr); return code_generation_data_->GetScopedAllocator(); @@ -1627,17 +1608,6 @@ void CodeGenerator::EmitParallelMoves(Location from1, GetMoveResolver()->EmitNativeCode(¶llel_move); } -bool CodeGenerator::StoreNeedsWriteBarrier(DataType::Type type, - HInstruction* value, - WriteBarrierKind write_barrier_kind) const { - // Check that null value is not represented as an integer constant. - DCHECK_IMPLIES(type == DataType::Type::kReference, !value->IsIntConstant()); - // Branch profiling currently doesn't support running optimizations. - return GetGraph()->IsCompilingBaseline() - ? CodeGenerator::StoreNeedsWriteBarrier(type, value) - : write_barrier_kind != WriteBarrierKind::kDontEmit; -} - void CodeGenerator::ValidateInvokeRuntime(QuickEntrypointEnum entrypoint, HInstruction* instruction, SlowPathCode* slow_path) { diff --git a/compiler/optimizing/code_generator.h b/compiler/optimizing/code_generator.h index 0e26a501da..de6fc85da4 100644 --- a/compiler/optimizing/code_generator.h +++ b/compiler/optimizing/code_generator.h @@ -380,11 +380,6 @@ class CodeGenerator : public DeletableArenaObject<kArenaAllocCodeGenerator> { bool EmitNonBakerReadBarrier() const; ReadBarrierOption GetCompilerReadBarrierOption() const; - // Returns true if we should check the GC card for consistency purposes. - bool ShouldCheckGCCard(DataType::Type type, - HInstruction* value, - WriteBarrierKind write_barrier_kind) const; - // Get the ScopedArenaAllocator used for codegen memory allocation. ScopedArenaAllocator* GetScopedAllocator(); @@ -508,11 +503,6 @@ class CodeGenerator : public DeletableArenaObject<kArenaAllocCodeGenerator> { return type == DataType::Type::kReference && !value->IsNullConstant(); } - // If we are compiling a graph with the WBE pass enabled, we want to honor the WriteBarrierKind - // set during the WBE pass. - bool StoreNeedsWriteBarrier(DataType::Type type, - HInstruction* value, - WriteBarrierKind write_barrier_kind) const; // Performs checks pertaining to an InvokeRuntime call. void ValidateInvokeRuntime(QuickEntrypointEnum entrypoint, diff --git a/compiler/optimizing/code_generator_arm64.cc b/compiler/optimizing/code_generator_arm64.cc index db5bb43c79..9027976165 100644 --- a/compiler/optimizing/code_generator_arm64.cc +++ b/compiler/optimizing/code_generator_arm64.cc @@ -1499,24 +1499,18 @@ void CodeGeneratorARM64::AddLocationAsTemp(Location location, LocationSummary* l } } -void CodeGeneratorARM64::MaybeMarkGCCard(Register object, Register value, bool emit_null_check) { +void CodeGeneratorARM64::MarkGCCard(Register object, Register value, bool emit_null_check) { + UseScratchRegisterScope temps(GetVIXLAssembler()); + Register card = temps.AcquireX(); + Register temp = temps.AcquireW(); // Index within the CardTable - 32bit. vixl::aarch64::Label done; if (emit_null_check) { __ Cbz(value, &done); } - MarkGCCard(object); - if (emit_null_check) { - __ Bind(&done); - } -} - -void CodeGeneratorARM64::MarkGCCard(Register object) { - UseScratchRegisterScope temps(GetVIXLAssembler()); - Register card = temps.AcquireX(); - Register temp = temps.AcquireW(); // Index within the CardTable - 32bit. // Load the address of the card table into `card`. __ Ldr(card, MemOperand(tr, Thread::CardTableOffset<kArm64PointerSize>().Int32Value())); - // Calculate the offset (in the card table) of the card corresponding to `object`. + // Calculate the offset (in the card table) of the card corresponding to + // `object`. __ Lsr(temp, object, gc::accounting::CardTable::kCardShift); // Write the `art::gc::accounting::CardTable::kCardDirty` value into the // `object`'s card. @@ -1532,24 +1526,9 @@ void CodeGeneratorARM64::MarkGCCard(Register object) { // of the card to mark; and 2. to load the `kCardDirty` value) saves a load // (no need to explicitly load `kCardDirty` as an immediate value). __ Strb(card, MemOperand(card, temp.X())); -} - -void CodeGeneratorARM64::CheckGCCardIsValid(Register object) { - UseScratchRegisterScope temps(GetVIXLAssembler()); - Register card = temps.AcquireX(); - Register temp = temps.AcquireW(); // Index within the CardTable - 32bit. - vixl::aarch64::Label done; - // Load the address of the card table into `card`. - __ Ldr(card, MemOperand(tr, Thread::CardTableOffset<kArm64PointerSize>().Int32Value())); - // Calculate the offset (in the card table) of the card corresponding to `object`. - __ Lsr(temp, object, gc::accounting::CardTable::kCardShift); - // assert (!clean || !self->is_gc_marking) - __ Ldrb(temp, MemOperand(card, temp.X())); - static_assert(gc::accounting::CardTable::kCardClean == 0); - __ Cbnz(temp, &done); - __ Cbz(mr, &done); - __ Unreachable(); - __ Bind(&done); + if (emit_null_check) { + __ Bind(&done); + } } void CodeGeneratorARM64::SetupBlockedRegisters() const { @@ -2339,16 +2318,12 @@ void InstructionCodeGeneratorARM64::HandleFieldSet(HInstruction* instruction, } } - const bool needs_write_barrier = - codegen_->StoreNeedsWriteBarrier(field_type, instruction->InputAt(1), write_barrier_kind); - - if (needs_write_barrier) { - codegen_->MaybeMarkGCCard( + if (CodeGenerator::StoreNeedsWriteBarrier(field_type, instruction->InputAt(1)) && + write_barrier_kind != WriteBarrierKind::kDontEmit) { + codegen_->MarkGCCard( obj, Register(value), - value_can_be_null && write_barrier_kind == WriteBarrierKind::kEmitNotBeingReliedOn); - } else if (codegen_->ShouldCheckGCCard(field_type, instruction->InputAt(1), write_barrier_kind)) { - codegen_->CheckGCCardIsValid(obj); + value_can_be_null && write_barrier_kind == WriteBarrierKind::kEmitWithNullCheck); } } @@ -2902,9 +2877,8 @@ void InstructionCodeGeneratorARM64::VisitArraySet(HArraySet* instruction) { DataType::Type value_type = instruction->GetComponentType(); LocationSummary* locations = instruction->GetLocations(); bool needs_type_check = instruction->NeedsTypeCheck(); - const WriteBarrierKind write_barrier_kind = instruction->GetWriteBarrierKind(); bool needs_write_barrier = - codegen_->StoreNeedsWriteBarrier(value_type, instruction->GetValue(), write_barrier_kind); + CodeGenerator::StoreNeedsWriteBarrier(value_type, instruction->GetValue()); Register array = InputRegisterAt(instruction, 0); CPURegister value = InputCPURegisterOrZeroRegAt(instruction, 2); @@ -2915,10 +2889,6 @@ void InstructionCodeGeneratorARM64::VisitArraySet(HArraySet* instruction) { MacroAssembler* masm = GetVIXLAssembler(); if (!needs_write_barrier) { - if (codegen_->ShouldCheckGCCard(value_type, instruction->GetValue(), write_barrier_kind)) { - codegen_->CheckGCCardIsValid(array); - } - DCHECK(!needs_type_check); if (index.IsConstant()) { offset += Int64FromLocation(index) << DataType::SizeShift(value_type); @@ -2952,85 +2922,78 @@ void InstructionCodeGeneratorARM64::VisitArraySet(HArraySet* instruction) { } else { DCHECK(!instruction->GetArray()->IsIntermediateAddress()); - // We can have `value` be `Zero` here if we have a write barrier we are relying on.' - DCHECK_IMPLIES(Register(value).IsZero(), - write_barrier_kind == WriteBarrierKind::kEmitBeingReliedOn); - bool can_value_be_null = true; + bool can_value_be_null = instruction->GetValueCanBeNull(); + vixl::aarch64::Label do_store; + if (can_value_be_null) { + __ Cbz(Register(value), &do_store); + } + SlowPathCodeARM64* slow_path = nullptr; - if (!Register(value).IsZero()) { - can_value_be_null = instruction->GetValueCanBeNull(); - vixl::aarch64::Label do_store; - if (can_value_be_null) { - __ Cbz(Register(value), &do_store); - } + if (needs_type_check) { + slow_path = new (codegen_->GetScopedAllocator()) ArraySetSlowPathARM64(instruction); + codegen_->AddSlowPath(slow_path); - if (needs_type_check) { - slow_path = new (codegen_->GetScopedAllocator()) ArraySetSlowPathARM64(instruction); - codegen_->AddSlowPath(slow_path); + const uint32_t class_offset = mirror::Object::ClassOffset().Int32Value(); + const uint32_t super_offset = mirror::Class::SuperClassOffset().Int32Value(); + const uint32_t component_offset = mirror::Class::ComponentTypeOffset().Int32Value(); - const uint32_t class_offset = mirror::Object::ClassOffset().Int32Value(); - const uint32_t super_offset = mirror::Class::SuperClassOffset().Int32Value(); - const uint32_t component_offset = mirror::Class::ComponentTypeOffset().Int32Value(); + UseScratchRegisterScope temps(masm); + Register temp = temps.AcquireSameSizeAs(array); + Register temp2 = temps.AcquireSameSizeAs(array); - UseScratchRegisterScope temps(masm); - Register temp = temps.AcquireSameSizeAs(array); - Register temp2 = temps.AcquireSameSizeAs(array); + // Note that when Baker read barriers are enabled, the type + // checks are performed without read barriers. This is fine, + // even in the case where a class object is in the from-space + // after the flip, as a comparison involving such a type would + // not produce a false positive; it may of course produce a + // false negative, in which case we would take the ArraySet + // slow path. - // Note that when Baker read barriers are enabled, the type - // checks are performed without read barriers. This is fine, - // even in the case where a class object is in the from-space - // after the flip, as a comparison involving such a type would - // not produce a false positive; it may of course produce a - // false negative, in which case we would take the ArraySet - // slow path. + // /* HeapReference<Class> */ temp = array->klass_ + { + // Ensure that between load and MaybeRecordImplicitNullCheck there are no pools emitted. + EmissionCheckScope guard(GetVIXLAssembler(), kMaxMacroInstructionSizeInBytes); + __ Ldr(temp, HeapOperand(array, class_offset)); + codegen_->MaybeRecordImplicitNullCheck(instruction); + } + GetAssembler()->MaybeUnpoisonHeapReference(temp); - // /* HeapReference<Class> */ temp = array->klass_ - { - // Ensure that between load and MaybeRecordImplicitNullCheck there are no pools emitted. - EmissionCheckScope guard(GetVIXLAssembler(), kMaxMacroInstructionSizeInBytes); - __ Ldr(temp, HeapOperand(array, class_offset)); - codegen_->MaybeRecordImplicitNullCheck(instruction); - } + // /* HeapReference<Class> */ temp = temp->component_type_ + __ Ldr(temp, HeapOperand(temp, component_offset)); + // /* HeapReference<Class> */ temp2 = value->klass_ + __ Ldr(temp2, HeapOperand(Register(value), class_offset)); + // If heap poisoning is enabled, no need to unpoison `temp` + // nor `temp2`, as we are comparing two poisoned references. + __ Cmp(temp, temp2); + + if (instruction->StaticTypeOfArrayIsObjectArray()) { + vixl::aarch64::Label do_put; + __ B(eq, &do_put); + // If heap poisoning is enabled, the `temp` reference has + // not been unpoisoned yet; unpoison it now. GetAssembler()->MaybeUnpoisonHeapReference(temp); - // /* HeapReference<Class> */ temp = temp->component_type_ - __ Ldr(temp, HeapOperand(temp, component_offset)); - // /* HeapReference<Class> */ temp2 = value->klass_ - __ Ldr(temp2, HeapOperand(Register(value), class_offset)); - // If heap poisoning is enabled, no need to unpoison `temp` - // nor `temp2`, as we are comparing two poisoned references. - __ Cmp(temp, temp2); - - if (instruction->StaticTypeOfArrayIsObjectArray()) { - vixl::aarch64::Label do_put; - __ B(eq, &do_put); - // If heap poisoning is enabled, the `temp` reference has - // not been unpoisoned yet; unpoison it now. - GetAssembler()->MaybeUnpoisonHeapReference(temp); - - // /* HeapReference<Class> */ temp = temp->super_class_ - __ Ldr(temp, HeapOperand(temp, super_offset)); - // If heap poisoning is enabled, no need to unpoison - // `temp`, as we are comparing against null below. - __ Cbnz(temp, slow_path->GetEntryLabel()); - __ Bind(&do_put); - } else { - __ B(ne, slow_path->GetEntryLabel()); - } + // /* HeapReference<Class> */ temp = temp->super_class_ + __ Ldr(temp, HeapOperand(temp, super_offset)); + // If heap poisoning is enabled, no need to unpoison + // `temp`, as we are comparing against null below. + __ Cbnz(temp, slow_path->GetEntryLabel()); + __ Bind(&do_put); + } else { + __ B(ne, slow_path->GetEntryLabel()); } + } - if (can_value_be_null) { - DCHECK(do_store.IsLinked()); - __ Bind(&do_store); - } + if (instruction->GetWriteBarrierKind() != WriteBarrierKind::kDontEmit) { + DCHECK_EQ(instruction->GetWriteBarrierKind(), WriteBarrierKind::kEmitNoNullCheck) + << " Already null checked so we shouldn't do it again."; + codegen_->MarkGCCard(array, value.W(), /* emit_null_check= */ false); } - DCHECK_NE(write_barrier_kind, WriteBarrierKind::kDontEmit); - // TODO(solanes): The WriteBarrierKind::kEmitNotBeingReliedOn case should be able to skip this - // write barrier when its value is null (without an extra cbz since we already checked if the - // value is null for the type check). This will be done as a follow-up since it is a runtime - // optimization that needs extra care. - codegen_->MarkGCCard(array); + if (can_value_be_null) { + DCHECK(do_store.IsLinked()); + __ Bind(&do_store); + } UseScratchRegisterScope temps(masm); if (kPoisonHeapReferences) { diff --git a/compiler/optimizing/code_generator_arm64.h b/compiler/optimizing/code_generator_arm64.h index c78137b6ed..7ff08f55cb 100644 --- a/compiler/optimizing/code_generator_arm64.h +++ b/compiler/optimizing/code_generator_arm64.h @@ -658,20 +658,10 @@ class CodeGeneratorARM64 : public CodeGenerator { const Arm64Assembler& GetAssembler() const override { return assembler_; } vixl::aarch64::MacroAssembler* GetVIXLAssembler() { return GetAssembler()->GetVIXLAssembler(); } - // Emit a write barrier if: - // A) emit_null_check is false - // B) emit_null_check is true, and value is not null. - void MaybeMarkGCCard(vixl::aarch64::Register object, - vixl::aarch64::Register value, - bool emit_null_check); - - // Emit a write barrier unconditionally. - void MarkGCCard(vixl::aarch64::Register object); - - // Crash if the card table is not valid. This check is only emitted for the CC GC. We assert - // `(!clean || !self->is_gc_marking)`, since the card table should not be set to clean when the CC - // GC is marking for eliminated write barriers. - void CheckGCCardIsValid(vixl::aarch64::Register object); + // Emit a write barrier. + void MarkGCCard(vixl::aarch64::Register object, + vixl::aarch64::Register value, + bool emit_null_check); void GenerateMemoryBarrier(MemBarrierKind kind); diff --git a/compiler/optimizing/code_generator_arm_vixl.cc b/compiler/optimizing/code_generator_arm_vixl.cc index 544d35c206..00c14b0b46 100644 --- a/compiler/optimizing/code_generator_arm_vixl.cc +++ b/compiler/optimizing/code_generator_arm_vixl.cc @@ -22,7 +22,6 @@ #include "art_method-inl.h" #include "base/bit_utils.h" #include "base/bit_utils_iterator.h" -#include "base/globals.h" #include "class_root-inl.h" #include "class_table.h" #include "code_generator_utils.h" @@ -5931,15 +5930,16 @@ void LocationsBuilderARMVIXL::HandleFieldSet(HInstruction* instruction, && is_wide && !codegen_->GetInstructionSetFeatures().HasAtomicLdrdAndStrd(); bool needs_write_barrier = - codegen_->StoreNeedsWriteBarrier(field_type, instruction->InputAt(1), write_barrier_kind); - bool check_gc_card = - codegen_->ShouldCheckGCCard(field_type, instruction->InputAt(1), write_barrier_kind); - + CodeGenerator::StoreNeedsWriteBarrier(field_type, instruction->InputAt(1)); // Temporary registers for the write barrier. // TODO: consider renaming StoreNeedsWriteBarrier to StoreNeedsGCMark. - if (needs_write_barrier || check_gc_card) { - locations->AddTemp(Location::RequiresRegister()); - locations->AddTemp(Location::RequiresRegister()); + if (needs_write_barrier) { + if (write_barrier_kind != WriteBarrierKind::kDontEmit) { + locations->AddTemp(Location::RequiresRegister()); + locations->AddTemp(Location::RequiresRegister()); + } else if (kPoisonHeapReferences) { + locations->AddTemp(Location::RequiresRegister()); + } } else if (generate_volatile) { // ARM encoding have some additional constraints for ldrexd/strexd: // - registers need to be consecutive @@ -5955,8 +5955,6 @@ void LocationsBuilderARMVIXL::HandleFieldSet(HInstruction* instruction, locations->AddTemp(LocationFrom(r2)); locations->AddTemp(LocationFrom(r3)); } - } else if (kPoisonHeapReferences && field_type == DataType::Type::kReference) { - locations->AddTemp(Location::RequiresRegister()); } } @@ -5975,7 +5973,7 @@ void InstructionCodeGeneratorARMVIXL::HandleFieldSet(HInstruction* instruction, DataType::Type field_type = field_info.GetFieldType(); uint32_t offset = field_info.GetFieldOffset().Uint32Value(); bool needs_write_barrier = - codegen_->StoreNeedsWriteBarrier(field_type, instruction->InputAt(1), write_barrier_kind); + CodeGenerator::StoreNeedsWriteBarrier(field_type, instruction->InputAt(1)); if (is_volatile) { codegen_->GenerateMemoryBarrier(MemBarrierKind::kAnyStore); @@ -5998,7 +5996,10 @@ void InstructionCodeGeneratorARMVIXL::HandleFieldSet(HInstruction* instruction, case DataType::Type::kReference: { vixl32::Register value_reg = RegisterFrom(value); - if (kPoisonHeapReferences) { + if (kPoisonHeapReferences && needs_write_barrier) { + // Note that in the case where `value` is a null reference, + // we do not enter this block, as a null reference does not + // need poisoning. DCHECK_EQ(field_type, DataType::Type::kReference); value_reg = RegisterFrom(locations->GetTemp(0)); __ Mov(value_reg, RegisterFrom(value)); @@ -6068,19 +6069,16 @@ void InstructionCodeGeneratorARMVIXL::HandleFieldSet(HInstruction* instruction, UNREACHABLE(); } - if (needs_write_barrier) { + if (CodeGenerator::StoreNeedsWriteBarrier(field_type, instruction->InputAt(1)) && + write_barrier_kind != WriteBarrierKind::kDontEmit) { vixl32::Register temp = RegisterFrom(locations->GetTemp(0)); vixl32::Register card = RegisterFrom(locations->GetTemp(1)); - codegen_->MaybeMarkGCCard( + codegen_->MarkGCCard( temp, card, base, RegisterFrom(value), - value_can_be_null && write_barrier_kind == WriteBarrierKind::kEmitNotBeingReliedOn); - } else if (codegen_->ShouldCheckGCCard(field_type, instruction->InputAt(1), write_barrier_kind)) { - vixl32::Register temp = RegisterFrom(locations->GetTemp(0)); - vixl32::Register card = RegisterFrom(locations->GetTemp(1)); - codegen_->CheckGCCardIsValid(temp, card, base); + value_can_be_null && write_barrier_kind == WriteBarrierKind::kEmitWithNullCheck); } if (is_volatile) { @@ -6833,12 +6831,8 @@ void InstructionCodeGeneratorARMVIXL::VisitArrayGet(HArrayGet* instruction) { void LocationsBuilderARMVIXL::VisitArraySet(HArraySet* instruction) { DataType::Type value_type = instruction->GetComponentType(); - const WriteBarrierKind write_barrier_kind = instruction->GetWriteBarrierKind(); bool needs_write_barrier = - codegen_->StoreNeedsWriteBarrier(value_type, instruction->GetValue(), write_barrier_kind); - bool check_gc_card = - codegen_->ShouldCheckGCCard(value_type, instruction->GetValue(), write_barrier_kind); - + CodeGenerator::StoreNeedsWriteBarrier(value_type, instruction->GetValue()); bool needs_type_check = instruction->NeedsTypeCheck(); LocationSummary* locations = new (GetGraph()->GetAllocator()) LocationSummary( @@ -6852,12 +6846,11 @@ void LocationsBuilderARMVIXL::VisitArraySet(HArraySet* instruction) { } else { locations->SetInAt(2, Location::RequiresRegister()); } - if (needs_write_barrier || check_gc_card || instruction->NeedsTypeCheck()) { - // Temporary registers for type checking, write barrier, checking the dirty bit, or register - // poisoning. - locations->AddTemp(Location::RequiresRegister()); + if (needs_write_barrier) { + // Temporary registers for the write barrier or register poisoning. + // TODO(solanes): We could reduce the temp usage but it requires some non-trivial refactoring of + // InstructionCodeGeneratorARMVIXL::VisitArraySet. locations->AddTemp(Location::RequiresRegister()); - } else if (kPoisonHeapReferences && value_type == DataType::Type::kReference) { locations->AddTemp(Location::RequiresRegister()); } } @@ -6868,9 +6861,8 @@ void InstructionCodeGeneratorARMVIXL::VisitArraySet(HArraySet* instruction) { Location index = locations->InAt(1); DataType::Type value_type = instruction->GetComponentType(); bool needs_type_check = instruction->NeedsTypeCheck(); - const WriteBarrierKind write_barrier_kind = instruction->GetWriteBarrierKind(); bool needs_write_barrier = - codegen_->StoreNeedsWriteBarrier(value_type, instruction->GetValue(), write_barrier_kind); + CodeGenerator::StoreNeedsWriteBarrier(value_type, instruction->GetValue()); uint32_t data_offset = mirror::Array::DataOffset(DataType::Size(value_type)).Uint32Value(); Location value_loc = locations->InAt(2); @@ -6939,18 +6931,17 @@ void InstructionCodeGeneratorARMVIXL::VisitArraySet(HArraySet* instruction) { codegen_->StoreToShiftedRegOffset(value_type, value_loc, temp, RegisterFrom(index)); } codegen_->MaybeRecordImplicitNullCheck(instruction); - if (write_barrier_kind == WriteBarrierKind::kEmitBeingReliedOn) { - // We need to set a write barrier here even though we are writing null, since this write - // barrier is being relied on. - DCHECK(needs_write_barrier); - vixl32::Register temp1 = RegisterFrom(locations->GetTemp(0)); - vixl32::Register temp2 = RegisterFrom(locations->GetTemp(1)); - codegen_->MarkGCCard(temp1, temp2, array); - } + DCHECK(!needs_write_barrier); DCHECK(!needs_type_check); break; } + DCHECK(needs_write_barrier); + Location temp1_loc = locations->GetTemp(0); + vixl32::Register temp1 = RegisterFrom(temp1_loc); + Location temp2_loc = locations->GetTemp(1); + vixl32::Register temp2 = RegisterFrom(temp2_loc); + bool can_value_be_null = instruction->GetValueCanBeNull(); vixl32::Label do_store; if (can_value_be_null) { @@ -6974,9 +6965,6 @@ void InstructionCodeGeneratorARMVIXL::VisitArraySet(HArraySet* instruction) { // negative, in which case we would take the ArraySet slow // path. - vixl32::Register temp1 = RegisterFrom(locations->GetTemp(0)); - vixl32::Register temp2 = RegisterFrom(locations->GetTemp(1)); - { // Ensure we record the pc position immediately after the `ldr` instruction. ExactAssemblyScope aas(GetVIXLAssembler(), @@ -7014,29 +7002,22 @@ void InstructionCodeGeneratorARMVIXL::VisitArraySet(HArraySet* instruction) { } } + if (instruction->GetWriteBarrierKind() != WriteBarrierKind::kDontEmit) { + DCHECK_EQ(instruction->GetWriteBarrierKind(), WriteBarrierKind::kEmitNoNullCheck) + << " Already null checked so we shouldn't do it again."; + codegen_->MarkGCCard(temp1, temp2, array, value, /* emit_null_check= */ false); + } + if (can_value_be_null) { DCHECK(do_store.IsReferenced()); __ Bind(&do_store); } - if (needs_write_barrier) { - // TODO(solanes): The WriteBarrierKind::kEmitNotBeingReliedOn case should be able to skip - // this write barrier when its value is null (without an extra CompareAndBranchIfZero since - // we already checked if the value is null for the type check). This will be done as a - // follow-up since it is a runtime optimization that needs extra care. - vixl32::Register temp1 = RegisterFrom(locations->GetTemp(0)); - vixl32::Register temp2 = RegisterFrom(locations->GetTemp(1)); - codegen_->MarkGCCard(temp1, temp2, array); - } else if (codegen_->ShouldCheckGCCard( - value_type, instruction->GetValue(), write_barrier_kind)) { - vixl32::Register temp1 = RegisterFrom(locations->GetTemp(0)); - vixl32::Register temp2 = RegisterFrom(locations->GetTemp(1)); - codegen_->CheckGCCardIsValid(temp1, temp2, array); - } - vixl32::Register source = value; if (kPoisonHeapReferences) { - vixl32::Register temp1 = RegisterFrom(locations->GetTemp(0)); + // Note that in the case where `value` is a null reference, + // we do not enter this block, as a null reference does not + // need poisoning. DCHECK_EQ(value_type, DataType::Type::kReference); __ Mov(temp1, value); GetAssembler()->PoisonHeapReference(temp1); @@ -7252,28 +7233,20 @@ void InstructionCodeGeneratorARMVIXL::VisitBoundsCheck(HBoundsCheck* instruction } } -void CodeGeneratorARMVIXL::MaybeMarkGCCard(vixl32::Register temp, - vixl32::Register card, - vixl32::Register object, - vixl32::Register value, - bool emit_null_check) { +void CodeGeneratorARMVIXL::MarkGCCard(vixl32::Register temp, + vixl32::Register card, + vixl32::Register object, + vixl32::Register value, + bool emit_null_check) { vixl32::Label is_null; if (emit_null_check) { __ CompareAndBranchIfZero(value, &is_null, /* is_far_target=*/ false); } - MarkGCCard(temp, card, object); - if (emit_null_check) { - __ Bind(&is_null); - } -} - -void CodeGeneratorARMVIXL::MarkGCCard(vixl32::Register temp, - vixl32::Register card, - vixl32::Register object) { // Load the address of the card table into `card`. GetAssembler()->LoadFromOffset( kLoadWord, card, tr, Thread::CardTableOffset<kArmPointerSize>().Int32Value()); - // Calculate the offset (in the card table) of the card corresponding to `object`. + // Calculate the offset (in the card table) of the card corresponding to + // `object`. __ Lsr(temp, object, Operand::From(gc::accounting::CardTable::kCardShift)); // Write the `art::gc::accounting::CardTable::kCardDirty` value into the // `object`'s card. @@ -7289,24 +7262,9 @@ void CodeGeneratorARMVIXL::MarkGCCard(vixl32::Register temp, // of the card to mark; and 2. to load the `kCardDirty` value) saves a load // (no need to explicitly load `kCardDirty` as an immediate value). __ Strb(card, MemOperand(card, temp)); -} - -void CodeGeneratorARMVIXL::CheckGCCardIsValid(vixl32::Register temp, - vixl32::Register card, - vixl32::Register object) { - vixl32::Label done; - // Load the address of the card table into `card`. - GetAssembler()->LoadFromOffset( - kLoadWord, card, tr, Thread::CardTableOffset<kArmPointerSize>().Int32Value()); - // Calculate the offset (in the card table) of the card corresponding to `object`. - __ Lsr(temp, object, Operand::From(gc::accounting::CardTable::kCardShift)); - // assert (!clean || !self->is_gc_marking) - __ Ldrb(temp, MemOperand(card, temp)); - static_assert(gc::accounting::CardTable::kCardClean == 0); - __ CompareAndBranchIfNonZero(temp, &done, /*is_far_target=*/false); - __ CompareAndBranchIfZero(mr, &done, /*is_far_target=*/false); - __ Bkpt(0); - __ Bind(&done); + if (emit_null_check) { + __ Bind(&is_null); + } } void LocationsBuilderARMVIXL::VisitParallelMove([[maybe_unused]] HParallelMove* instruction) { diff --git a/compiler/optimizing/code_generator_arm_vixl.h b/compiler/optimizing/code_generator_arm_vixl.h index c5b24470bf..00e0bfa399 100644 --- a/compiler/optimizing/code_generator_arm_vixl.h +++ b/compiler/optimizing/code_generator_arm_vixl.h @@ -615,26 +615,12 @@ class CodeGeneratorARMVIXL : public CodeGenerator { HInstruction* instruction, SlowPathCode* slow_path); - // Emit a write barrier if: - // A) emit_null_check is false - // B) emit_null_check is true, and value is not null. - void MaybeMarkGCCard(vixl::aarch32::Register temp, - vixl::aarch32::Register card, - vixl::aarch32::Register object, - vixl::aarch32::Register value, - bool emit_null_check); - - // Emit a write barrier unconditionally. + // Emit a write barrier. void MarkGCCard(vixl::aarch32::Register temp, vixl::aarch32::Register card, - vixl::aarch32::Register object); - - // Crash if the card table is not valid. This check is only emitted for the CC GC. We assert - // `(!clean || !self->is_gc_marking)`, since the card table should not be set to clean when the CC - // GC is marking for eliminated write barriers. - void CheckGCCardIsValid(vixl::aarch32::Register temp, - vixl::aarch32::Register card, - vixl::aarch32::Register object); + vixl::aarch32::Register object, + vixl::aarch32::Register value, + bool emit_null_check); void GenerateMemoryBarrier(MemBarrierKind kind); diff --git a/compiler/optimizing/code_generator_riscv64.cc b/compiler/optimizing/code_generator_riscv64.cc index 92eef9f824..0c0b8a9f14 100644 --- a/compiler/optimizing/code_generator_riscv64.cc +++ b/compiler/optimizing/code_generator_riscv64.cc @@ -2422,21 +2422,16 @@ void InstructionCodeGeneratorRISCV64::HandleShift(HBinaryOperation* instruction) } } -void CodeGeneratorRISCV64::MaybeMarkGCCard(XRegister object, - XRegister value, - bool value_can_be_null) { +void CodeGeneratorRISCV64::MarkGCCard(XRegister object, + XRegister value, + bool value_can_be_null) { Riscv64Label done; - if (value_can_be_null) { - __ Beqz(value, &done); - } - MarkGCCard(object); - __ Bind(&done); -} - -void CodeGeneratorRISCV64::MarkGCCard(XRegister object) { ScratchRegisterScope srs(GetAssembler()); XRegister card = srs.AllocateXRegister(); XRegister temp = srs.AllocateXRegister(); + if (value_can_be_null) { + __ Beqz(value, &done); + } // Load the address of the card table into `card`. __ Loadd(card, TR, Thread::CardTableOffset<kRiscv64PointerSize>().Int32Value()); @@ -2457,27 +2452,9 @@ void CodeGeneratorRISCV64::MarkGCCard(XRegister object) { // of the card to mark; and 2. to load the `kCardDirty` value) saves a load // (no need to explicitly load `kCardDirty` as an immediate value). __ Sb(card, temp, 0); // No scratch register left for `Storeb()`. -} - -void CodeGeneratorRISCV64::CheckGCCardIsValid(XRegister object) { - Riscv64Label done; - ScratchRegisterScope srs(GetAssembler()); - XRegister card = srs.AllocateXRegister(); - XRegister temp = srs.AllocateXRegister(); - // Load the address of the card table into `card`. - __ Loadd(card, TR, Thread::CardTableOffset<kRiscv64PointerSize>().Int32Value()); - - // Calculate the address of the card corresponding to `object`. - __ Srli(temp, object, gc::accounting::CardTable::kCardShift); - __ Add(temp, card, temp); - // assert (!clean || !self->is_gc_marking) - __ Lb(temp, temp, 0); - static_assert(gc::accounting::CardTable::kCardClean == 0); - __ Bnez(temp, &done); - __ Loadw(temp, TR, Thread::IsGcMarkingOffset<kRiscv64PointerSize>().Int32Value()); - __ Beqz(temp, &done); - __ Unimp(); - __ Bind(&done); + if (value_can_be_null) { + __ Bind(&done); + } } void LocationsBuilderRISCV64::HandleFieldSet(HInstruction* instruction) { @@ -2506,15 +2483,12 @@ void InstructionCodeGeneratorRISCV64::HandleFieldSet(HInstruction* instruction, codegen_->MaybeRecordImplicitNullCheck(instruction); } - bool needs_write_barrier = - codegen_->StoreNeedsWriteBarrier(type, instruction->InputAt(1), write_barrier_kind); - if (needs_write_barrier) { - codegen_->MaybeMarkGCCard( + if (CodeGenerator::StoreNeedsWriteBarrier(type, instruction->InputAt(1)) && + write_barrier_kind != WriteBarrierKind::kDontEmit) { + codegen_->MarkGCCard( obj, value.AsRegister<XRegister>(), - value_can_be_null && write_barrier_kind == WriteBarrierKind::kEmitNotBeingReliedOn); - } else if (codegen_->ShouldCheckGCCard(type, instruction->InputAt(1), write_barrier_kind)) { - codegen_->CheckGCCardIsValid(obj); + value_can_be_null && write_barrier_kind == WriteBarrierKind::kEmitWithNullCheck); } } @@ -2925,9 +2899,8 @@ void InstructionCodeGeneratorRISCV64::VisitArraySet(HArraySet* instruction) { Location value = locations->InAt(2); DataType::Type value_type = instruction->GetComponentType(); bool needs_type_check = instruction->NeedsTypeCheck(); - const WriteBarrierKind write_barrier_kind = instruction->GetWriteBarrierKind(); bool needs_write_barrier = - codegen_->StoreNeedsWriteBarrier(value_type, instruction->GetValue(), write_barrier_kind); + CodeGenerator::StoreNeedsWriteBarrier(value_type, instruction->GetValue()); size_t data_offset = mirror::Array::DataOffset(DataType::Size(value_type)).Uint32Value(); SlowPathCodeRISCV64* slow_path = nullptr; @@ -2988,18 +2961,15 @@ void InstructionCodeGeneratorRISCV64::VisitArraySet(HArraySet* instruction) { } } + if (instruction->GetWriteBarrierKind() != WriteBarrierKind::kDontEmit) { + DCHECK_EQ(instruction->GetWriteBarrierKind(), WriteBarrierKind::kEmitNoNullCheck) + << " Already null checked so we shouldn't do it again."; + codegen_->MarkGCCard(array, value.AsRegister<XRegister>(), /* value_can_be_null= */ false); + } + if (can_value_be_null) { __ Bind(&do_store); } - - DCHECK_NE(instruction->GetWriteBarrierKind(), WriteBarrierKind::kDontEmit); - // TODO(solanes): The WriteBarrierKind::kEmitNotBeingReliedOn case should be able to skip - // this write barrier when its value is null (without an extra Beqz since we already checked - // if the value is null for the type check). This will be done as a follow-up since it is a - // runtime optimization that needs extra care. - codegen_->MarkGCCard(array); - } else if (codegen_->ShouldCheckGCCard(value_type, instruction->GetValue(), write_barrier_kind)) { - codegen_->CheckGCCardIsValid(array); } if (index.IsConstant()) { diff --git a/compiler/optimizing/code_generator_riscv64.h b/compiler/optimizing/code_generator_riscv64.h index ba43090590..321403f7f2 100644 --- a/compiler/optimizing/code_generator_riscv64.h +++ b/compiler/optimizing/code_generator_riscv64.h @@ -750,18 +750,7 @@ class CodeGeneratorRISCV64 : public CodeGenerator { // artReadBarrierForRootSlow. void GenerateReadBarrierForRootSlow(HInstruction* instruction, Location out, Location root); - // Emit a write barrier if: - // A) emit_null_check is false - // B) emit_null_check is true, and value is not null. - void MaybeMarkGCCard(XRegister object, XRegister value, bool emit_null_check); - - // Emit a write barrier unconditionally. - void MarkGCCard(XRegister object); - - // Crash if the card table is not valid. This check is only emitted for the CC GC. We assert - // `(!clean || !self->is_gc_marking)`, since the card table should not be set to clean when the CC - // GC is marking for eliminated write barriers. - void CheckGCCardIsValid(XRegister object); + void MarkGCCard(XRegister object, XRegister value, bool value_can_be_null); // // Heap poisoning. diff --git a/compiler/optimizing/code_generator_x86.cc b/compiler/optimizing/code_generator_x86.cc index 058ecf7242..71db5c99af 100644 --- a/compiler/optimizing/code_generator_x86.cc +++ b/compiler/optimizing/code_generator_x86.cc @@ -44,7 +44,6 @@ #include "utils/assembler.h" #include "utils/stack_checks.h" #include "utils/x86/assembler_x86.h" -#include "utils/x86/constants_x86.h" #include "utils/x86/managed_register_x86.h" namespace art HIDDEN { @@ -5873,23 +5872,17 @@ void CodeGeneratorX86::EmitLinkerPatches(ArenaVector<linker::LinkerPatch>* linke DCHECK_EQ(size, linker_patches->size()); } -void CodeGeneratorX86::MaybeMarkGCCard( +void CodeGeneratorX86::MarkGCCard( Register temp, Register card, Register object, Register value, bool emit_null_check) { NearLabel is_null; if (emit_null_check) { __ testl(value, value); __ j(kEqual, &is_null); } - MarkGCCard(temp, card, object); - if (emit_null_check) { - __ Bind(&is_null); - } -} - -void CodeGeneratorX86::MarkGCCard(Register temp, Register card, Register object) { // Load the address of the card table into `card`. __ fs()->movl(card, Address::Absolute(Thread::CardTableOffset<kX86PointerSize>().Int32Value())); - // Calculate the offset (in the card table) of the card corresponding to `object`. + // Calculate the offset (in the card table) of the card corresponding to + // `object`. __ movl(temp, object); __ shrl(temp, Immediate(gc::accounting::CardTable::kCardShift)); // Write the `art::gc::accounting::CardTable::kCardDirty` value into the @@ -5907,23 +5900,9 @@ void CodeGeneratorX86::MarkGCCard(Register temp, Register card, Register object) // (no need to explicitly load `kCardDirty` as an immediate value). __ movb(Address(temp, card, TIMES_1, 0), X86ManagedRegister::FromCpuRegister(card).AsByteRegister()); -} - -void CodeGeneratorX86::CheckGCCardIsValid(Register temp, Register card, Register object) { - NearLabel done; - __ j(kEqual, &done); - // Load the address of the card table into `card`. - __ fs()->movl(card, Address::Absolute(Thread::CardTableOffset<kX86PointerSize>().Int32Value())); - // Calculate the offset (in the card table) of the card corresponding to `object`. - __ movl(temp, object); - __ shrl(temp, Immediate(gc::accounting::CardTable::kCardShift)); - // assert (!clean || !self->is_gc_marking) - __ cmpb(Address(temp, card, TIMES_1, 0), Immediate(gc::accounting::CardTable::kCardClean)); - __ j(kNotEqual, &done); - __ fs()->cmpl(Address::Absolute(Thread::IsGcMarkingOffset<kX86PointerSize>()), Immediate(0)); - __ j(kEqual, &done); - __ int3(); - __ Bind(&done); + if (emit_null_check) { + __ Bind(&is_null); + } } void LocationsBuilderX86::HandleFieldGet(HInstruction* instruction, const FieldInfo& field_info) { @@ -6049,17 +6028,14 @@ void LocationsBuilderX86::HandleFieldSet(HInstruction* instruction, } else { locations->SetInAt(1, Location::RegisterOrConstant(instruction->InputAt(1))); - bool needs_write_barrier = - codegen_->StoreNeedsWriteBarrier(field_type, instruction->InputAt(1), write_barrier_kind); - bool check_gc_card = - codegen_->ShouldCheckGCCard(field_type, instruction->InputAt(1), write_barrier_kind); - - if (needs_write_barrier || check_gc_card) { - locations->AddTemp(Location::RequiresRegister()); - // Ensure the card is in a byte register. - locations->AddTemp(Location::RegisterLocation(ECX)); - } else if (kPoisonHeapReferences && field_type == DataType::Type::kReference) { - locations->AddTemp(Location::RequiresRegister()); + if (CodeGenerator::StoreNeedsWriteBarrier(field_type, instruction->InputAt(1))) { + if (write_barrier_kind != WriteBarrierKind::kDontEmit) { + locations->AddTemp(Location::RequiresRegister()); + // Ensure the card is in a byte register. + locations->AddTemp(Location::RegisterLocation(ECX)); + } else if (kPoisonHeapReferences) { + locations->AddTemp(Location::RequiresRegister()); + } } } } @@ -6075,7 +6051,7 @@ void InstructionCodeGeneratorX86::HandleFieldSet(HInstruction* instruction, LocationSummary* locations = instruction->GetLocations(); Location value = locations->InAt(value_index); bool needs_write_barrier = - codegen_->StoreNeedsWriteBarrier(field_type, instruction->InputAt(1), write_barrier_kind); + CodeGenerator::StoreNeedsWriteBarrier(field_type, instruction->InputAt(value_index)); if (is_volatile) { codegen_->GenerateMemoryBarrier(MemBarrierKind::kAnyStore); @@ -6107,19 +6083,15 @@ void InstructionCodeGeneratorX86::HandleFieldSet(HInstruction* instruction, case DataType::Type::kInt32: case DataType::Type::kReference: { - if (kPoisonHeapReferences && field_type == DataType::Type::kReference) { - if (value.IsConstant()) { - DCHECK(value.GetConstant()->IsNullConstant()) - << "constant value " << CodeGenerator::GetInt32ValueOf(value.GetConstant()) - << " is not null. Instruction " << *instruction; - // No need to poison null, just do a movl. - __ movl(field_addr, Immediate(0)); - } else { - Register temp = locations->GetTemp(0).AsRegister<Register>(); - __ movl(temp, value.AsRegister<Register>()); - __ PoisonHeapReference(temp); - __ movl(field_addr, temp); - } + if (kPoisonHeapReferences && needs_write_barrier) { + // Note that in the case where `value` is a null reference, + // we do not enter this block, as the reference does not + // need poisoning. + DCHECK_EQ(field_type, DataType::Type::kReference); + Register temp = locations->GetTemp(0).AsRegister<Register>(); + __ movl(temp, value.AsRegister<Register>()); + __ PoisonHeapReference(temp); + __ movl(field_addr, temp); } else if (value.IsConstant()) { int32_t v = CodeGenerator::GetInt32ValueOf(value.GetConstant()); __ movl(field_addr, Immediate(v)); @@ -6188,38 +6160,15 @@ void InstructionCodeGeneratorX86::HandleFieldSet(HInstruction* instruction, codegen_->MaybeRecordImplicitNullCheck(instruction); } - if (needs_write_barrier) { + if (needs_write_barrier && write_barrier_kind != WriteBarrierKind::kDontEmit) { Register temp = locations->GetTemp(0).AsRegister<Register>(); Register card = locations->GetTemp(1).AsRegister<Register>(); - if (value.IsConstant()) { - DCHECK(value.GetConstant()->IsNullConstant()) - << "constant value " << CodeGenerator::GetInt32ValueOf(value.GetConstant()) - << " is not null. Instruction: " << *instruction; - if (write_barrier_kind == WriteBarrierKind::kEmitBeingReliedOn) { - codegen_->MarkGCCard(temp, card, base); - } - } else { - codegen_->MaybeMarkGCCard( - temp, - card, - base, - value.AsRegister<Register>(), - value_can_be_null && write_barrier_kind == WriteBarrierKind::kEmitNotBeingReliedOn); - } - } else if (codegen_->ShouldCheckGCCard(field_type, instruction->InputAt(1), write_barrier_kind)) { - if (value.IsConstant()) { - // If we are storing a constant for a reference, we are in the case where we are storing - // null but we cannot skip it as this write barrier is being relied on by coalesced write - // barriers. - DCHECK(value.GetConstant()->IsNullConstant()) - << "constant value " << CodeGenerator::GetInt32ValueOf(value.GetConstant()) - << " is not null. Instruction: " << *instruction; - // No need to check the dirty bit as this value is null. - } else { - Register temp = locations->GetTemp(0).AsRegister<Register>(); - Register card = locations->GetTemp(1).AsRegister<Register>(); - codegen_->CheckGCCardIsValid(temp, card, base); - } + codegen_->MarkGCCard( + temp, + card, + base, + value.AsRegister<Register>(), + value_can_be_null && write_barrier_kind == WriteBarrierKind::kEmitWithNullCheck); } if (is_volatile) { @@ -6500,11 +6449,8 @@ void InstructionCodeGeneratorX86::VisitArrayGet(HArrayGet* instruction) { void LocationsBuilderX86::VisitArraySet(HArraySet* instruction) { DataType::Type value_type = instruction->GetComponentType(); - WriteBarrierKind write_barrier_kind = instruction->GetWriteBarrierKind(); bool needs_write_barrier = - codegen_->StoreNeedsWriteBarrier(value_type, instruction->GetValue(), write_barrier_kind); - bool check_gc_card = - codegen_->ShouldCheckGCCard(value_type, instruction->GetValue(), write_barrier_kind); + CodeGenerator::StoreNeedsWriteBarrier(value_type, instruction->GetValue()); bool needs_type_check = instruction->NeedsTypeCheck(); LocationSummary* locations = new (GetGraph()->GetAllocator()) LocationSummary( @@ -6525,14 +6471,13 @@ void LocationsBuilderX86::VisitArraySet(HArraySet* instruction) { } else { locations->SetInAt(2, Location::RegisterOrConstant(instruction->InputAt(2))); } - if (needs_write_barrier || check_gc_card) { - // Used by reference poisoning, type checking, emitting, or checking a write barrier. - locations->AddTemp(Location::RequiresRegister()); - // Only used when emitting or checking a write barrier. Ensure the card is in a byte register. - locations->AddTemp(Location::RegisterLocation(ECX)); - } else if ((kPoisonHeapReferences && value_type == DataType::Type::kReference) || - instruction->NeedsTypeCheck()) { + if (needs_write_barrier) { + // Used by reference poisoning or emitting write barrier. locations->AddTemp(Location::RequiresRegister()); + if (instruction->GetWriteBarrierKind() != WriteBarrierKind::kDontEmit) { + // Only used when emitting a write barrier. Ensure the card is in a byte register. + locations->AddTemp(Location::RegisterLocation(ECX)); + } } } @@ -6544,9 +6489,8 @@ void InstructionCodeGeneratorX86::VisitArraySet(HArraySet* instruction) { Location value = locations->InAt(2); DataType::Type value_type = instruction->GetComponentType(); bool needs_type_check = instruction->NeedsTypeCheck(); - WriteBarrierKind write_barrier_kind = instruction->GetWriteBarrierKind(); bool needs_write_barrier = - codegen_->StoreNeedsWriteBarrier(value_type, instruction->GetValue(), write_barrier_kind); + CodeGenerator::StoreNeedsWriteBarrier(value_type, instruction->GetValue()); switch (value_type) { case DataType::Type::kBool: @@ -6586,19 +6530,16 @@ void InstructionCodeGeneratorX86::VisitArraySet(HArraySet* instruction) { DCHECK(value.IsConstant()) << value; __ movl(address, Immediate(0)); codegen_->MaybeRecordImplicitNullCheck(instruction); - if (write_barrier_kind == WriteBarrierKind::kEmitBeingReliedOn) { - // We need to set a write barrier here even though we are writing null, since this write - // barrier is being relied on. - DCHECK(needs_write_barrier); - Register temp = locations->GetTemp(0).AsRegister<Register>(); - Register card = locations->GetTemp(1).AsRegister<Register>(); - codegen_->MarkGCCard(temp, card, array); - } + DCHECK(!needs_write_barrier); DCHECK(!needs_type_check); break; } + DCHECK(needs_write_barrier); Register register_value = value.AsRegister<Register>(); + Location temp_loc = locations->GetTemp(0); + Register temp = temp_loc.AsRegister<Register>(); + bool can_value_be_null = instruction->GetValueCanBeNull(); NearLabel do_store; if (can_value_be_null) { @@ -6623,7 +6564,6 @@ void InstructionCodeGeneratorX86::VisitArraySet(HArraySet* instruction) { // false negative, in which case we would take the ArraySet // slow path. - Register temp = locations->GetTemp(0).AsRegister<Register>(); // /* HeapReference<Class> */ temp = array->klass_ __ movl(temp, Address(array, class_offset)); codegen_->MaybeRecordImplicitNullCheck(instruction); @@ -6654,29 +6594,24 @@ void InstructionCodeGeneratorX86::VisitArraySet(HArraySet* instruction) { } } + if (instruction->GetWriteBarrierKind() != WriteBarrierKind::kDontEmit) { + DCHECK_EQ(instruction->GetWriteBarrierKind(), WriteBarrierKind::kEmitNoNullCheck) + << " Already null checked so we shouldn't do it again."; + Register card = locations->GetTemp(1).AsRegister<Register>(); + codegen_->MarkGCCard(temp, + card, + array, + value.AsRegister<Register>(), + /* emit_null_check= */ false); + } + if (can_value_be_null) { DCHECK(do_store.IsLinked()); __ Bind(&do_store); } - if (needs_write_barrier) { - // TODO(solanes): The WriteBarrierKind::kEmitNotBeingReliedOn case should be able to skip - // this write barrier when its value is null (without an extra testl since we already - // checked if the value is null for the type check). This will be done as a follow-up since - // it is a runtime optimization that needs extra care. - Register temp = locations->GetTemp(0).AsRegister<Register>(); - Register card = locations->GetTemp(1).AsRegister<Register>(); - codegen_->MarkGCCard(temp, card, array); - } else if (codegen_->ShouldCheckGCCard( - value_type, instruction->GetValue(), write_barrier_kind)) { - Register temp = locations->GetTemp(0).AsRegister<Register>(); - Register card = locations->GetTemp(1).AsRegister<Register>(); - codegen_->CheckGCCardIsValid(temp, card, array); - } - Register source = register_value; if (kPoisonHeapReferences) { - Register temp = locations->GetTemp(0).AsRegister<Register>(); __ movl(temp, register_value); __ PoisonHeapReference(temp); source = temp; diff --git a/compiler/optimizing/code_generator_x86.h b/compiler/optimizing/code_generator_x86.h index 8a4718143b..5b59bfc7e3 100644 --- a/compiler/optimizing/code_generator_x86.h +++ b/compiler/optimizing/code_generator_x86.h @@ -567,20 +567,10 @@ class CodeGeneratorX86 : public CodeGenerator { uint64_t index_in_table) const; void EmitJitRootPatches(uint8_t* code, const uint8_t* roots_data) override; - // Emit a write barrier if: - // A) emit_null_check is false - // B) emit_null_check is true, and value is not null. - void MaybeMarkGCCard( + // Emit a write barrier. + void MarkGCCard( Register temp, Register card, Register object, Register value, bool emit_null_check); - // Emit a write barrier unconditionally. - void MarkGCCard(Register temp, Register card, Register object); - - // Crash if the card table is not valid. This check is only emitted for the CC GC. We assert - // `(!clean || !self->is_gc_marking)`, since the card table should not be set to clean when the CC - // GC is marking for eliminated write barriers. - void CheckGCCardIsValid(Register temp, Register card, Register object); - void GenerateMemoryBarrier(MemBarrierKind kind); Label* GetLabelOf(HBasicBlock* block) const { diff --git a/compiler/optimizing/code_generator_x86_64.cc b/compiler/optimizing/code_generator_x86_64.cc index 50701d4d95..9d010190f7 100644 --- a/compiler/optimizing/code_generator_x86_64.cc +++ b/compiler/optimizing/code_generator_x86_64.cc @@ -5307,8 +5307,7 @@ void InstructionCodeGeneratorX86_64::HandleFieldGet(HInstruction* instruction, } void LocationsBuilderX86_64::HandleFieldSet(HInstruction* instruction, - const FieldInfo& field_info, - WriteBarrierKind write_barrier_kind) { + const FieldInfo& field_info) { DCHECK(instruction->IsInstanceFieldSet() || instruction->IsStaticFieldSet()); LocationSummary* locations = @@ -5316,9 +5315,7 @@ void LocationsBuilderX86_64::HandleFieldSet(HInstruction* instruction, DataType::Type field_type = field_info.GetFieldType(); bool is_volatile = field_info.IsVolatile(); bool needs_write_barrier = - codegen_->StoreNeedsWriteBarrier(field_type, instruction->InputAt(1), write_barrier_kind); - bool check_gc_card = - codegen_->ShouldCheckGCCard(field_type, instruction->InputAt(1), write_barrier_kind); + CodeGenerator::StoreNeedsWriteBarrier(field_type, instruction->InputAt(1)); locations->SetInAt(0, Location::RequiresRegister()); if (DataType::IsFloatingPointType(instruction->InputAt(1)->GetType())) { @@ -5339,7 +5336,7 @@ void LocationsBuilderX86_64::HandleFieldSet(HInstruction* instruction, // TODO(solanes): We could reduce the temp usage but it requires some non-trivial refactoring of // InstructionCodeGeneratorX86_64::HandleFieldSet. - if (needs_write_barrier || check_gc_card) { + if (needs_write_barrier) { // Temporary registers for the write barrier. locations->AddTemp(Location::RequiresRegister()); locations->AddTemp(Location::RequiresRegister()); // Possibly used for reference poisoning too. @@ -5519,35 +5516,16 @@ void InstructionCodeGeneratorX86_64::HandleFieldSet(HInstruction* instruction, codegen_->MaybeRecordImplicitNullCheck(instruction); } - bool needs_write_barrier = - codegen_->StoreNeedsWriteBarrier(field_type, instruction->InputAt(1), write_barrier_kind); - if (needs_write_barrier) { - if (value.IsConstant()) { - DCHECK(value.GetConstant()->IsNullConstant()); - if (write_barrier_kind == WriteBarrierKind::kEmitBeingReliedOn) { - DCHECK_NE(extra_temp_index, 0u); - CpuRegister temp = locations->GetTemp(0).AsRegister<CpuRegister>(); - CpuRegister card = locations->GetTemp(extra_temp_index).AsRegister<CpuRegister>(); - codegen_->MarkGCCard(temp, card, base); - } - } else { - DCHECK_NE(extra_temp_index, 0u); - CpuRegister temp = locations->GetTemp(0).AsRegister<CpuRegister>(); - CpuRegister card = locations->GetTemp(extra_temp_index).AsRegister<CpuRegister>(); - codegen_->MaybeMarkGCCard( - temp, - card, - base, - value.AsRegister<CpuRegister>(), - value_can_be_null && write_barrier_kind == WriteBarrierKind::kEmitNotBeingReliedOn); - } - } else if (codegen_->ShouldCheckGCCard( - field_type, instruction->InputAt(value_index), write_barrier_kind)) { - DCHECK_NE(extra_temp_index, 0u); - DCHECK(value.IsRegister()); + if (CodeGenerator::StoreNeedsWriteBarrier(field_type, instruction->InputAt(value_index)) && + write_barrier_kind != WriteBarrierKind::kDontEmit) { CpuRegister temp = locations->GetTemp(0).AsRegister<CpuRegister>(); CpuRegister card = locations->GetTemp(extra_temp_index).AsRegister<CpuRegister>(); - codegen_->CheckGCCardIsValid(temp, card, base); + codegen_->MarkGCCard( + temp, + card, + base, + value.AsRegister<CpuRegister>(), + value_can_be_null && write_barrier_kind == WriteBarrierKind::kEmitWithNullCheck); } if (is_volatile) { @@ -5581,7 +5559,7 @@ void InstructionCodeGeneratorX86_64::HandleFieldSet(HInstruction* instruction, } void LocationsBuilderX86_64::VisitInstanceFieldSet(HInstanceFieldSet* instruction) { - HandleFieldSet(instruction, instruction->GetFieldInfo(), instruction->GetWriteBarrierKind()); + HandleFieldSet(instruction, instruction->GetFieldInfo()); } void InstructionCodeGeneratorX86_64::VisitInstanceFieldSet(HInstanceFieldSet* instruction) { @@ -5608,7 +5586,7 @@ void InstructionCodeGeneratorX86_64::VisitStaticFieldGet(HStaticFieldGet* instru } void LocationsBuilderX86_64::VisitStaticFieldSet(HStaticFieldSet* instruction) { - HandleFieldSet(instruction, instruction->GetFieldInfo(), instruction->GetWriteBarrierKind()); + HandleFieldSet(instruction, instruction->GetFieldInfo()); } void InstructionCodeGeneratorX86_64::VisitStaticFieldSet(HStaticFieldSet* instruction) { @@ -5829,11 +5807,8 @@ void InstructionCodeGeneratorX86_64::VisitArrayGet(HArrayGet* instruction) { void LocationsBuilderX86_64::VisitArraySet(HArraySet* instruction) { DataType::Type value_type = instruction->GetComponentType(); - WriteBarrierKind write_barrier_kind = instruction->GetWriteBarrierKind(); bool needs_write_barrier = - codegen_->StoreNeedsWriteBarrier(value_type, instruction->GetValue(), write_barrier_kind); - bool check_gc_card = - codegen_->ShouldCheckGCCard(value_type, instruction->GetValue(), write_barrier_kind); + CodeGenerator::StoreNeedsWriteBarrier(value_type, instruction->GetValue()); bool needs_type_check = instruction->NeedsTypeCheck(); LocationSummary* locations = new (GetGraph()->GetAllocator()) LocationSummary( @@ -5848,16 +5823,13 @@ void LocationsBuilderX86_64::VisitArraySet(HArraySet* instruction) { locations->SetInAt(2, Location::RegisterOrConstant(instruction->InputAt(2))); } - if (needs_write_barrier || check_gc_card) { - // Used by reference poisoning, type checking, emitting write barrier, or checking write - // barrier. - locations->AddTemp(Location::RequiresRegister()); - // Only used when emitting a write barrier, or when checking for the card table. - locations->AddTemp(Location::RequiresRegister()); - } else if ((kPoisonHeapReferences && value_type == DataType::Type::kReference) || - instruction->NeedsTypeCheck()) { - // Used for poisoning or type checking. + if (needs_write_barrier) { + // Used by reference poisoning or emitting write barrier. locations->AddTemp(Location::RequiresRegister()); + if (instruction->GetWriteBarrierKind() != WriteBarrierKind::kDontEmit) { + // Only used when emitting a write barrier. + locations->AddTemp(Location::RequiresRegister()); + } } } @@ -5869,9 +5841,8 @@ void InstructionCodeGeneratorX86_64::VisitArraySet(HArraySet* instruction) { Location value = locations->InAt(2); DataType::Type value_type = instruction->GetComponentType(); bool needs_type_check = instruction->NeedsTypeCheck(); - const WriteBarrierKind write_barrier_kind = instruction->GetWriteBarrierKind(); bool needs_write_barrier = - codegen_->StoreNeedsWriteBarrier(value_type, instruction->GetValue(), write_barrier_kind); + CodeGenerator::StoreNeedsWriteBarrier(value_type, instruction->GetValue()); switch (value_type) { case DataType::Type::kBool: @@ -5912,19 +5883,16 @@ void InstructionCodeGeneratorX86_64::VisitArraySet(HArraySet* instruction) { DCHECK(value.IsConstant()) << value; __ movl(address, Immediate(0)); codegen_->MaybeRecordImplicitNullCheck(instruction); - if (write_barrier_kind == WriteBarrierKind::kEmitBeingReliedOn) { - // We need to set a write barrier here even though we are writing null, since this write - // barrier is being relied on. - DCHECK(needs_write_barrier); - CpuRegister temp = locations->GetTemp(0).AsRegister<CpuRegister>(); - CpuRegister card = locations->GetTemp(1).AsRegister<CpuRegister>(); - codegen_->MarkGCCard(temp, card, array); - } + DCHECK(!needs_write_barrier); DCHECK(!needs_type_check); break; } + DCHECK(needs_write_barrier); CpuRegister register_value = value.AsRegister<CpuRegister>(); + Location temp_loc = locations->GetTemp(0); + CpuRegister temp = temp_loc.AsRegister<CpuRegister>(); + bool can_value_be_null = instruction->GetValueCanBeNull(); NearLabel do_store; if (can_value_be_null) { @@ -5949,7 +5917,6 @@ void InstructionCodeGeneratorX86_64::VisitArraySet(HArraySet* instruction) { // false negative, in which case we would take the ArraySet // slow path. - CpuRegister temp = locations->GetTemp(0).AsRegister<CpuRegister>(); // /* HeapReference<Class> */ temp = array->klass_ __ movl(temp, Address(array, class_offset)); codegen_->MaybeRecordImplicitNullCheck(instruction); @@ -5980,30 +5947,24 @@ void InstructionCodeGeneratorX86_64::VisitArraySet(HArraySet* instruction) { } } + if (instruction->GetWriteBarrierKind() != WriteBarrierKind::kDontEmit) { + DCHECK_EQ(instruction->GetWriteBarrierKind(), WriteBarrierKind::kEmitNoNullCheck) + << " Already null checked so we shouldn't do it again."; + CpuRegister card = locations->GetTemp(1).AsRegister<CpuRegister>(); + codegen_->MarkGCCard(temp, + card, + array, + value.AsRegister<CpuRegister>(), + /* emit_null_check= */ false); + } + if (can_value_be_null) { DCHECK(do_store.IsLinked()); __ Bind(&do_store); } - if (needs_write_barrier) { - // TODO(solanes): The WriteBarrierKind::kEmitNotBeingReliedOn case should be able to skip - // this write barrier when its value is null (without an extra testl since we already - // checked if the value is null for the type check). This will be done as a follow-up since - // it is a runtime optimization that needs extra care. - CpuRegister temp = locations->GetTemp(0).AsRegister<CpuRegister>(); - CpuRegister card = locations->GetTemp(1).AsRegister<CpuRegister>(); - codegen_->MarkGCCard(temp, card, array); - } else if (codegen_->ShouldCheckGCCard( - value_type, instruction->GetValue(), write_barrier_kind)) { - CpuRegister temp = locations->GetTemp(0).AsRegister<CpuRegister>(); - CpuRegister card = locations->GetTemp(1).AsRegister<CpuRegister>(); - codegen_->CheckGCCardIsValid(temp, card, array); - } - Location source = value; if (kPoisonHeapReferences) { - Location temp_loc = locations->GetTemp(0); - CpuRegister temp = temp_loc.AsRegister<CpuRegister>(); __ movl(temp, register_value); __ PoisonHeapReference(temp); source = temp_loc; @@ -6189,28 +6150,21 @@ void InstructionCodeGeneratorX86_64::VisitBoundsCheck(HBoundsCheck* instruction) } } -void CodeGeneratorX86_64::MaybeMarkGCCard(CpuRegister temp, - CpuRegister card, - CpuRegister object, - CpuRegister value, - bool emit_null_check) { +void CodeGeneratorX86_64::MarkGCCard(CpuRegister temp, + CpuRegister card, + CpuRegister object, + CpuRegister value, + bool emit_null_check) { NearLabel is_null; if (emit_null_check) { __ testl(value, value); __ j(kEqual, &is_null); } - MarkGCCard(temp, card, object); - if (emit_null_check) { - __ Bind(&is_null); - } -} - -void CodeGeneratorX86_64::MarkGCCard(CpuRegister temp, CpuRegister card, CpuRegister object) { // Load the address of the card table into `card`. - __ gs()->movq(card, - Address::Absolute(Thread::CardTableOffset<kX86_64PointerSize>().Int32Value(), - /* no_rip= */ true)); - // Calculate the offset (in the card table) of the card corresponding to `object`. + __ gs()->movq(card, Address::Absolute(Thread::CardTableOffset<kX86_64PointerSize>().Int32Value(), + /* no_rip= */ true)); + // Calculate the offset (in the card table) of the card corresponding to + // `object`. __ movq(temp, object); __ shrq(temp, Immediate(gc::accounting::CardTable::kCardShift)); // Write the `art::gc::accounting::CardTable::kCardDirty` value into the @@ -6227,27 +6181,9 @@ void CodeGeneratorX86_64::MarkGCCard(CpuRegister temp, CpuRegister card, CpuRegi // of the card to mark; and 2. to load the `kCardDirty` value) saves a load // (no need to explicitly load `kCardDirty` as an immediate value). __ movb(Address(temp, card, TIMES_1, 0), card); -} - -void CodeGeneratorX86_64::CheckGCCardIsValid(CpuRegister temp, - CpuRegister card, - CpuRegister object) { - NearLabel done; - // Load the address of the card table into `card`. - __ gs()->movq(card, - Address::Absolute(Thread::CardTableOffset<kX86_64PointerSize>().Int32Value(), - /* no_rip= */ true)); - // Calculate the offset (in the card table) of the card corresponding to `object`. - __ movq(temp, object); - __ shrq(temp, Immediate(gc::accounting::CardTable::kCardShift)); - // assert (!clean || !self->is_gc_marking) - __ cmpb(Address(temp, card, TIMES_1, 0), Immediate(gc::accounting::CardTable::kCardClean)); - __ j(kNotEqual, &done); - __ gs()->cmpl(Address::Absolute(Thread::IsGcMarkingOffset<kX86_64PointerSize>(), true), - Immediate(0)); - __ j(kEqual, &done); - __ int3(); - __ Bind(&done); + if (emit_null_check) { + __ Bind(&is_null); + } } void LocationsBuilderX86_64::VisitParallelMove([[maybe_unused]] HParallelMove* instruction) { diff --git a/compiler/optimizing/code_generator_x86_64.h b/compiler/optimizing/code_generator_x86_64.h index b9467f9f10..e4d3eac6bc 100644 --- a/compiler/optimizing/code_generator_x86_64.h +++ b/compiler/optimizing/code_generator_x86_64.h @@ -237,9 +237,7 @@ class LocationsBuilderX86_64 : public HGraphVisitor { void HandleBitwiseOperation(HBinaryOperation* operation); void HandleCondition(HCondition* condition); void HandleShift(HBinaryOperation* operation); - void HandleFieldSet(HInstruction* instruction, - const FieldInfo& field_info, - WriteBarrierKind write_barrier_kind); + void HandleFieldSet(HInstruction* instruction, const FieldInfo& field_info); void HandleFieldGet(HInstruction* instruction); bool CpuHasAvxFeatureFlag(); bool CpuHasAvx2FeatureFlag(); @@ -471,22 +469,12 @@ class CodeGeneratorX86_64 : public CodeGenerator { const X86_64InstructionSetFeatures& GetInstructionSetFeatures() const; - // Emit a write barrier if: - // A) emit_null_check is false - // B) emit_null_check is true, and value is not null. - void MaybeMarkGCCard(CpuRegister temp, - CpuRegister card, - CpuRegister object, - CpuRegister value, - bool emit_null_check); - - // Emit a write barrier unconditionally. - void MarkGCCard(CpuRegister temp, CpuRegister card, CpuRegister object); - - // Crash if the card table is not valid. This check is only emitted for the CC GC. We assert - // `(!clean || !self->is_gc_marking)`, since the card table should not be set to clean when the CC - // GC is marking for eliminated write barriers. - void CheckGCCardIsValid(CpuRegister temp, CpuRegister card, CpuRegister object); + // Emit a write barrier. + void MarkGCCard(CpuRegister temp, + CpuRegister card, + CpuRegister object, + CpuRegister value, + bool emit_null_check); void GenerateMemoryBarrier(MemBarrierKind kind); diff --git a/compiler/optimizing/graph_checker.cc b/compiler/optimizing/graph_checker.cc index ef0a449e1a..e8c94dd6b4 100644 --- a/compiler/optimizing/graph_checker.cc +++ b/compiler/optimizing/graph_checker.cc @@ -30,7 +30,6 @@ #include "mirror/class.h" #include "nodes.h" #include "obj_ptr-inl.h" -#include "optimizing/data_type.h" #include "scoped_thread_state_change-inl.h" #include "subtype_check.h" @@ -1309,26 +1308,6 @@ void GraphChecker::VisitNeg(HNeg* instruction) { } } -HInstruction* HuntForOriginalReference(HInstruction* ref) { - // An original reference can be transformed by instructions like: - // i0 NewArray - // i1 HInstruction(i0) <-- NullCheck, BoundType, IntermediateAddress. - // i2 ArraySet(i1, index, value) - DCHECK(ref != nullptr); - while (ref->IsNullCheck() || ref->IsBoundType() || ref->IsIntermediateAddress()) { - ref = ref->InputAt(0); - } - return ref; -} - -bool IsRemovedWriteBarrier(DataType::Type type, - WriteBarrierKind write_barrier_kind, - HInstruction* value) { - return write_barrier_kind == WriteBarrierKind::kDontEmit && - type == DataType::Type::kReference && - !value->IsNullConstant(); -} - void GraphChecker::VisitArraySet(HArraySet* instruction) { VisitInstruction(instruction); @@ -1342,80 +1321,6 @@ void GraphChecker::VisitArraySet(HArraySet* instruction) { StrBool(instruction->NeedsTypeCheck()), StrBool(instruction->GetSideEffects().Includes(SideEffects::CanTriggerGC())))); } - - if (IsRemovedWriteBarrier(instruction->GetComponentType(), - instruction->GetWriteBarrierKind(), - instruction->GetValue())) { - CheckWriteBarrier(instruction, [](HInstruction* it_instr) { - return it_instr->AsArraySet()->GetWriteBarrierKind(); - }); - } -} - -void GraphChecker::VisitInstanceFieldSet(HInstanceFieldSet* instruction) { - VisitInstruction(instruction); - if (IsRemovedWriteBarrier(instruction->GetFieldType(), - instruction->GetWriteBarrierKind(), - instruction->GetValue())) { - CheckWriteBarrier(instruction, [](HInstruction* it_instr) { - return it_instr->AsInstanceFieldSet()->GetWriteBarrierKind(); - }); - } -} - -void GraphChecker::VisitStaticFieldSet(HStaticFieldSet* instruction) { - VisitInstruction(instruction); - if (IsRemovedWriteBarrier(instruction->GetFieldType(), - instruction->GetWriteBarrierKind(), - instruction->GetValue())) { - CheckWriteBarrier(instruction, [](HInstruction* it_instr) { - return it_instr->AsStaticFieldSet()->GetWriteBarrierKind(); - }); - } -} - -template <typename GetWriteBarrierKind> -void GraphChecker::CheckWriteBarrier(HInstruction* instruction, - GetWriteBarrierKind&& get_write_barrier_kind) { - DCHECK(instruction->IsStaticFieldSet() || - instruction->IsInstanceFieldSet() || - instruction->IsArraySet()); - - // For removed write barriers, we expect that the write barrier they are relying on is: - // A) In the same block, and - // B) There's no instruction between them that can trigger a GC. - HInstruction* object = HuntForOriginalReference(instruction->InputAt(0)); - bool found = false; - for (HBackwardInstructionIterator it(instruction); !it.Done(); it.Advance()) { - if (instruction->GetKind() == it.Current()->GetKind() && - object == HuntForOriginalReference(it.Current()->InputAt(0)) && - get_write_barrier_kind(it.Current()) == WriteBarrierKind::kEmitBeingReliedOn) { - // Found the write barrier we are relying on. - found = true; - break; - } - - // We check the `SideEffects::CanTriggerGC` after failing to find the write barrier since having - // a write barrier that's relying on an ArraySet that can trigger GC is fine because the card - // table is marked after the GC happens. - if (it.Current()->GetSideEffects().Includes(SideEffects::CanTriggerGC())) { - AddError( - StringPrintf("%s %d from block %d was expecting a write barrier and it didn't find " - "any. %s %d can trigger GC", - instruction->DebugName(), - instruction->GetId(), - instruction->GetBlock()->GetBlockId(), - it.Current()->DebugName(), - it.Current()->GetId())); - } - } - - if (!found) { - AddError(StringPrintf("%s %d in block %d didn't find a write barrier to latch onto", - instruction->DebugName(), - instruction->GetId(), - instruction->GetBlock()->GetBlockId())); - } } void GraphChecker::VisitBinaryOperation(HBinaryOperation* op) { diff --git a/compiler/optimizing/graph_checker.h b/compiler/optimizing/graph_checker.h index 5704bcec1a..38e2d7ced9 100644 --- a/compiler/optimizing/graph_checker.h +++ b/compiler/optimizing/graph_checker.h @@ -59,8 +59,6 @@ class GraphChecker : public HGraphDelegateVisitor { void VisitPhi(HPhi* phi) override; void VisitArraySet(HArraySet* instruction) override; - void VisitInstanceFieldSet(HInstanceFieldSet* instruction) override; - void VisitStaticFieldSet(HStaticFieldSet* instruction) override; void VisitBinaryOperation(HBinaryOperation* op) override; void VisitBooleanNot(HBooleanNot* instruction) override; void VisitBoundType(HBoundType* instruction) override; @@ -95,9 +93,6 @@ class GraphChecker : public HGraphDelegateVisitor { void HandleLoop(HBasicBlock* loop_header); void HandleBooleanInput(HInstruction* instruction, size_t input_index); - template <typename GetWriteBarrierKind> - void CheckWriteBarrier(HInstruction* instruction, GetWriteBarrierKind&& get_write_barrier_kind); - // Was the last visit of the graph valid? bool IsValid() const { return errors_.empty(); diff --git a/compiler/optimizing/intrinsics_arm64.cc b/compiler/optimizing/intrinsics_arm64.cc index 2ae44cd4b0..98e526196c 100644 --- a/compiler/optimizing/intrinsics_arm64.cc +++ b/compiler/optimizing/intrinsics_arm64.cc @@ -986,7 +986,7 @@ static void GenUnsafePut(HInvoke* invoke, if (type == DataType::Type::kReference) { bool value_can_be_null = true; // TODO: Worth finding out this information? - codegen->MaybeMarkGCCard(base, value, value_can_be_null); + codegen->MarkGCCard(base, value, value_can_be_null); } } @@ -1457,7 +1457,7 @@ static void GenUnsafeCas(HInvoke* invoke, DataType::Type type, CodeGeneratorARM6 if (type == DataType::Type::kReference) { // Mark card for object assuming new value is stored. bool new_value_can_be_null = true; // TODO: Worth finding out this information? - codegen->MaybeMarkGCCard(base, new_value, new_value_can_be_null); + codegen->MarkGCCard(base, new_value, new_value_can_be_null); } UseScratchRegisterScope temps(masm); @@ -1733,7 +1733,7 @@ static void GenUnsafeGetAndUpdate(HInvoke* invoke, DCHECK(get_and_update_op == GetAndUpdateOp::kSet); // Mark card for object as a new value shall be stored. bool new_value_can_be_null = true; // TODO: Worth finding out this information? - codegen->MaybeMarkGCCard(base, /*value=*/arg, new_value_can_be_null); + codegen->MarkGCCard(base, /*value=*/ arg, new_value_can_be_null); } __ Add(tmp_ptr, base.X(), Operand(offset)); @@ -3394,7 +3394,7 @@ void IntrinsicCodeGeneratorARM64::VisitSystemArrayCopy(HInvoke* invoke) { } // We only need one card marking on the destination array. - codegen_->MarkGCCard(dest.W()); + codegen_->MarkGCCard(dest.W(), Register(), /* emit_null_check= */ false); __ Bind(&skip_copy_and_write_barrier); } @@ -4977,7 +4977,7 @@ static void GenerateVarHandleSet(HInvoke* invoke, } if (CodeGenerator::StoreNeedsWriteBarrier(value_type, invoke->InputAt(value_index))) { - codegen->MaybeMarkGCCard(target.object, Register(value), /* emit_null_check= */ true); + codegen->MarkGCCard(target.object, Register(value), /* emit_null_check= */ true); } if (slow_path != nullptr) { @@ -5141,7 +5141,7 @@ static void GenerateVarHandleCompareAndSetOrExchange(HInvoke* invoke, if (CodeGenerator::StoreNeedsWriteBarrier(value_type, invoke->InputAt(new_value_index))) { // Mark card for object assuming new value is stored. bool new_value_can_be_null = true; // TODO: Worth finding out this information? - codegen->MaybeMarkGCCard(target.object, new_value.W(), new_value_can_be_null); + codegen->MarkGCCard(target.object, new_value.W(), new_value_can_be_null); } // Reuse the `offset` temporary for the pointer to the target location, @@ -5445,7 +5445,7 @@ static void GenerateVarHandleGetAndUpdate(HInvoke* invoke, DCHECK(get_and_update_op == GetAndUpdateOp::kSet); // Mark card for object, the new value shall be stored. bool new_value_can_be_null = true; // TODO: Worth finding out this information? - codegen->MaybeMarkGCCard(target.object, arg.W(), new_value_can_be_null); + codegen->MarkGCCard(target.object, arg.W(), new_value_can_be_null); } // Reuse the `target.offset` temporary for the pointer to the target location, diff --git a/compiler/optimizing/intrinsics_arm_vixl.cc b/compiler/optimizing/intrinsics_arm_vixl.cc index c763721aec..0e2d1fdf53 100644 --- a/compiler/optimizing/intrinsics_arm_vixl.cc +++ b/compiler/optimizing/intrinsics_arm_vixl.cc @@ -1592,7 +1592,7 @@ void IntrinsicCodeGeneratorARMVIXL::VisitSystemArrayCopy(HInvoke* invoke) { } // We only need one card marking on the destination array. - codegen_->MarkGCCard(temp1, temp2, dest); + codegen_->MarkGCCard(temp1, temp2, dest, NoReg, /* emit_null_check= */ false); __ Bind(&skip_copy_and_write_barrier); } @@ -3020,7 +3020,7 @@ static void GenUnsafePut(HInvoke* invoke, UseScratchRegisterScope temps(assembler->GetVIXLAssembler()); vixl32::Register card = temps.Acquire(); bool value_can_be_null = true; // TODO: Worth finding out this information? - codegen->MaybeMarkGCCard(temp, card, base, RegisterFrom(value), value_can_be_null); + codegen->MarkGCCard(temp, card, base, RegisterFrom(value), value_can_be_null); } } @@ -3612,7 +3612,7 @@ static void GenUnsafeCas(HInvoke* invoke, DataType::Type type, CodeGeneratorARMV // Mark card for object assuming new value is stored. Worst case we will mark an unchanged // object and scan the receiver at the next GC for nothing. bool value_can_be_null = true; // TODO: Worth finding out this information? - codegen->MaybeMarkGCCard(tmp_ptr, tmp, base, new_value, value_can_be_null); + codegen->MarkGCCard(tmp_ptr, tmp, base, new_value, value_can_be_null); } vixl32::Label exit_loop_label; @@ -3923,7 +3923,7 @@ static void GenUnsafeGetAndUpdate(HInvoke* invoke, // Mark card for object as a new value shall be stored. bool new_value_can_be_null = true; // TODO: Worth finding out this information? vixl32::Register card = tmp_ptr; // Use the `tmp_ptr` also as the `card` temporary. - codegen->MaybeMarkGCCard(temp, card, base, /*value=*/ RegisterFrom(arg), new_value_can_be_null); + codegen->MarkGCCard(temp, card, base, /*value=*/ RegisterFrom(arg), new_value_can_be_null); } // Note: UnsafeGetAndUpdate operations are sequentially consistent, requiring @@ -4779,7 +4779,7 @@ static void GenerateVarHandleSet(HInvoke* invoke, vixl32::Register temp = target.offset; vixl32::Register card = temps.Acquire(); vixl32::Register value_reg = RegisterFrom(value); - codegen->MaybeMarkGCCard(temp, card, target.object, value_reg, /* emit_null_check= */ true); + codegen->MarkGCCard(temp, card, target.object, value_reg, /* emit_null_check= */ true); } if (slow_path != nullptr) { @@ -5079,8 +5079,7 @@ static void GenerateVarHandleCompareAndSetOrExchange(HInvoke* invoke, vixl32::Register card = tmp_ptr; // Mark card for object assuming new value is stored. bool new_value_can_be_null = true; // TODO: Worth finding out this information? - codegen->MaybeMarkGCCard( - temp, card, target.object, RegisterFrom(new_value), new_value_can_be_null); + codegen->MarkGCCard(temp, card, target.object, RegisterFrom(new_value), new_value_can_be_null); } if (slow_path != nullptr) { @@ -5398,7 +5397,7 @@ static void GenerateVarHandleGetAndUpdate(HInvoke* invoke, vixl32::Register card = tmp_ptr; // Mark card for object assuming new value is stored. bool new_value_can_be_null = true; // TODO: Worth finding out this information? - codegen->MaybeMarkGCCard(temp, card, target.object, RegisterFrom(arg), new_value_can_be_null); + codegen->MarkGCCard(temp, card, target.object, RegisterFrom(arg), new_value_can_be_null); } if (slow_path != nullptr) { diff --git a/compiler/optimizing/intrinsics_riscv64.cc b/compiler/optimizing/intrinsics_riscv64.cc index 698c7e5157..a43ab2f206 100644 --- a/compiler/optimizing/intrinsics_riscv64.cc +++ b/compiler/optimizing/intrinsics_riscv64.cc @@ -1771,7 +1771,7 @@ void IntrinsicCodeGeneratorRISCV64::VisitSystemArrayCopy(HInvoke* invoke) { } // We only need one card marking on the destination array. - codegen_->MarkGCCard(dest); + codegen_->MarkGCCard(dest, XRegister(kNoXRegister), /* emit_null_check= */ false); __ Bind(&skip_copy_and_write_barrier); } @@ -2127,7 +2127,7 @@ static void GenUnsafePut(HInvoke* invoke, if (type == DataType::Type::kReference) { bool value_can_be_null = true; // TODO: Worth finding out this information? - codegen->MaybeMarkGCCard(base, value.AsRegister<XRegister>(), value_can_be_null); + codegen->MarkGCCard(base, value.AsRegister<XRegister>(), value_can_be_null); } } @@ -2348,7 +2348,7 @@ static void GenUnsafeCas(HInvoke* invoke, CodeGeneratorRISCV64* codegen, DataTyp if (type == DataType::Type::kReference) { // Mark card for object assuming new value is stored. bool new_value_can_be_null = true; // TODO: Worth finding out this information? - codegen->MaybeMarkGCCard(object, new_value, new_value_can_be_null); + codegen->MarkGCCard(object, new_value, new_value_can_be_null); } ScratchRegisterScope srs(assembler); @@ -2547,7 +2547,7 @@ static void GenUnsafeGetAndUpdate(HInvoke* invoke, DCHECK(get_and_update_op == GetAndUpdateOp::kSet); // Mark card for object as a new value shall be stored. bool new_value_can_be_null = true; // TODO: Worth finding out this information? - codegen->MaybeMarkGCCard(base, /*value=*/arg, new_value_can_be_null); + codegen->MarkGCCard(base, /*value=*/ arg, new_value_can_be_null); } ScratchRegisterScope srs(assembler); @@ -3332,8 +3332,7 @@ static void GenerateVarHandleSet(HInvoke* invoke, } if (CodeGenerator::StoreNeedsWriteBarrier(value_type, invoke->InputAt(value_index))) { - codegen->MaybeMarkGCCard( - target.object, value.AsRegister<XRegister>(), /* emit_null_check= */ true); + codegen->MarkGCCard(target.object, value.AsRegister<XRegister>(), /* emit_null_check= */ true); } if (slow_path != nullptr) { @@ -3555,8 +3554,7 @@ static void GenerateVarHandleCompareAndSetOrExchange(HInvoke* invoke, if (CodeGenerator::StoreNeedsWriteBarrier(value_type, invoke->InputAt(new_value_index))) { // Mark card for object assuming new value is stored. bool new_value_can_be_null = true; // TODO: Worth finding out this information? - codegen->MaybeMarkGCCard( - target.object, new_value.AsRegister<XRegister>(), new_value_can_be_null); + codegen->MarkGCCard(target.object, new_value.AsRegister<XRegister>(), new_value_can_be_null); } // Scratch registers may be needed for `new_value` and `expected`. @@ -3921,7 +3919,7 @@ static void GenerateVarHandleGetAndUpdate(HInvoke* invoke, DCHECK(get_and_update_op == GetAndUpdateOp::kSet); // Mark card for object, the new value shall be stored. bool new_value_can_be_null = true; // TODO: Worth finding out this information? - codegen->MaybeMarkGCCard(target.object, arg.AsRegister<XRegister>(), new_value_can_be_null); + codegen->MarkGCCard(target.object, arg.AsRegister<XRegister>(), new_value_can_be_null); } size_t data_size = DataType::Size(value_type); diff --git a/compiler/optimizing/intrinsics_x86.cc b/compiler/optimizing/intrinsics_x86.cc index b21f36cfcf..5986a2f3a0 100644 --- a/compiler/optimizing/intrinsics_x86.cc +++ b/compiler/optimizing/intrinsics_x86.cc @@ -2023,11 +2023,11 @@ static void GenUnsafePut(LocationSummary* locations, if (type == DataType::Type::kReference) { bool value_can_be_null = true; // TODO: Worth finding out this information? - codegen->MaybeMarkGCCard(locations->GetTemp(0).AsRegister<Register>(), - locations->GetTemp(1).AsRegister<Register>(), - base, - value_loc.AsRegister<Register>(), - value_can_be_null); + codegen->MarkGCCard(locations->GetTemp(0).AsRegister<Register>(), + locations->GetTemp(1).AsRegister<Register>(), + base, + value_loc.AsRegister<Register>(), + value_can_be_null); } } @@ -2363,7 +2363,7 @@ static void GenReferenceCAS(HInvoke* invoke, bool value_can_be_null = true; // TODO: Worth finding out this information? NearLabel skip_mark_gc_card; __ j(kNotZero, &skip_mark_gc_card); - codegen->MaybeMarkGCCard(temp, temp2, base, value, value_can_be_null); + codegen->MarkGCCard(temp, temp2, base, value, value_can_be_null); __ Bind(&skip_mark_gc_card); // If heap poisoning is enabled, we need to unpoison the values @@ -2629,7 +2629,7 @@ static void GenUnsafeGetAndUpdate(HInvoke* invoke, // Mark card for object as a new value shall be stored. bool new_value_can_be_null = true; // TODO: Worth finding out this information? DCHECK_EQ(temp2, ECX); // Byte register for `MarkGCCard()`. - codegen->MaybeMarkGCCard(temp1, temp2, base, /*value=*/out_reg, new_value_can_be_null); + codegen->MarkGCCard(temp1, temp2, base, /*value=*/ out_reg, new_value_can_be_null); if (kPoisonHeapReferences) { // Use a temp to avoid poisoning base of the field address, which might happen if `out` @@ -3357,7 +3357,7 @@ void IntrinsicCodeGeneratorX86::VisitSystemArrayCopy(HInvoke* invoke) { } // We only need one card marking on the destination array. - codegen_->MarkGCCard(temp1, temp3, dest); + codegen_->MarkGCCard(temp1, temp3, dest, Register(kNoRegister), /* emit_null_check= */ false); __ Bind(&skip_copy_and_write_barrier); } @@ -4176,8 +4176,7 @@ static void GenerateVarHandleSet(HInvoke* invoke, CodeGeneratorX86* codegen) { is_volatile, /* value_can_be_null */ true, // Value can be null, and this write barrier is not being relied on for other sets. - value_type == DataType::Type::kReference ? WriteBarrierKind::kEmitNotBeingReliedOn : - WriteBarrierKind::kDontEmit); + WriteBarrierKind::kEmitWithNullCheck); __ Bind(slow_path->GetExitLabel()); } @@ -4337,7 +4336,8 @@ static void GenerateVarHandleGetAndSet(HInvoke* invoke, CodeGeneratorX86* codege /* always_update_field= */ true, &temp2); } - codegen->MarkGCCard(temp, temp2, reference); + codegen->MarkGCCard( + temp, temp2, reference, value.AsRegister<Register>(), /* emit_null_check= */ false); if (kPoisonHeapReferences) { __ movl(temp, value.AsRegister<Register>()); __ PoisonHeapReference(temp); diff --git a/compiler/optimizing/intrinsics_x86_64.cc b/compiler/optimizing/intrinsics_x86_64.cc index 1876a70541..5177ac4d44 100644 --- a/compiler/optimizing/intrinsics_x86_64.cc +++ b/compiler/optimizing/intrinsics_x86_64.cc @@ -1153,7 +1153,8 @@ void IntrinsicCodeGeneratorX86_64::VisitSystemArrayCopy(HInvoke* invoke) { } // We only need one card marking on the destination array. - codegen_->MarkGCCard(temp1, temp2, dest); + codegen_->MarkGCCard( + temp1, temp2, dest, CpuRegister(kNoRegister), /* emit_null_check= */ false); __ Bind(&skip_copy_and_write_barrier); } @@ -2090,11 +2091,11 @@ static void GenUnsafePut(LocationSummary* locations, DataType::Type type, bool i if (type == DataType::Type::kReference) { bool value_can_be_null = true; // TODO: Worth finding out this information? - codegen->MaybeMarkGCCard(locations->GetTemp(0).AsRegister<CpuRegister>(), - locations->GetTemp(1).AsRegister<CpuRegister>(), - base, - value, - value_can_be_null); + codegen->MarkGCCard(locations->GetTemp(0).AsRegister<CpuRegister>(), + locations->GetTemp(1).AsRegister<CpuRegister>(), + base, + value, + value_can_be_null); } } @@ -2389,7 +2390,7 @@ static void GenCompareAndSetOrExchangeRef(CodeGeneratorX86_64* codegen, // Mark card for object assuming new value is stored. bool value_can_be_null = true; // TODO: Worth finding out this information? - codegen->MaybeMarkGCCard(temp1, temp2, base, value, value_can_be_null); + codegen->MarkGCCard(temp1, temp2, base, value, value_can_be_null); Address field_addr(base, offset, TIMES_1, 0); if (codegen->EmitBakerReadBarrier()) { @@ -2700,7 +2701,7 @@ static void GenUnsafeGetAndUpdate(HInvoke* invoke, // Mark card for object as a new value shall be stored. bool new_value_can_be_null = true; // TODO: Worth finding out this information? - codegen->MaybeMarkGCCard(temp1, temp2, base, /*value=*/out, new_value_can_be_null); + codegen->MarkGCCard(temp1, temp2, base, /*value=*/ out, new_value_can_be_null); if (kPoisonHeapReferences) { // Use a temp to avoid poisoning base of the field address, which might happen if `out` @@ -4158,8 +4159,7 @@ static void GenerateVarHandleSet(HInvoke* invoke, /*value_can_be_null=*/true, byte_swap, // Value can be null, and this write barrier is not being relied on for other sets. - value_type == DataType::Type::kReference ? WriteBarrierKind::kEmitNotBeingReliedOn : - WriteBarrierKind::kDontEmit); + WriteBarrierKind::kEmitWithNullCheck); // setVolatile needs kAnyAny barrier, but HandleFieldSet takes care of that. @@ -4444,7 +4444,7 @@ static void GenerateVarHandleGetAndSet(HInvoke* invoke, &temp1, &temp2); } - codegen->MarkGCCard(temp1, temp2, ref); + codegen->MarkGCCard(temp1, temp2, ref, valreg, /* emit_null_check= */ false); DCHECK_EQ(valreg, out.AsRegister<CpuRegister>()); if (kPoisonHeapReferences) { diff --git a/compiler/optimizing/nodes.h b/compiler/optimizing/nodes.h index 8dd89fa4e4..0efe8f4335 100644 --- a/compiler/optimizing/nodes.h +++ b/compiler/optimizing/nodes.h @@ -2903,10 +2903,6 @@ class HBackwardInstructionIterator : public ValueObject { next_ = Done() ? nullptr : instruction_->GetPrevious(); } - explicit HBackwardInstructionIterator(HInstruction* instruction) : instruction_(instruction) { - next_ = Done() ? nullptr : instruction_->GetPrevious(); - } - bool Done() const { return instruction_ == nullptr; } HInstruction* Current() const { return instruction_; } void Advance() { @@ -6373,13 +6369,19 @@ class HInstanceFieldGet final : public HExpression<1> { }; enum class WriteBarrierKind { - // Emit the write barrier. This write barrier is not being relied on so e.g. codegen can decide to - // skip it if the value stored is null. This is the default behavior. - kEmitNotBeingReliedOn, - // Emit the write barrier. This write barrier is being relied on and must be emitted. - kEmitBeingReliedOn, + // Emit the write barrier, with a runtime optimization which checks if the value that it is being + // set is null. + kEmitWithNullCheck, + // Emit the write barrier, without the runtime null check optimization. This could be set because: + // A) It is a write barrier for an ArraySet (which does the optimization with the type check, so + // it never does the optimization at the write barrier stage) + // B) We know that the input can't be null + // C) This write barrier is actually several write barriers coalesced into one. Potentially we + // could ask if every value is null for a runtime optimization at the cost of compile time / code + // size. At the time of writing it was deemed not worth the effort. + kEmitNoNullCheck, // Skip emitting the write barrier. This could be set because: - // A) The write barrier is not needed (i.e. it is not a reference, or the value is the null + // A) The write barrier is not needed (e.g. it is not a reference, or the value is the null // constant) // B) This write barrier was coalesced into another one so there's no need to emit it. kDontEmit, @@ -6410,7 +6412,7 @@ class HInstanceFieldSet final : public HExpression<2> { declaring_class_def_index, dex_file) { SetPackedFlag<kFlagValueCanBeNull>(true); - SetPackedField<WriteBarrierKindField>(WriteBarrierKind::kEmitNotBeingReliedOn); + SetPackedField<WriteBarrierKindField>(WriteBarrierKind::kEmitWithNullCheck); SetRawInputAt(0, object); SetRawInputAt(1, value); } @@ -6431,11 +6433,8 @@ class HInstanceFieldSet final : public HExpression<2> { void ClearValueCanBeNull() { SetPackedFlag<kFlagValueCanBeNull>(false); } WriteBarrierKind GetWriteBarrierKind() { return GetPackedField<WriteBarrierKindField>(); } void SetWriteBarrierKind(WriteBarrierKind kind) { - DCHECK(kind != WriteBarrierKind::kEmitNotBeingReliedOn) + DCHECK(kind != WriteBarrierKind::kEmitWithNullCheck) << "We shouldn't go back to the original value."; - DCHECK_IMPLIES(kind == WriteBarrierKind::kDontEmit, - GetWriteBarrierKind() != WriteBarrierKind::kEmitBeingReliedOn) - << "If a write barrier was relied on by other write barriers, we cannot skip emitting it."; SetPackedField<WriteBarrierKindField>(kind); } @@ -6577,7 +6576,8 @@ class HArraySet final : public HExpression<3> { SetPackedFlag<kFlagNeedsTypeCheck>(value->GetType() == DataType::Type::kReference); SetPackedFlag<kFlagValueCanBeNull>(true); SetPackedFlag<kFlagStaticTypeOfArrayIsObjectArray>(false); - SetPackedField<WriteBarrierKindField>(WriteBarrierKind::kEmitNotBeingReliedOn); + // ArraySets never do the null check optimization at the write barrier stage. + SetPackedField<WriteBarrierKindField>(WriteBarrierKind::kEmitNoNullCheck); SetRawInputAt(0, array); SetRawInputAt(1, index); SetRawInputAt(2, value); @@ -6653,11 +6653,10 @@ class HArraySet final : public HExpression<3> { WriteBarrierKind GetWriteBarrierKind() { return GetPackedField<WriteBarrierKindField>(); } void SetWriteBarrierKind(WriteBarrierKind kind) { - DCHECK(kind != WriteBarrierKind::kEmitNotBeingReliedOn) + DCHECK(kind != WriteBarrierKind::kEmitNoNullCheck) << "We shouldn't go back to the original value."; - DCHECK_IMPLIES(kind == WriteBarrierKind::kDontEmit, - GetWriteBarrierKind() != WriteBarrierKind::kEmitBeingReliedOn) - << "If a write barrier was relied on by other write barriers, we cannot skip emitting it."; + DCHECK(kind != WriteBarrierKind::kEmitWithNullCheck) + << "We never do the null check optimization for ArraySets."; SetPackedField<WriteBarrierKindField>(kind); } @@ -7517,7 +7516,7 @@ class HStaticFieldSet final : public HExpression<2> { declaring_class_def_index, dex_file) { SetPackedFlag<kFlagValueCanBeNull>(true); - SetPackedField<WriteBarrierKindField>(WriteBarrierKind::kEmitNotBeingReliedOn); + SetPackedField<WriteBarrierKindField>(WriteBarrierKind::kEmitWithNullCheck); SetRawInputAt(0, cls); SetRawInputAt(1, value); } @@ -7535,11 +7534,8 @@ class HStaticFieldSet final : public HExpression<2> { WriteBarrierKind GetWriteBarrierKind() { return GetPackedField<WriteBarrierKindField>(); } void SetWriteBarrierKind(WriteBarrierKind kind) { - DCHECK(kind != WriteBarrierKind::kEmitNotBeingReliedOn) + DCHECK(kind != WriteBarrierKind::kEmitWithNullCheck) << "We shouldn't go back to the original value."; - DCHECK_IMPLIES(kind == WriteBarrierKind::kDontEmit, - GetWriteBarrierKind() != WriteBarrierKind::kEmitBeingReliedOn) - << "If a write barrier was relied on by other write barriers, we cannot skip emitting it."; SetPackedField<WriteBarrierKindField>(kind); } diff --git a/compiler/optimizing/scheduler_arm.cc b/compiler/optimizing/scheduler_arm.cc index 510a0f5496..cafb0f5da6 100644 --- a/compiler/optimizing/scheduler_arm.cc +++ b/compiler/optimizing/scheduler_arm.cc @@ -977,6 +977,8 @@ void SchedulingLatencyVisitorARM::HandleFieldSetLatencies(HInstruction* instruct DCHECK(codegen_ != nullptr); bool is_volatile = field_info.IsVolatile(); DataType::Type field_type = field_info.GetFieldType(); + bool needs_write_barrier = + CodeGenerator::StoreNeedsWriteBarrier(field_type, instruction->InputAt(1)); bool atomic_ldrd_strd = codegen_->GetInstructionSetFeatures().HasAtomicLdrdAndStrd(); switch (field_type) { @@ -995,7 +997,7 @@ void SchedulingLatencyVisitorARM::HandleFieldSetLatencies(HInstruction* instruct case DataType::Type::kInt32: case DataType::Type::kReference: - if (kPoisonHeapReferences && field_type == DataType::Type::kReference) { + if (kPoisonHeapReferences && needs_write_barrier) { last_visited_internal_latency_ += kArmIntegerOpLatency * 2; } last_visited_latency_ = kArmMemoryStoreLatency; diff --git a/compiler/optimizing/write_barrier_elimination.cc b/compiler/optimizing/write_barrier_elimination.cc index 27348cd87d..6182125b74 100644 --- a/compiler/optimizing/write_barrier_elimination.cc +++ b/compiler/optimizing/write_barrier_elimination.cc @@ -21,8 +21,8 @@ #include "base/scoped_arena_containers.h" #include "optimizing/nodes.h" -// TODO(b/310755375, solanes): Enable WBE with the fixes. -constexpr bool kWBEEnabled = true; +// TODO(b/310755375, solanes): Disable WBE while we investigate crashes. +constexpr bool kWBEEnabled = false; namespace art HIDDEN { @@ -58,7 +58,7 @@ class WBEVisitor final : public HGraphVisitor { DCHECK(it->second->AsInstanceFieldSet()->GetWriteBarrierKind() != WriteBarrierKind::kDontEmit); DCHECK_EQ(it->second->GetBlock(), instruction->GetBlock()); - it->second->AsInstanceFieldSet()->SetWriteBarrierKind(WriteBarrierKind::kEmitBeingReliedOn); + it->second->AsInstanceFieldSet()->SetWriteBarrierKind(WriteBarrierKind::kEmitNoNullCheck); instruction->SetWriteBarrierKind(WriteBarrierKind::kDontEmit); MaybeRecordStat(stats_, MethodCompilationStat::kRemovedWriteBarrier); } else { @@ -84,7 +84,7 @@ class WBEVisitor final : public HGraphVisitor { DCHECK(it->second->IsStaticFieldSet()); DCHECK(it->second->AsStaticFieldSet()->GetWriteBarrierKind() != WriteBarrierKind::kDontEmit); DCHECK_EQ(it->second->GetBlock(), instruction->GetBlock()); - it->second->AsStaticFieldSet()->SetWriteBarrierKind(WriteBarrierKind::kEmitBeingReliedOn); + it->second->AsStaticFieldSet()->SetWriteBarrierKind(WriteBarrierKind::kEmitNoNullCheck); instruction->SetWriteBarrierKind(WriteBarrierKind::kDontEmit); MaybeRecordStat(stats_, MethodCompilationStat::kRemovedWriteBarrier); } else { @@ -112,7 +112,8 @@ class WBEVisitor final : public HGraphVisitor { DCHECK(it->second->IsArraySet()); DCHECK(it->second->AsArraySet()->GetWriteBarrierKind() != WriteBarrierKind::kDontEmit); DCHECK_EQ(it->second->GetBlock(), instruction->GetBlock()); - it->second->AsArraySet()->SetWriteBarrierKind(WriteBarrierKind::kEmitBeingReliedOn); + // We never skip the null check in ArraySets so that value is already set. + DCHECK(it->second->AsArraySet()->GetWriteBarrierKind() == WriteBarrierKind::kEmitNoNullCheck); instruction->SetWriteBarrierKind(WriteBarrierKind::kDontEmit); MaybeRecordStat(stats_, MethodCompilationStat::kRemovedWriteBarrier); } else { diff --git a/compiler/optimizing/write_barrier_elimination.h b/compiler/optimizing/write_barrier_elimination.h index 1e9ab7b23b..a3769e7421 100644 --- a/compiler/optimizing/write_barrier_elimination.h +++ b/compiler/optimizing/write_barrier_elimination.h @@ -33,7 +33,7 @@ namespace art HIDDEN { // We can keep the write barrier for `inner_obj` and remove the other two. // // In order to do this, we set the WriteBarrierKind of the instruction. The instruction's kind are -// set to kEmitBeingReliedOn (if this write barrier coalesced other write barriers, we don't want to +// set to kEmitNoNullCheck (if this write barrier coalesced other write barriers, we don't want to // perform the null check optimization), or to kDontEmit (if the write barrier as a whole is not // needed). class WriteBarrierElimination : public HOptimization { |