diff options
30 files changed, 973 insertions, 449 deletions
diff --git a/compiler/optimizing/code_generator.cc b/compiler/optimizing/code_generator.cc index b00e7e1873..441a93c38f 100644 --- a/compiler/optimizing/code_generator.cc +++ b/compiler/optimizing/code_generator.cc @@ -158,6 +158,25 @@ 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(); @@ -1608,6 +1627,17 @@ 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 88e5a20240..fbb4f9e21e 100644 --- a/compiler/optimizing/code_generator.h +++ b/compiler/optimizing/code_generator.h @@ -380,6 +380,11 @@ 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(); @@ -503,6 +508,11 @@ 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 9027976165..0332d9bc4b 100644 --- a/compiler/optimizing/code_generator_arm64.cc +++ b/compiler/optimizing/code_generator_arm64.cc @@ -1499,18 +1499,24 @@ void CodeGeneratorARM64::AddLocationAsTemp(Location location, LocationSummary* l } } -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. +void CodeGeneratorARM64::MaybeMarkGCCard(Register object, Register value, bool emit_null_check) { 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. @@ -1526,9 +1532,24 @@ void CodeGeneratorARM64::MarkGCCard(Register object, Register value, bool emit_n // 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())); - if (emit_null_check) { - __ Bind(&done); - } +} + +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); } void CodeGeneratorARM64::SetupBlockedRegisters() const { @@ -2318,12 +2339,16 @@ void InstructionCodeGeneratorARM64::HandleFieldSet(HInstruction* instruction, } } - if (CodeGenerator::StoreNeedsWriteBarrier(field_type, instruction->InputAt(1)) && - write_barrier_kind != WriteBarrierKind::kDontEmit) { - codegen_->MarkGCCard( + const bool needs_write_barrier = + codegen_->StoreNeedsWriteBarrier(field_type, instruction->InputAt(1), write_barrier_kind); + + if (needs_write_barrier) { + codegen_->MaybeMarkGCCard( obj, Register(value), - value_can_be_null && write_barrier_kind == WriteBarrierKind::kEmitWithNullCheck); + value_can_be_null && write_barrier_kind == WriteBarrierKind::kEmitNotBeingReliedOn); + } else if (codegen_->ShouldCheckGCCard(field_type, instruction->InputAt(1), write_barrier_kind)) { + codegen_->CheckGCCardIsValid(obj); } } @@ -2877,8 +2902,9 @@ 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 = - CodeGenerator::StoreNeedsWriteBarrier(value_type, instruction->GetValue()); + codegen_->StoreNeedsWriteBarrier(value_type, instruction->GetValue(), write_barrier_kind); Register array = InputRegisterAt(instruction, 0); CPURegister value = InputCPURegisterOrZeroRegAt(instruction, 2); @@ -2889,13 +2915,17 @@ 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); + UseScratchRegisterScope temps(masm); if (index.IsConstant()) { offset += Int64FromLocation(index) << DataType::SizeShift(value_type); destination = HeapOperand(array, offset); } else { - UseScratchRegisterScope temps(masm); - Register temp = temps.AcquireSameSizeAs(array); + Register temp_dest = temps.AcquireSameSizeAs(array); if (instruction->GetArray()->IsIntermediateAddress()) { // We do not need to compute the intermediate address from the array: the // input instruction has done it already. See the comment in @@ -2904,101 +2934,116 @@ void InstructionCodeGeneratorARM64::VisitArraySet(HArraySet* instruction) { HIntermediateAddress* interm_addr = instruction->GetArray()->AsIntermediateAddress(); DCHECK(interm_addr->GetOffset()->AsIntConstant()->GetValueAsUint64() == offset); } - temp = array; + temp_dest = array; } else { - __ Add(temp, array, offset); + __ Add(temp_dest, array, offset); } - destination = HeapOperand(temp, + destination = HeapOperand(temp_dest, XRegisterFrom(index), LSL, DataType::SizeShift(value_type)); } + + if (kPoisonHeapReferences && value_type == DataType::Type::kReference) { + DCHECK(value.IsW()); + Register temp_src = temps.AcquireW(); + __ Mov(temp_src, value.W()); + GetAssembler()->PoisonHeapReference(temp_src.W()); + source = temp_src; + } + { // Ensure that between store and MaybeRecordImplicitNullCheck there are no pools emitted. EmissionCheckScope guard(GetVIXLAssembler(), kMaxMacroInstructionSizeInBytes); - codegen_->Store(value_type, value, destination); + codegen_->Store(value_type, source, destination); codegen_->MaybeRecordImplicitNullCheck(instruction); } } else { DCHECK(!instruction->GetArray()->IsIntermediateAddress()); - - bool can_value_be_null = instruction->GetValueCanBeNull(); - vixl::aarch64::Label do_store; - if (can_value_be_null) { - __ Cbz(Register(value), &do_store); - } - + bool can_value_be_null = true; SlowPathCodeARM64* slow_path = nullptr; - if (needs_type_check) { - slow_path = new (codegen_->GetScopedAllocator()) ArraySetSlowPathARM64(instruction); - codegen_->AddSlowPath(slow_path); + 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); + } - 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(); + if (needs_type_check) { + slow_path = new (codegen_->GetScopedAllocator()) ArraySetSlowPathARM64(instruction); + codegen_->AddSlowPath(slow_path); - UseScratchRegisterScope temps(masm); - Register temp = temps.AcquireSameSizeAs(array); - Register temp2 = temps.AcquireSameSizeAs(array); + 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(); - // 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. + UseScratchRegisterScope temps(masm); + Register temp = temps.AcquireSameSizeAs(array); + Register temp2 = temps.AcquireSameSizeAs(array); - // /* 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); + // 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 = 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. + // /* 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 = 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->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()); + } } - } - 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); + if (can_value_be_null) { + DCHECK(do_store.IsLinked()); + __ Bind(&do_store); + } } - if (can_value_be_null) { - DCHECK(do_store.IsLinked()); - __ Bind(&do_store); - } + 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. + // TODO(solanes): We can also skip it for known zero values which are not relied on i.e. when + // we have the Zero register as the value. If we do `HuntForOriginalReference` on the value + // we'll resolve this. + codegen_->MarkGCCard(array); UseScratchRegisterScope temps(masm); if (kPoisonHeapReferences) { - Register temp_source = temps.AcquireSameSizeAs(array); - DCHECK(value.IsW()); + DCHECK(value.IsW()); + Register temp_source = temps.AcquireW(); __ Mov(temp_source, value.W()); GetAssembler()->PoisonHeapReference(temp_source); source = temp_source; diff --git a/compiler/optimizing/code_generator_arm64.h b/compiler/optimizing/code_generator_arm64.h index 7ff08f55cb..c78137b6ed 100644 --- a/compiler/optimizing/code_generator_arm64.h +++ b/compiler/optimizing/code_generator_arm64.h @@ -658,10 +658,20 @@ class CodeGeneratorARM64 : public CodeGenerator { const Arm64Assembler& GetAssembler() const override { return assembler_; } vixl::aarch64::MacroAssembler* GetVIXLAssembler() { return GetAssembler()->GetVIXLAssembler(); } - // Emit a write barrier. - void MarkGCCard(vixl::aarch64::Register object, - vixl::aarch64::Register value, - bool emit_null_check); + // 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); void GenerateMemoryBarrier(MemBarrierKind kind); diff --git a/compiler/optimizing/code_generator_arm_vixl.cc b/compiler/optimizing/code_generator_arm_vixl.cc index 00c14b0b46..544d35c206 100644 --- a/compiler/optimizing/code_generator_arm_vixl.cc +++ b/compiler/optimizing/code_generator_arm_vixl.cc @@ -22,6 +22,7 @@ #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" @@ -5930,16 +5931,15 @@ void LocationsBuilderARMVIXL::HandleFieldSet(HInstruction* instruction, && is_wide && !codegen_->GetInstructionSetFeatures().HasAtomicLdrdAndStrd(); bool needs_write_barrier = - CodeGenerator::StoreNeedsWriteBarrier(field_type, instruction->InputAt(1)); + codegen_->StoreNeedsWriteBarrier(field_type, instruction->InputAt(1), write_barrier_kind); + bool check_gc_card = + codegen_->ShouldCheckGCCard(field_type, instruction->InputAt(1), write_barrier_kind); + // Temporary registers for the write barrier. // TODO: consider renaming StoreNeedsWriteBarrier to StoreNeedsGCMark. - 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()); - } + if (needs_write_barrier || check_gc_card) { + locations->AddTemp(Location::RequiresRegister()); + locations->AddTemp(Location::RequiresRegister()); } else if (generate_volatile) { // ARM encoding have some additional constraints for ldrexd/strexd: // - registers need to be consecutive @@ -5955,6 +5955,8 @@ 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()); } } @@ -5973,7 +5975,7 @@ void InstructionCodeGeneratorARMVIXL::HandleFieldSet(HInstruction* instruction, DataType::Type field_type = field_info.GetFieldType(); uint32_t offset = field_info.GetFieldOffset().Uint32Value(); bool needs_write_barrier = - CodeGenerator::StoreNeedsWriteBarrier(field_type, instruction->InputAt(1)); + codegen_->StoreNeedsWriteBarrier(field_type, instruction->InputAt(1), write_barrier_kind); if (is_volatile) { codegen_->GenerateMemoryBarrier(MemBarrierKind::kAnyStore); @@ -5996,10 +5998,7 @@ void InstructionCodeGeneratorARMVIXL::HandleFieldSet(HInstruction* instruction, case DataType::Type::kReference: { vixl32::Register value_reg = RegisterFrom(value); - 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. + if (kPoisonHeapReferences) { DCHECK_EQ(field_type, DataType::Type::kReference); value_reg = RegisterFrom(locations->GetTemp(0)); __ Mov(value_reg, RegisterFrom(value)); @@ -6069,16 +6068,19 @@ void InstructionCodeGeneratorARMVIXL::HandleFieldSet(HInstruction* instruction, UNREACHABLE(); } - if (CodeGenerator::StoreNeedsWriteBarrier(field_type, instruction->InputAt(1)) && - write_barrier_kind != WriteBarrierKind::kDontEmit) { + if (needs_write_barrier) { vixl32::Register temp = RegisterFrom(locations->GetTemp(0)); vixl32::Register card = RegisterFrom(locations->GetTemp(1)); - codegen_->MarkGCCard( + codegen_->MaybeMarkGCCard( temp, card, base, RegisterFrom(value), - value_can_be_null && write_barrier_kind == WriteBarrierKind::kEmitWithNullCheck); + 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); } if (is_volatile) { @@ -6831,8 +6833,12 @@ 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 = - CodeGenerator::StoreNeedsWriteBarrier(value_type, instruction->GetValue()); + codegen_->StoreNeedsWriteBarrier(value_type, instruction->GetValue(), write_barrier_kind); + bool check_gc_card = + codegen_->ShouldCheckGCCard(value_type, instruction->GetValue(), write_barrier_kind); + bool needs_type_check = instruction->NeedsTypeCheck(); LocationSummary* locations = new (GetGraph()->GetAllocator()) LocationSummary( @@ -6846,11 +6852,12 @@ void LocationsBuilderARMVIXL::VisitArraySet(HArraySet* instruction) { } else { locations->SetInAt(2, 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. + 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()); locations->AddTemp(Location::RequiresRegister()); + } else if (kPoisonHeapReferences && value_type == DataType::Type::kReference) { locations->AddTemp(Location::RequiresRegister()); } } @@ -6861,8 +6868,9 @@ 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 = - CodeGenerator::StoreNeedsWriteBarrier(value_type, instruction->GetValue()); + codegen_->StoreNeedsWriteBarrier(value_type, instruction->GetValue(), write_barrier_kind); uint32_t data_offset = mirror::Array::DataOffset(DataType::Size(value_type)).Uint32Value(); Location value_loc = locations->InAt(2); @@ -6931,17 +6939,18 @@ void InstructionCodeGeneratorARMVIXL::VisitArraySet(HArraySet* instruction) { codegen_->StoreToShiftedRegOffset(value_type, value_loc, temp, RegisterFrom(index)); } codegen_->MaybeRecordImplicitNullCheck(instruction); - DCHECK(!needs_write_barrier); + 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_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) { @@ -6965,6 +6974,9 @@ 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(), @@ -7002,22 +7014,29 @@ 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) { - // 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. + vixl32::Register temp1 = RegisterFrom(locations->GetTemp(0)); DCHECK_EQ(value_type, DataType::Type::kReference); __ Mov(temp1, value); GetAssembler()->PoisonHeapReference(temp1); @@ -7233,20 +7252,28 @@ void InstructionCodeGeneratorARMVIXL::VisitBoundsCheck(HBoundsCheck* instruction } } -void CodeGeneratorARMVIXL::MarkGCCard(vixl32::Register temp, - vixl32::Register card, - vixl32::Register object, - vixl32::Register value, - bool emit_null_check) { +void CodeGeneratorARMVIXL::MaybeMarkGCCard(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. @@ -7262,9 +7289,24 @@ 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)); - if (emit_null_check) { - __ Bind(&is_null); - } +} + +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); } 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 00e0bfa399..c5b24470bf 100644 --- a/compiler/optimizing/code_generator_arm_vixl.h +++ b/compiler/optimizing/code_generator_arm_vixl.h @@ -615,12 +615,26 @@ class CodeGeneratorARMVIXL : public CodeGenerator { HInstruction* instruction, SlowPathCode* slow_path); - // Emit a write barrier. + // 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. void MarkGCCard(vixl::aarch32::Register temp, vixl::aarch32::Register card, - vixl::aarch32::Register object, - vixl::aarch32::Register value, - bool emit_null_check); + 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); void GenerateMemoryBarrier(MemBarrierKind kind); diff --git a/compiler/optimizing/code_generator_riscv64.cc b/compiler/optimizing/code_generator_riscv64.cc index 0c0b8a9f14..92eef9f824 100644 --- a/compiler/optimizing/code_generator_riscv64.cc +++ b/compiler/optimizing/code_generator_riscv64.cc @@ -2422,16 +2422,21 @@ void InstructionCodeGeneratorRISCV64::HandleShift(HBinaryOperation* instruction) } } -void CodeGeneratorRISCV64::MarkGCCard(XRegister object, - XRegister value, - bool value_can_be_null) { +void CodeGeneratorRISCV64::MaybeMarkGCCard(XRegister object, + XRegister value, + bool value_can_be_null) { Riscv64Label done; - ScratchRegisterScope srs(GetAssembler()); - XRegister card = srs.AllocateXRegister(); - XRegister temp = srs.AllocateXRegister(); 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(); // Load the address of the card table into `card`. __ Loadd(card, TR, Thread::CardTableOffset<kRiscv64PointerSize>().Int32Value()); @@ -2452,9 +2457,27 @@ 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()`. - if (value_can_be_null) { - __ Bind(&done); - } +} + +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); } void LocationsBuilderRISCV64::HandleFieldSet(HInstruction* instruction) { @@ -2483,12 +2506,15 @@ void InstructionCodeGeneratorRISCV64::HandleFieldSet(HInstruction* instruction, codegen_->MaybeRecordImplicitNullCheck(instruction); } - if (CodeGenerator::StoreNeedsWriteBarrier(type, instruction->InputAt(1)) && - write_barrier_kind != WriteBarrierKind::kDontEmit) { - codegen_->MarkGCCard( + bool needs_write_barrier = + codegen_->StoreNeedsWriteBarrier(type, instruction->InputAt(1), write_barrier_kind); + if (needs_write_barrier) { + codegen_->MaybeMarkGCCard( obj, value.AsRegister<XRegister>(), - value_can_be_null && write_barrier_kind == WriteBarrierKind::kEmitWithNullCheck); + value_can_be_null && write_barrier_kind == WriteBarrierKind::kEmitNotBeingReliedOn); + } else if (codegen_->ShouldCheckGCCard(type, instruction->InputAt(1), write_barrier_kind)) { + codegen_->CheckGCCardIsValid(obj); } } @@ -2899,8 +2925,9 @@ 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 = - CodeGenerator::StoreNeedsWriteBarrier(value_type, instruction->GetValue()); + codegen_->StoreNeedsWriteBarrier(value_type, instruction->GetValue(), write_barrier_kind); size_t data_offset = mirror::Array::DataOffset(DataType::Size(value_type)).Uint32Value(); SlowPathCodeRISCV64* slow_path = nullptr; @@ -2961,15 +2988,18 @@ 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 321403f7f2..ba43090590 100644 --- a/compiler/optimizing/code_generator_riscv64.h +++ b/compiler/optimizing/code_generator_riscv64.h @@ -750,7 +750,18 @@ class CodeGeneratorRISCV64 : public CodeGenerator { // artReadBarrierForRootSlow. void GenerateReadBarrierForRootSlow(HInstruction* instruction, Location out, Location root); - void MarkGCCard(XRegister object, XRegister value, bool value_can_be_null); + // 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); // // Heap poisoning. diff --git a/compiler/optimizing/code_generator_x86.cc b/compiler/optimizing/code_generator_x86.cc index 71db5c99af..058ecf7242 100644 --- a/compiler/optimizing/code_generator_x86.cc +++ b/compiler/optimizing/code_generator_x86.cc @@ -44,6 +44,7 @@ #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 { @@ -5872,17 +5873,23 @@ void CodeGeneratorX86::EmitLinkerPatches(ArenaVector<linker::LinkerPatch>* linke DCHECK_EQ(size, linker_patches->size()); } -void CodeGeneratorX86::MarkGCCard( +void CodeGeneratorX86::MaybeMarkGCCard( 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 @@ -5900,9 +5907,23 @@ void CodeGeneratorX86::MarkGCCard( // (no need to explicitly load `kCardDirty` as an immediate value). __ movb(Address(temp, card, TIMES_1, 0), X86ManagedRegister::FromCpuRegister(card).AsByteRegister()); - if (emit_null_check) { - __ Bind(&is_null); - } +} + +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); } void LocationsBuilderX86::HandleFieldGet(HInstruction* instruction, const FieldInfo& field_info) { @@ -6028,14 +6049,17 @@ void LocationsBuilderX86::HandleFieldSet(HInstruction* instruction, } else { locations->SetInAt(1, Location::RegisterOrConstant(instruction->InputAt(1))); - 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()); - } + 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()); } } } @@ -6051,7 +6075,7 @@ void InstructionCodeGeneratorX86::HandleFieldSet(HInstruction* instruction, LocationSummary* locations = instruction->GetLocations(); Location value = locations->InAt(value_index); bool needs_write_barrier = - CodeGenerator::StoreNeedsWriteBarrier(field_type, instruction->InputAt(value_index)); + codegen_->StoreNeedsWriteBarrier(field_type, instruction->InputAt(1), write_barrier_kind); if (is_volatile) { codegen_->GenerateMemoryBarrier(MemBarrierKind::kAnyStore); @@ -6083,15 +6107,19 @@ void InstructionCodeGeneratorX86::HandleFieldSet(HInstruction* instruction, case DataType::Type::kInt32: case DataType::Type::kReference: { - 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); + 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); + } } else if (value.IsConstant()) { int32_t v = CodeGenerator::GetInt32ValueOf(value.GetConstant()); __ movl(field_addr, Immediate(v)); @@ -6160,15 +6188,38 @@ void InstructionCodeGeneratorX86::HandleFieldSet(HInstruction* instruction, codegen_->MaybeRecordImplicitNullCheck(instruction); } - if (needs_write_barrier && write_barrier_kind != WriteBarrierKind::kDontEmit) { + if (needs_write_barrier) { Register temp = locations->GetTemp(0).AsRegister<Register>(); Register card = locations->GetTemp(1).AsRegister<Register>(); - codegen_->MarkGCCard( - temp, - card, - base, - value.AsRegister<Register>(), - value_can_be_null && write_barrier_kind == WriteBarrierKind::kEmitWithNullCheck); + 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); + } } if (is_volatile) { @@ -6449,8 +6500,11 @@ 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 = - CodeGenerator::StoreNeedsWriteBarrier(value_type, instruction->GetValue()); + codegen_->StoreNeedsWriteBarrier(value_type, instruction->GetValue(), write_barrier_kind); + bool check_gc_card = + codegen_->ShouldCheckGCCard(value_type, instruction->GetValue(), write_barrier_kind); bool needs_type_check = instruction->NeedsTypeCheck(); LocationSummary* locations = new (GetGraph()->GetAllocator()) LocationSummary( @@ -6471,13 +6525,14 @@ void LocationsBuilderX86::VisitArraySet(HArraySet* instruction) { } else { locations->SetInAt(2, Location::RegisterOrConstant(instruction->InputAt(2))); } - if (needs_write_barrier) { - // Used by reference poisoning or emitting write barrier. + 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()) { 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)); - } } } @@ -6489,8 +6544,9 @@ 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 = - CodeGenerator::StoreNeedsWriteBarrier(value_type, instruction->GetValue()); + codegen_->StoreNeedsWriteBarrier(value_type, instruction->GetValue(), write_barrier_kind); switch (value_type) { case DataType::Type::kBool: @@ -6530,16 +6586,19 @@ void InstructionCodeGeneratorX86::VisitArraySet(HArraySet* instruction) { DCHECK(value.IsConstant()) << value; __ movl(address, Immediate(0)); codegen_->MaybeRecordImplicitNullCheck(instruction); - DCHECK(!needs_write_barrier); + 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_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) { @@ -6564,6 +6623,7 @@ 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); @@ -6594,24 +6654,29 @@ 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 5b59bfc7e3..8a4718143b 100644 --- a/compiler/optimizing/code_generator_x86.h +++ b/compiler/optimizing/code_generator_x86.h @@ -567,10 +567,20 @@ 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. - void MarkGCCard( + // Emit a write barrier if: + // A) emit_null_check is false + // B) emit_null_check is true, and value is not null. + void MaybeMarkGCCard( 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 9d010190f7..705c14c009 100644 --- a/compiler/optimizing/code_generator_x86_64.cc +++ b/compiler/optimizing/code_generator_x86_64.cc @@ -5307,7 +5307,8 @@ void InstructionCodeGeneratorX86_64::HandleFieldGet(HInstruction* instruction, } void LocationsBuilderX86_64::HandleFieldSet(HInstruction* instruction, - const FieldInfo& field_info) { + const FieldInfo& field_info, + WriteBarrierKind write_barrier_kind) { DCHECK(instruction->IsInstanceFieldSet() || instruction->IsStaticFieldSet()); LocationSummary* locations = @@ -5315,7 +5316,9 @@ void LocationsBuilderX86_64::HandleFieldSet(HInstruction* instruction, DataType::Type field_type = field_info.GetFieldType(); bool is_volatile = field_info.IsVolatile(); bool needs_write_barrier = - CodeGenerator::StoreNeedsWriteBarrier(field_type, instruction->InputAt(1)); + codegen_->StoreNeedsWriteBarrier(field_type, instruction->InputAt(1), write_barrier_kind); + bool check_gc_card = + codegen_->ShouldCheckGCCard(field_type, instruction->InputAt(1), write_barrier_kind); locations->SetInAt(0, Location::RequiresRegister()); if (DataType::IsFloatingPointType(instruction->InputAt(1)->GetType())) { @@ -5335,14 +5338,13 @@ 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) { + // InstructionCodeGeneratorX86_64::HandleFieldSet, GenerateVarHandleSet due to `extra_temp_index`. + if (needs_write_barrier || + check_gc_card || + (kPoisonHeapReferences && field_type == DataType::Type::kReference)) { // Temporary registers for the write barrier. locations->AddTemp(Location::RequiresRegister()); locations->AddTemp(Location::RequiresRegister()); // Possibly used for reference poisoning too. - } else if (kPoisonHeapReferences && field_type == DataType::Type::kReference) { - // Temporary register for the reference poisoning. - locations->AddTemp(Location::RequiresRegister()); } } @@ -5516,16 +5518,35 @@ void InstructionCodeGeneratorX86_64::HandleFieldSet(HInstruction* instruction, codegen_->MaybeRecordImplicitNullCheck(instruction); } - if (CodeGenerator::StoreNeedsWriteBarrier(field_type, instruction->InputAt(value_index)) && - write_barrier_kind != WriteBarrierKind::kDontEmit) { + 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()); CpuRegister temp = locations->GetTemp(0).AsRegister<CpuRegister>(); CpuRegister card = locations->GetTemp(extra_temp_index).AsRegister<CpuRegister>(); - codegen_->MarkGCCard( - temp, - card, - base, - value.AsRegister<CpuRegister>(), - value_can_be_null && write_barrier_kind == WriteBarrierKind::kEmitWithNullCheck); + codegen_->CheckGCCardIsValid(temp, card, base); } if (is_volatile) { @@ -5559,7 +5580,7 @@ void InstructionCodeGeneratorX86_64::HandleFieldSet(HInstruction* instruction, } void LocationsBuilderX86_64::VisitInstanceFieldSet(HInstanceFieldSet* instruction) { - HandleFieldSet(instruction, instruction->GetFieldInfo()); + HandleFieldSet(instruction, instruction->GetFieldInfo(), instruction->GetWriteBarrierKind()); } void InstructionCodeGeneratorX86_64::VisitInstanceFieldSet(HInstanceFieldSet* instruction) { @@ -5586,7 +5607,7 @@ void InstructionCodeGeneratorX86_64::VisitStaticFieldGet(HStaticFieldGet* instru } void LocationsBuilderX86_64::VisitStaticFieldSet(HStaticFieldSet* instruction) { - HandleFieldSet(instruction, instruction->GetFieldInfo()); + HandleFieldSet(instruction, instruction->GetFieldInfo(), instruction->GetWriteBarrierKind()); } void InstructionCodeGeneratorX86_64::VisitStaticFieldSet(HStaticFieldSet* instruction) { @@ -5807,8 +5828,11 @@ 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 = - CodeGenerator::StoreNeedsWriteBarrier(value_type, instruction->GetValue()); + codegen_->StoreNeedsWriteBarrier(value_type, instruction->GetValue(), write_barrier_kind); + bool check_gc_card = + codegen_->ShouldCheckGCCard(value_type, instruction->GetValue(), write_barrier_kind); bool needs_type_check = instruction->NeedsTypeCheck(); LocationSummary* locations = new (GetGraph()->GetAllocator()) LocationSummary( @@ -5823,13 +5847,16 @@ void LocationsBuilderX86_64::VisitArraySet(HArraySet* instruction) { locations->SetInAt(2, Location::RegisterOrConstant(instruction->InputAt(2))); } - if (needs_write_barrier) { - // Used by reference poisoning or emitting write barrier. + 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. locations->AddTemp(Location::RequiresRegister()); - if (instruction->GetWriteBarrierKind() != WriteBarrierKind::kDontEmit) { - // Only used when emitting a write barrier. - locations->AddTemp(Location::RequiresRegister()); - } } } @@ -5841,8 +5868,9 @@ 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 = - CodeGenerator::StoreNeedsWriteBarrier(value_type, instruction->GetValue()); + codegen_->StoreNeedsWriteBarrier(value_type, instruction->GetValue(), write_barrier_kind); switch (value_type) { case DataType::Type::kBool: @@ -5883,16 +5911,19 @@ void InstructionCodeGeneratorX86_64::VisitArraySet(HArraySet* instruction) { DCHECK(value.IsConstant()) << value; __ movl(address, Immediate(0)); codegen_->MaybeRecordImplicitNullCheck(instruction); - DCHECK(!needs_write_barrier); + 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_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) { @@ -5917,6 +5948,7 @@ 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); @@ -5947,24 +5979,30 @@ 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; @@ -6150,21 +6188,28 @@ void InstructionCodeGeneratorX86_64::VisitBoundsCheck(HBoundsCheck* instruction) } } -void CodeGeneratorX86_64::MarkGCCard(CpuRegister temp, - CpuRegister card, - CpuRegister object, - CpuRegister value, - bool emit_null_check) { +void CodeGeneratorX86_64::MaybeMarkGCCard(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 @@ -6181,9 +6226,27 @@ void CodeGeneratorX86_64::MarkGCCard(CpuRegister 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). __ movb(Address(temp, card, TIMES_1, 0), card); - if (emit_null_check) { - __ Bind(&is_null); - } +} + +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); } 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 e4d3eac6bc..b9467f9f10 100644 --- a/compiler/optimizing/code_generator_x86_64.h +++ b/compiler/optimizing/code_generator_x86_64.h @@ -237,7 +237,9 @@ 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); + void HandleFieldSet(HInstruction* instruction, + const FieldInfo& field_info, + WriteBarrierKind write_barrier_kind); void HandleFieldGet(HInstruction* instruction); bool CpuHasAvxFeatureFlag(); bool CpuHasAvx2FeatureFlag(); @@ -469,12 +471,22 @@ class CodeGeneratorX86_64 : public CodeGenerator { const X86_64InstructionSetFeatures& GetInstructionSetFeatures() const; - // Emit a write barrier. - void MarkGCCard(CpuRegister temp, - CpuRegister card, - CpuRegister object, - CpuRegister value, - bool emit_null_check); + // 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); void GenerateMemoryBarrier(MemBarrierKind kind); diff --git a/compiler/optimizing/graph_checker.cc b/compiler/optimizing/graph_checker.cc index d4b0bf2e81..d60ec06097 100644 --- a/compiler/optimizing/graph_checker.cc +++ b/compiler/optimizing/graph_checker.cc @@ -31,6 +31,7 @@ #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" @@ -1273,6 +1274,26 @@ 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); @@ -1286,6 +1307,80 @@ 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 38e2d7ced9..5704bcec1a 100644 --- a/compiler/optimizing/graph_checker.h +++ b/compiler/optimizing/graph_checker.h @@ -59,6 +59,8 @@ 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; @@ -93,6 +95,9 @@ 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 98e526196c..2ae44cd4b0 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->MarkGCCard(base, value, value_can_be_null); + codegen->MaybeMarkGCCard(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->MarkGCCard(base, new_value, new_value_can_be_null); + codegen->MaybeMarkGCCard(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->MarkGCCard(base, /*value=*/ arg, new_value_can_be_null); + codegen->MaybeMarkGCCard(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(), Register(), /* emit_null_check= */ false); + codegen_->MarkGCCard(dest.W()); __ 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->MarkGCCard(target.object, Register(value), /* emit_null_check= */ true); + codegen->MaybeMarkGCCard(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->MarkGCCard(target.object, new_value.W(), new_value_can_be_null); + codegen->MaybeMarkGCCard(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->MarkGCCard(target.object, arg.W(), new_value_can_be_null); + codegen->MaybeMarkGCCard(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 0e2d1fdf53..c763721aec 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, NoReg, /* emit_null_check= */ false); + codegen_->MarkGCCard(temp1, temp2, dest); __ 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->MarkGCCard(temp, card, base, RegisterFrom(value), value_can_be_null); + codegen->MaybeMarkGCCard(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->MarkGCCard(tmp_ptr, tmp, base, new_value, value_can_be_null); + codegen->MaybeMarkGCCard(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->MarkGCCard(temp, card, base, /*value=*/ RegisterFrom(arg), new_value_can_be_null); + codegen->MaybeMarkGCCard(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->MarkGCCard(temp, card, target.object, value_reg, /* emit_null_check= */ true); + codegen->MaybeMarkGCCard(temp, card, target.object, value_reg, /* emit_null_check= */ true); } if (slow_path != nullptr) { @@ -5079,7 +5079,8 @@ 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->MarkGCCard(temp, card, target.object, RegisterFrom(new_value), new_value_can_be_null); + codegen->MaybeMarkGCCard( + temp, card, target.object, RegisterFrom(new_value), new_value_can_be_null); } if (slow_path != nullptr) { @@ -5397,7 +5398,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->MarkGCCard(temp, card, target.object, RegisterFrom(arg), new_value_can_be_null); + codegen->MaybeMarkGCCard(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 a43ab2f206..698c7e5157 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, XRegister(kNoXRegister), /* emit_null_check= */ false); + codegen_->MarkGCCard(dest); __ 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->MarkGCCard(base, value.AsRegister<XRegister>(), value_can_be_null); + codegen->MaybeMarkGCCard(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->MarkGCCard(object, new_value, new_value_can_be_null); + codegen->MaybeMarkGCCard(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->MarkGCCard(base, /*value=*/ arg, new_value_can_be_null); + codegen->MaybeMarkGCCard(base, /*value=*/arg, new_value_can_be_null); } ScratchRegisterScope srs(assembler); @@ -3332,7 +3332,8 @@ static void GenerateVarHandleSet(HInvoke* invoke, } if (CodeGenerator::StoreNeedsWriteBarrier(value_type, invoke->InputAt(value_index))) { - codegen->MarkGCCard(target.object, value.AsRegister<XRegister>(), /* emit_null_check= */ true); + codegen->MaybeMarkGCCard( + target.object, value.AsRegister<XRegister>(), /* emit_null_check= */ true); } if (slow_path != nullptr) { @@ -3554,7 +3555,8 @@ 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->MarkGCCard(target.object, new_value.AsRegister<XRegister>(), new_value_can_be_null); + codegen->MaybeMarkGCCard( + target.object, new_value.AsRegister<XRegister>(), new_value_can_be_null); } // Scratch registers may be needed for `new_value` and `expected`. @@ -3919,7 +3921,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->MarkGCCard(target.object, arg.AsRegister<XRegister>(), new_value_can_be_null); + codegen->MaybeMarkGCCard(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 5986a2f3a0..b21f36cfcf 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->MarkGCCard(locations->GetTemp(0).AsRegister<Register>(), - locations->GetTemp(1).AsRegister<Register>(), - base, - value_loc.AsRegister<Register>(), - value_can_be_null); + codegen->MaybeMarkGCCard(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->MarkGCCard(temp, temp2, base, value, value_can_be_null); + codegen->MaybeMarkGCCard(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->MarkGCCard(temp1, temp2, base, /*value=*/ out_reg, new_value_can_be_null); + codegen->MaybeMarkGCCard(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, Register(kNoRegister), /* emit_null_check= */ false); + codegen_->MarkGCCard(temp1, temp3, dest); __ Bind(&skip_copy_and_write_barrier); } @@ -4176,7 +4176,8 @@ 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. - WriteBarrierKind::kEmitWithNullCheck); + value_type == DataType::Type::kReference ? WriteBarrierKind::kEmitNotBeingReliedOn : + WriteBarrierKind::kDontEmit); __ Bind(slow_path->GetExitLabel()); } @@ -4336,8 +4337,7 @@ static void GenerateVarHandleGetAndSet(HInvoke* invoke, CodeGeneratorX86* codege /* always_update_field= */ true, &temp2); } - codegen->MarkGCCard( - temp, temp2, reference, value.AsRegister<Register>(), /* emit_null_check= */ false); + codegen->MarkGCCard(temp, temp2, reference); 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 5177ac4d44..1876a70541 100644 --- a/compiler/optimizing/intrinsics_x86_64.cc +++ b/compiler/optimizing/intrinsics_x86_64.cc @@ -1153,8 +1153,7 @@ void IntrinsicCodeGeneratorX86_64::VisitSystemArrayCopy(HInvoke* invoke) { } // We only need one card marking on the destination array. - codegen_->MarkGCCard( - temp1, temp2, dest, CpuRegister(kNoRegister), /* emit_null_check= */ false); + codegen_->MarkGCCard(temp1, temp2, dest); __ Bind(&skip_copy_and_write_barrier); } @@ -2091,11 +2090,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->MarkGCCard(locations->GetTemp(0).AsRegister<CpuRegister>(), - locations->GetTemp(1).AsRegister<CpuRegister>(), - base, - value, - value_can_be_null); + codegen->MaybeMarkGCCard(locations->GetTemp(0).AsRegister<CpuRegister>(), + locations->GetTemp(1).AsRegister<CpuRegister>(), + base, + value, + value_can_be_null); } } @@ -2390,7 +2389,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->MarkGCCard(temp1, temp2, base, value, value_can_be_null); + codegen->MaybeMarkGCCard(temp1, temp2, base, value, value_can_be_null); Address field_addr(base, offset, TIMES_1, 0); if (codegen->EmitBakerReadBarrier()) { @@ -2701,7 +2700,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->MarkGCCard(temp1, temp2, base, /*value=*/ out, new_value_can_be_null); + codegen->MaybeMarkGCCard(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` @@ -4159,7 +4158,8 @@ 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. - WriteBarrierKind::kEmitWithNullCheck); + value_type == DataType::Type::kReference ? WriteBarrierKind::kEmitNotBeingReliedOn : + WriteBarrierKind::kDontEmit); // 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, valreg, /* emit_null_check= */ false); + codegen->MarkGCCard(temp1, temp2, ref); DCHECK_EQ(valreg, out.AsRegister<CpuRegister>()); if (kPoisonHeapReferences) { diff --git a/compiler/optimizing/nodes.h b/compiler/optimizing/nodes.h index 0efe8f4335..8dd89fa4e4 100644 --- a/compiler/optimizing/nodes.h +++ b/compiler/optimizing/nodes.h @@ -2903,6 +2903,10 @@ 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() { @@ -6369,19 +6373,13 @@ class HInstanceFieldGet final : public HExpression<1> { }; enum class WriteBarrierKind { - // 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, + // 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, // Skip emitting the write barrier. This could be set because: - // A) The write barrier is not needed (e.g. it is not a reference, or the value is the null + // A) The write barrier is not needed (i.e. 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, @@ -6412,7 +6410,7 @@ class HInstanceFieldSet final : public HExpression<2> { declaring_class_def_index, dex_file) { SetPackedFlag<kFlagValueCanBeNull>(true); - SetPackedField<WriteBarrierKindField>(WriteBarrierKind::kEmitWithNullCheck); + SetPackedField<WriteBarrierKindField>(WriteBarrierKind::kEmitNotBeingReliedOn); SetRawInputAt(0, object); SetRawInputAt(1, value); } @@ -6433,8 +6431,11 @@ class HInstanceFieldSet final : public HExpression<2> { void ClearValueCanBeNull() { SetPackedFlag<kFlagValueCanBeNull>(false); } WriteBarrierKind GetWriteBarrierKind() { return GetPackedField<WriteBarrierKindField>(); } void SetWriteBarrierKind(WriteBarrierKind kind) { - DCHECK(kind != WriteBarrierKind::kEmitWithNullCheck) + DCHECK(kind != WriteBarrierKind::kEmitNotBeingReliedOn) << "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); } @@ -6576,8 +6577,7 @@ class HArraySet final : public HExpression<3> { SetPackedFlag<kFlagNeedsTypeCheck>(value->GetType() == DataType::Type::kReference); SetPackedFlag<kFlagValueCanBeNull>(true); SetPackedFlag<kFlagStaticTypeOfArrayIsObjectArray>(false); - // ArraySets never do the null check optimization at the write barrier stage. - SetPackedField<WriteBarrierKindField>(WriteBarrierKind::kEmitNoNullCheck); + SetPackedField<WriteBarrierKindField>(WriteBarrierKind::kEmitNotBeingReliedOn); SetRawInputAt(0, array); SetRawInputAt(1, index); SetRawInputAt(2, value); @@ -6653,10 +6653,11 @@ class HArraySet final : public HExpression<3> { WriteBarrierKind GetWriteBarrierKind() { return GetPackedField<WriteBarrierKindField>(); } void SetWriteBarrierKind(WriteBarrierKind kind) { - DCHECK(kind != WriteBarrierKind::kEmitNoNullCheck) + DCHECK(kind != WriteBarrierKind::kEmitNotBeingReliedOn) << "We shouldn't go back to the original value."; - DCHECK(kind != WriteBarrierKind::kEmitWithNullCheck) - << "We never do the null check optimization for ArraySets."; + 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); } @@ -7516,7 +7517,7 @@ class HStaticFieldSet final : public HExpression<2> { declaring_class_def_index, dex_file) { SetPackedFlag<kFlagValueCanBeNull>(true); - SetPackedField<WriteBarrierKindField>(WriteBarrierKind::kEmitWithNullCheck); + SetPackedField<WriteBarrierKindField>(WriteBarrierKind::kEmitNotBeingReliedOn); SetRawInputAt(0, cls); SetRawInputAt(1, value); } @@ -7534,8 +7535,11 @@ class HStaticFieldSet final : public HExpression<2> { WriteBarrierKind GetWriteBarrierKind() { return GetPackedField<WriteBarrierKindField>(); } void SetWriteBarrierKind(WriteBarrierKind kind) { - DCHECK(kind != WriteBarrierKind::kEmitWithNullCheck) + DCHECK(kind != WriteBarrierKind::kEmitNotBeingReliedOn) << "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 cafb0f5da6..510a0f5496 100644 --- a/compiler/optimizing/scheduler_arm.cc +++ b/compiler/optimizing/scheduler_arm.cc @@ -977,8 +977,6 @@ 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) { @@ -997,7 +995,7 @@ void SchedulingLatencyVisitorARM::HandleFieldSetLatencies(HInstruction* instruct case DataType::Type::kInt32: case DataType::Type::kReference: - if (kPoisonHeapReferences && needs_write_barrier) { + if (kPoisonHeapReferences && field_type == DataType::Type::kReference) { 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 6182125b74..27348cd87d 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): Disable WBE while we investigate crashes. -constexpr bool kWBEEnabled = false; +// TODO(b/310755375, solanes): Enable WBE with the fixes. +constexpr bool kWBEEnabled = true; 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::kEmitNoNullCheck); + it->second->AsInstanceFieldSet()->SetWriteBarrierKind(WriteBarrierKind::kEmitBeingReliedOn); 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::kEmitNoNullCheck); + it->second->AsStaticFieldSet()->SetWriteBarrierKind(WriteBarrierKind::kEmitBeingReliedOn); instruction->SetWriteBarrierKind(WriteBarrierKind::kDontEmit); MaybeRecordStat(stats_, MethodCompilationStat::kRemovedWriteBarrier); } else { @@ -112,8 +112,7 @@ class WBEVisitor final : public HGraphVisitor { DCHECK(it->second->IsArraySet()); DCHECK(it->second->AsArraySet()->GetWriteBarrierKind() != WriteBarrierKind::kDontEmit); DCHECK_EQ(it->second->GetBlock(), instruction->GetBlock()); - // We never skip the null check in ArraySets so that value is already set. - DCHECK(it->second->AsArraySet()->GetWriteBarrierKind() == WriteBarrierKind::kEmitNoNullCheck); + it->second->AsArraySet()->SetWriteBarrierKind(WriteBarrierKind::kEmitBeingReliedOn); 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 a3769e7421..1e9ab7b23b 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 kEmitNoNullCheck (if this write barrier coalesced other write barriers, we don't want to +// set to kEmitBeingReliedOn (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 { diff --git a/test/2247-checker-write-barrier-elimination/src/Main.java b/test/2247-checker-write-barrier-elimination/src/Main.java index c03ada30b5..edf5bc2eab 100644 --- a/test/2247-checker-write-barrier-elimination/src/Main.java +++ b/test/2247-checker-write-barrier-elimination/src/Main.java @@ -55,13 +55,10 @@ public class Main { } /// CHECK-START: Main Main.$noinline$testInstanceFieldSets(Main, java.lang.Object, java.lang.Object, java.lang.Object) disassembly (after) - /// CHECK: InstanceFieldSet field_name:Main.inner field_type:Reference write_barrier_kind:EmitNoNullCheck + /// CHECK: InstanceFieldSet field_name:Main.inner field_type:Reference write_barrier_kind:EmitBeingReliedOn + /// CHECK: ; card_table /// CHECK: InstanceFieldSet field_name:Main.inner2 field_type:Reference write_barrier_kind:DontEmit /// CHECK: InstanceFieldSet field_name:Main.inner3 field_type:Reference write_barrier_kind:DontEmit - - /// CHECK-START: Main Main.$noinline$testInstanceFieldSets(Main, java.lang.Object, java.lang.Object, java.lang.Object) disassembly (after) - /// CHECK: ; card_table - /// CHECK-NOT: ; card_table private static Main $noinline$testInstanceFieldSets(Main m, Object o, Object o2, Object o3) { m.inner = o; m.inner2 = o2; @@ -70,13 +67,10 @@ public class Main { } /// CHECK-START: void Main.$noinline$testStaticFieldSets(java.lang.Object, java.lang.Object, java.lang.Object) disassembly (after) - /// CHECK: StaticFieldSet field_name:Main.inner_static field_type:Reference write_barrier_kind:EmitNoNullCheck + /// CHECK: StaticFieldSet field_name:Main.inner_static field_type:Reference write_barrier_kind:EmitBeingReliedOn + /// CHECK: ; card_table /// CHECK: StaticFieldSet field_name:Main.inner_static2 field_type:Reference write_barrier_kind:DontEmit /// CHECK: StaticFieldSet field_name:Main.inner_static3 field_type:Reference write_barrier_kind:DontEmit - - /// CHECK-START: void Main.$noinline$testStaticFieldSets(java.lang.Object, java.lang.Object, java.lang.Object) disassembly (after) - /// CHECK: ; card_table - /// CHECK-NOT: ; card_table private static void $noinline$testStaticFieldSets(Object o, Object o2, Object o3) { inner_static = o; inner_static2 = o2; @@ -84,15 +78,12 @@ public class Main { } /// CHECK-START: java.lang.Object[] Main.$noinline$testArraySets(java.lang.Object[], java.lang.Object, java.lang.Object, java.lang.Object) disassembly (after) - /// CHECK: ArraySet needs_type_check:true can_trigger_gc:true write_barrier_kind:EmitNoNullCheck - /// CHECK: ArraySet needs_type_check:true can_trigger_gc:true write_barrier_kind:EmitNoNullCheck - /// CHECK: ArraySet needs_type_check:true can_trigger_gc:true write_barrier_kind:EmitNoNullCheck - - /// CHECK-START: java.lang.Object[] Main.$noinline$testArraySets(java.lang.Object[], java.lang.Object, java.lang.Object, java.lang.Object) disassembly (after) + /// CHECK: ArraySet needs_type_check:true can_trigger_gc:true write_barrier_kind:EmitNotBeingReliedOn /// CHECK: ; card_table + /// CHECK: ArraySet needs_type_check:true can_trigger_gc:true write_barrier_kind:EmitNotBeingReliedOn /// CHECK: ; card_table + /// CHECK: ArraySet needs_type_check:true can_trigger_gc:true write_barrier_kind:EmitNotBeingReliedOn /// CHECK: ; card_table - /// CHECK-NOT: ; card_table private static java.lang.Object[] $noinline$testArraySets( Object[] arr, Object o, Object o2, Object o3) { arr[0] = o; @@ -102,13 +93,10 @@ public class Main { } /// CHECK-START: java.lang.Object[] Main.$noinline$testSwapArray(java.lang.Object[]) disassembly (after) - /// CHECK: ArraySet needs_type_check:false can_trigger_gc:false write_barrier_kind:EmitNoNullCheck + /// CHECK: ArraySet needs_type_check:false can_trigger_gc:false write_barrier_kind:EmitBeingReliedOn + /// CHECK: ; card_table /// CHECK: ArraySet needs_type_check:false can_trigger_gc:false write_barrier_kind:DontEmit /// CHECK: ArraySet needs_type_check:false can_trigger_gc:false write_barrier_kind:DontEmit - - /// CHECK-START: java.lang.Object[] Main.$noinline$testSwapArray(java.lang.Object[]) disassembly (after) - /// CHECK: ; card_table - /// CHECK-NOT: ; card_table private static java.lang.Object[] $noinline$testSwapArray(Object[] arr) { arr[0] = arr[1]; arr[1] = arr[2]; @@ -117,13 +105,10 @@ public class Main { } /// CHECK-START: java.lang.Object[] Main.$noinline$testArraySetsSameRTI() disassembly (after) - /// CHECK: ArraySet needs_type_check:false can_trigger_gc:false write_barrier_kind:EmitNoNullCheck + /// CHECK: ArraySet needs_type_check:false can_trigger_gc:false write_barrier_kind:EmitBeingReliedOn + /// CHECK: ; card_table /// CHECK: ArraySet needs_type_check:false can_trigger_gc:false write_barrier_kind:DontEmit /// CHECK: ArraySet needs_type_check:false can_trigger_gc:false write_barrier_kind:DontEmit - - /// CHECK-START: java.lang.Object[] Main.$noinline$testArraySetsSameRTI() disassembly (after) - /// CHECK: ; card_table - /// CHECK-NOT: ; card_table private static java.lang.Object[] $noinline$testArraySetsSameRTI() { Object[] arr = new Object[3]; arr[0] = inner_static; @@ -134,12 +119,9 @@ public class Main { /// CHECK-START: Main Main.$noinline$testNullInstanceFieldSets(Main, java.lang.Object) disassembly (after) /// CHECK: InstanceFieldSet field_name:Main.inner field_type:Reference write_barrier_kind:DontEmit - /// CHECK: InstanceFieldSet field_name:Main.inner2 field_type:Reference write_barrier_kind:EmitWithNullCheck - /// CHECK: InstanceFieldSet field_name:Main.inner3 field_type:Reference write_barrier_kind:DontEmit - - /// CHECK-START: Main Main.$noinline$testNullInstanceFieldSets(Main, java.lang.Object) disassembly (after) + /// CHECK: InstanceFieldSet field_name:Main.inner2 field_type:Reference write_barrier_kind:EmitNotBeingReliedOn /// CHECK: ; card_table - /// CHECK-NOT: ; card_table + /// CHECK: InstanceFieldSet field_name:Main.inner3 field_type:Reference write_barrier_kind:DontEmit private static Main $noinline$testNullInstanceFieldSets(Main m, Object o) { m.inner = null; m.inner2 = o; @@ -149,12 +131,9 @@ public class Main { /// CHECK-START: void Main.$noinline$testNullStaticFieldSets(java.lang.Object) disassembly (after) /// CHECK: StaticFieldSet field_name:Main.inner_static field_type:Reference write_barrier_kind:DontEmit - /// CHECK: StaticFieldSet field_name:Main.inner_static2 field_type:Reference write_barrier_kind:EmitWithNullCheck - /// CHECK: StaticFieldSet field_name:Main.inner_static3 field_type:Reference write_barrier_kind:DontEmit - - /// CHECK-START: void Main.$noinline$testNullStaticFieldSets(java.lang.Object) disassembly (after) + /// CHECK: StaticFieldSet field_name:Main.inner_static2 field_type:Reference write_barrier_kind:EmitNotBeingReliedOn /// CHECK: ; card_table - /// CHECK-NOT: ; card_table + /// CHECK: StaticFieldSet field_name:Main.inner_static3 field_type:Reference write_barrier_kind:DontEmit private static void $noinline$testNullStaticFieldSets(Object o) { inner_static = null; inner_static2 = o; @@ -163,12 +142,9 @@ public class Main { /// CHECK-START: java.lang.Object[] Main.$noinline$testNullArraySets(java.lang.Object[], java.lang.Object) disassembly (after) /// CHECK: ArraySet needs_type_check:false can_trigger_gc:false write_barrier_kind:DontEmit - /// CHECK: ArraySet needs_type_check:true can_trigger_gc:true write_barrier_kind:EmitNoNullCheck - /// CHECK: ArraySet needs_type_check:false can_trigger_gc:false write_barrier_kind:DontEmit - - /// CHECK-START: java.lang.Object[] Main.$noinline$testNullArraySets(java.lang.Object[], java.lang.Object) disassembly (after) + /// CHECK: ArraySet needs_type_check:true can_trigger_gc:true write_barrier_kind:EmitNotBeingReliedOn /// CHECK: ; card_table - /// CHECK-NOT: ; card_table + /// CHECK: ArraySet needs_type_check:false can_trigger_gc:false write_barrier_kind:DontEmit private static Object[] $noinline$testNullArraySets(Object[] arr, Object o) { arr[0] = null; arr[1] = o; @@ -177,18 +153,11 @@ public class Main { } /// CHECK-START: Main Main.$noinline$testInstanceFieldSetsMultipleReceivers(Main, java.lang.Object, java.lang.Object, java.lang.Object) disassembly (after) - // There are two extra card_tables for the initialization of the MultipleObject. - /// CHECK: InstanceFieldSet field_name:MultipleObject.inner field_type:Reference write_barrier_kind:EmitNoNullCheck - /// CHECK: InstanceFieldSet field_name:MultipleObject.inner field_type:Reference write_barrier_kind:EmitWithNullCheck - /// CHECK: InstanceFieldSet field_name:MultipleObject.inner2 field_type:Reference write_barrier_kind:DontEmit - - // Each one of the two NewInstance instructions have their own `card_table` reference. - /// CHECK-START: Main Main.$noinline$testInstanceFieldSetsMultipleReceivers(Main, java.lang.Object, java.lang.Object, java.lang.Object) disassembly (after) + /// CHECK: InstanceFieldSet field_name:MultipleObject.inner field_type:Reference write_barrier_kind:EmitBeingReliedOn /// CHECK: ; card_table + /// CHECK: InstanceFieldSet field_name:MultipleObject.inner field_type:Reference write_barrier_kind:EmitNotBeingReliedOn /// CHECK: ; card_table - /// CHECK: ; card_table - /// CHECK: ; card_table - /// CHECK-NOT: ; card_table + /// CHECK: InstanceFieldSet field_name:MultipleObject.inner2 field_type:Reference write_barrier_kind:DontEmit private static Main $noinline$testInstanceFieldSetsMultipleReceivers( Main m, Object o, Object o2, Object o3) throws Error { m.mo = new MultipleObject(); @@ -204,14 +173,11 @@ public class Main { } /// CHECK-START: void Main.$noinline$testStaticFieldSetsMultipleReceivers(java.lang.Object, java.lang.Object, java.lang.Object) disassembly (after) - /// CHECK: StaticFieldSet field_name:MultipleObject.inner_static field_type:Reference write_barrier_kind:EmitWithNullCheck - /// CHECK: StaticFieldSet field_name:Main.inner_static2 field_type:Reference write_barrier_kind:EmitNoNullCheck - /// CHECK: StaticFieldSet field_name:Main.inner_static3 field_type:Reference write_barrier_kind:DontEmit - - /// CHECK-START: void Main.$noinline$testStaticFieldSetsMultipleReceivers(java.lang.Object, java.lang.Object, java.lang.Object) disassembly (after) + /// CHECK: StaticFieldSet field_name:MultipleObject.inner_static field_type:Reference write_barrier_kind:EmitNotBeingReliedOn /// CHECK: ; card_table + /// CHECK: StaticFieldSet field_name:Main.inner_static2 field_type:Reference write_barrier_kind:EmitBeingReliedOn /// CHECK: ; card_table - /// CHECK-NOT: ; card_table + /// CHECK: StaticFieldSet field_name:Main.inner_static3 field_type:Reference write_barrier_kind:DontEmit private static void $noinline$testStaticFieldSetsMultipleReceivers( Object o, Object o2, Object o3) { MultipleObject.inner_static = o; @@ -221,20 +187,15 @@ public class Main { /// CHECK-START: java.lang.Object[][] Main.$noinline$testArraySetsMultipleReceiversSameRTI() disassembly (after) // Initializing the values - /// CHECK: ArraySet needs_type_check:false can_trigger_gc:false write_barrier_kind:EmitNoNullCheck - /// CHECK: ArraySet needs_type_check:false can_trigger_gc:false write_barrier_kind:EmitNoNullCheck - /// CHECK: ArraySet needs_type_check:false can_trigger_gc:false write_barrier_kind:DontEmit - // Setting the `array_of_arrays`. - /// CHECK: ArraySet needs_type_check:false can_trigger_gc:false write_barrier_kind:EmitNoNullCheck - /// CHECK: ArraySet needs_type_check:false can_trigger_gc:false write_barrier_kind:DontEmit - - /// CHECK-START: java.lang.Object[][] Main.$noinline$testArraySetsMultipleReceiversSameRTI() disassembly (after) - // Two array sets can't eliminate the write barrier + /// CHECK: ArraySet needs_type_check:false can_trigger_gc:false write_barrier_kind:EmitBeingReliedOn /// CHECK: ; card_table + /// CHECK: ArraySet needs_type_check:false can_trigger_gc:false write_barrier_kind:EmitNotBeingReliedOn /// CHECK: ; card_table - // One write barrier for the array of arrays' sets + /// CHECK: ArraySet needs_type_check:false can_trigger_gc:false write_barrier_kind:DontEmit + // Setting the `array_of_arrays`. + /// CHECK: ArraySet needs_type_check:false can_trigger_gc:false write_barrier_kind:EmitBeingReliedOn /// CHECK: ; card_table - /// CHECK-NOT: ; card_table + /// CHECK: ArraySet needs_type_check:false can_trigger_gc:false write_barrier_kind:DontEmit private static java.lang.Object[][] $noinline$testArraySetsMultipleReceiversSameRTI() { Object[] arr = new Object[3]; Object[] other_arr = new Object[3]; @@ -251,17 +212,14 @@ public class Main { private static void $noinline$emptyMethod() {} /// CHECK-START: Main Main.$noinline$testInstanceFieldSetsBlocked(Main, java.lang.Object, java.lang.Object, java.lang.Object) disassembly (after) - /// CHECK: InstanceFieldSet field_name:Main.inner field_type:Reference write_barrier_kind:EmitWithNullCheck - /// CHECK: InvokeStaticOrDirect method_name:Main.$noinline$emptyMethod - /// CHECK: InstanceFieldSet field_name:Main.inner2 field_type:Reference write_barrier_kind:EmitWithNullCheck - /// CHECK: MonitorOperation kind:enter - /// CHECK: InstanceFieldSet field_name:Main.inner3 field_type:Reference write_barrier_kind:EmitWithNullCheck - - /// CHECK-START: Main Main.$noinline$testInstanceFieldSetsBlocked(Main, java.lang.Object, java.lang.Object, java.lang.Object) disassembly (after) + /// CHECK: InstanceFieldSet field_name:Main.inner field_type:Reference write_barrier_kind:EmitNotBeingReliedOn /// CHECK: ; card_table + /// CHECK: InvokeStaticOrDirect method_name:Main.$noinline$emptyMethod + /// CHECK: InstanceFieldSet field_name:Main.inner2 field_type:Reference write_barrier_kind:EmitNotBeingReliedOn /// CHECK: ; card_table + /// CHECK: MonitorOperation kind:enter + /// CHECK: InstanceFieldSet field_name:Main.inner3 field_type:Reference write_barrier_kind:EmitNotBeingReliedOn /// CHECK: ; card_table - /// CHECK-NOT: ; card_table private static Main $noinline$testInstanceFieldSetsBlocked( Main m, Object o, Object o2, Object o3) { m.inner = o; @@ -274,17 +232,14 @@ public class Main { } /// CHECK-START: void Main.$noinline$testStaticFieldSetsBlocked(Main, java.lang.Object, java.lang.Object, java.lang.Object) disassembly (after) - /// CHECK: StaticFieldSet field_name:Main.inner_static field_type:Reference write_barrier_kind:EmitWithNullCheck - /// CHECK: InvokeStaticOrDirect method_name:Main.$noinline$emptyMethod - /// CHECK: StaticFieldSet field_name:Main.inner_static2 field_type:Reference write_barrier_kind:EmitWithNullCheck - /// CHECK: MonitorOperation kind:enter - /// CHECK: StaticFieldSet field_name:Main.inner_static3 field_type:Reference write_barrier_kind:EmitWithNullCheck - - /// CHECK-START: void Main.$noinline$testStaticFieldSetsBlocked(Main, java.lang.Object, java.lang.Object, java.lang.Object) disassembly (after) + /// CHECK: StaticFieldSet field_name:Main.inner_static field_type:Reference write_barrier_kind:EmitNotBeingReliedOn /// CHECK: ; card_table + /// CHECK: InvokeStaticOrDirect method_name:Main.$noinline$emptyMethod + /// CHECK: StaticFieldSet field_name:Main.inner_static2 field_type:Reference write_barrier_kind:EmitNotBeingReliedOn /// CHECK: ; card_table + /// CHECK: MonitorOperation kind:enter + /// CHECK: StaticFieldSet field_name:Main.inner_static3 field_type:Reference write_barrier_kind:EmitNotBeingReliedOn /// CHECK: ; card_table - /// CHECK-NOT: ; card_table private static void $noinline$testStaticFieldSetsBlocked( Main m, Object o, Object o2, Object o3) { inner_static = o; @@ -296,17 +251,14 @@ public class Main { } /// CHECK-START: java.lang.Object[] Main.$noinline$testArraySetsSameRTIBlocked(Main) disassembly (after) - /// CHECK: ArraySet needs_type_check:false can_trigger_gc:false write_barrier_kind:EmitNoNullCheck - /// CHECK: InvokeStaticOrDirect method_name:Main.$noinline$emptyMethod - /// CHECK: ArraySet needs_type_check:false can_trigger_gc:false write_barrier_kind:EmitNoNullCheck - /// CHECK: MonitorOperation kind:enter - /// CHECK: ArraySet needs_type_check:false can_trigger_gc:false write_barrier_kind:EmitNoNullCheck - - /// CHECK-START: java.lang.Object[] Main.$noinline$testArraySetsSameRTIBlocked(Main) disassembly (after) + /// CHECK: ArraySet needs_type_check:false can_trigger_gc:false write_barrier_kind:EmitNotBeingReliedOn /// CHECK: ; card_table + /// CHECK: InvokeStaticOrDirect method_name:Main.$noinline$emptyMethod + /// CHECK: ArraySet needs_type_check:false can_trigger_gc:false write_barrier_kind:EmitNotBeingReliedOn /// CHECK: ; card_table + /// CHECK: MonitorOperation kind:enter + /// CHECK: ArraySet needs_type_check:false can_trigger_gc:false write_barrier_kind:EmitNotBeingReliedOn /// CHECK: ; card_table - /// CHECK-NOT: ; card_table private static java.lang.Object[] $noinline$testArraySetsSameRTIBlocked(Main m) { Object[] arr = new Object[3]; arr[0] = inner_static; diff --git a/test/2272-checker-codegen-honor-write-barrier-kind/expected-stderr.txt b/test/2272-checker-codegen-honor-write-barrier-kind/expected-stderr.txt new file mode 100644 index 0000000000..e69de29bb2 --- /dev/null +++ b/test/2272-checker-codegen-honor-write-barrier-kind/expected-stderr.txt diff --git a/test/2272-checker-codegen-honor-write-barrier-kind/expected-stdout.txt b/test/2272-checker-codegen-honor-write-barrier-kind/expected-stdout.txt new file mode 100644 index 0000000000..e69de29bb2 --- /dev/null +++ b/test/2272-checker-codegen-honor-write-barrier-kind/expected-stdout.txt diff --git a/test/2272-checker-codegen-honor-write-barrier-kind/info.txt b/test/2272-checker-codegen-honor-write-barrier-kind/info.txt new file mode 100644 index 0000000000..455bb77de3 --- /dev/null +++ b/test/2272-checker-codegen-honor-write-barrier-kind/info.txt @@ -0,0 +1,3 @@ +Two regression tests: +1) Regression test to honor the write barrier kind and set the dirty bit. +2) Regression test for skipping a needed write barrier at runtime. diff --git a/test/2272-checker-codegen-honor-write-barrier-kind/run.py b/test/2272-checker-codegen-honor-write-barrier-kind/run.py new file mode 100644 index 0000000000..ebdb9b5896 --- /dev/null +++ b/test/2272-checker-codegen-honor-write-barrier-kind/run.py @@ -0,0 +1,19 @@ +#! /bin/bash +# +# Copyright (C) 2023 The Android Open Source Project +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +def run(ctx, args): + # Limit the managed heap to 16 MiB to force more garbage collections. + ctx.default_run(args, runtime_option=["-Xmx16m"]) diff --git a/test/2272-checker-codegen-honor-write-barrier-kind/src/Main.java b/test/2272-checker-codegen-honor-write-barrier-kind/src/Main.java new file mode 100644 index 0000000000..f07286b5b9 --- /dev/null +++ b/test/2272-checker-codegen-honor-write-barrier-kind/src/Main.java @@ -0,0 +1,109 @@ +/* + * Copyright (C) 2024 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +public class Main { + public static void main(String[] args) { + $noinline$testHonorWriteBarrier(); + $noinline$testDontSkipWriteBarrier(); + } + + public static void $noinline$testHonorWriteBarrier() { + String[] arr = {"Hello", "World"}; + // We first run the gc to make sure the card is clean. + Runtime.getRuntime().gc(); + // Continually call $noinline$testArraySetsHonorWriteBarrier while allocating over 64 MiB of + // memory (with heap size limited to 16 MiB), in order to increase memory pressure and + // eventually trigger a concurrent garbage collection, which will start by putting the GC in + // marking mode to trigger the bug. + for (int i = 0; i != 64 * 1024; ++i) { + $noinline$allocateAtLeast1KiB(); + $noinline$testArraySetsHonorWriteBarrier(arr, "Universe"); + } + } + + // When the bug was present, $noinline$testArraySetsHonorWriteBarrier would never set the card + // as dirty (which is incorrect). arr[1]'s' write barrier depends on arr[0]'s write barrier. The + // disappeared BoundType in prepare_for_register_allocation shouldn't skip marking the card + // dirty. + + /// CHECK-START: java.lang.String[] Main.$noinline$testArraySetsHonorWriteBarrier(java.lang.String[], java.lang.String) prepare_for_register_allocation (before) + /// CHECK: <<Null:l\d+>> NullConstant + /// CHECK: <<BT:l\d+>> BoundType [<<Null>>] + /// CHECK: ArraySet [<<arr:l\d+>>,<<index:i\d+>>,<<BT>>] value_can_be_null:true needs_type_check:false can_trigger_gc:false write_barrier_kind:EmitBeingReliedOn + /// CHECK: ArraySet value_can_be_null:true needs_type_check:false can_trigger_gc:false write_barrier_kind:DontEmit + + /// CHECK-START: java.lang.String[] Main.$noinline$testArraySetsHonorWriteBarrier(java.lang.String[], java.lang.String) prepare_for_register_allocation (after) + /// CHECK: <<Null:l\d+>> NullConstant + /// CHECK: ArraySet [<<arr:l\d+>>,<<index:i\d+>>,<<Null>>] value_can_be_null:true needs_type_check:false can_trigger_gc:false write_barrier_kind:EmitBeingReliedOn + /// CHECK: ArraySet value_can_be_null:true needs_type_check:false can_trigger_gc:false write_barrier_kind:DontEmit + + /// CHECK-START: java.lang.String[] Main.$noinline$testArraySetsHonorWriteBarrier(java.lang.String[], java.lang.String) prepare_for_register_allocation (after) + /// CHECK-NOT: BoundType + + /// CHECK-START: java.lang.String[] Main.$noinline$testArraySetsHonorWriteBarrier(java.lang.String[], java.lang.String) disassembly (after) + /// CHECK: ArraySet value_can_be_null:true needs_type_check:false can_trigger_gc:false write_barrier_kind:EmitBeingReliedOn + // / CHECK: ; card_table + /// CHECK: ArraySet value_can_be_null:true needs_type_check:false can_trigger_gc:false write_barrier_kind:DontEmit + private static java.lang.String[] $noinline$testArraySetsHonorWriteBarrier( + String[] arr, String o2) { + Object o = null; + arr[0] = (String) o; + arr[1] = o2; + return arr; + } + + public static void $noinline$testDontSkipWriteBarrier() { + String[] arr = {"Hello", "World"}; + // We first run the gc to make sure the card is clean. + Runtime.getRuntime().gc(); + // Continually call $noinline$testArraySets while allocating over 64 MiB of memory (with + // heap size limited to 16 MiB), in order to increase memory pressure and eventually trigger + // a concurrent garbage collection, which will start by putting the GC in marking mode to + // trigger the bug. + for (int i = 0; i != 64 * 1024; ++i) { + $noinline$allocateAtLeast1KiB(); + $noinline$testArraySetsDontSkipWriteBarrier(arr, null, "Universe"); + } + } + + // When the bug was present, $noinline$testArraySetsDontSkipWriteBarrier would never set the + // card as dirty (which is incorrect). arr[1]'s write barrier depends on arr[0]'s write barrier. + // The code path should mark the card dirty without a null check on `o` in `arr[0] = o`. + + /// CHECK-START: java.lang.String[] Main.$noinline$testArraySetsDontSkipWriteBarrier(java.lang.String[], java.lang.String, java.lang.String) disassembly (after) + /// CHECK: ArraySet value_can_be_null:true needs_type_check:false can_trigger_gc:false write_barrier_kind:EmitBeingReliedOn + /// CHECK: ArraySet value_can_be_null:true needs_type_check:false can_trigger_gc:false write_barrier_kind:DontEmit + private static java.lang.String[] $noinline$testArraySetsDontSkipWriteBarrier( + String[] arr, String o, String o2) { + arr[0] = o; + arr[1] = o2; + return arr; + } + + // Allocate at least 1 KiB of memory on the managed heap. + // Retain some allocated memory and release old allocations so that the + // garbage collector has something to do. + public static void $noinline$allocateAtLeast1KiB() { + memory[allocationIndex] = new Object[1024 / 4]; + ++allocationIndex; + if (allocationIndex == memory.length) { + allocationIndex = 0; + } + } + + public static Object[] memory = new Object[1024]; + public static int allocationIndex = 0; +} diff --git a/test/knownfailures.json b/test/knownfailures.json index 269b68d014..a0b7dcd760 100644 --- a/test/knownfailures.json +++ b/test/knownfailures.json @@ -1,10 +1,5 @@ [ { - "tests": "2247-checker-write-barrier-elimination", - "description": ["Disable 2247- until we fix the WBE issue."], - "bug": "http://b/310755375" - }, - { "tests": "153-reference-stress", "description": ["Disable 153-reference-stress temporarily until a fix", "arrives."], |