diff options
author | 2024-01-22 08:57:31 +0000 | |
---|---|---|
committer | 2024-01-22 10:49:24 +0000 | |
commit | b5b98b9bb31acb2deffb692c50d0fbc71476663b (patch) | |
tree | 3bda094f8d5ea90ad34e5d357889ce4933f657f5 | |
parent | dbf9d9309f3df9c9ac8a9e30277b31ebb2977f4d (diff) |
Revert^3 "Disable write-barrier elimination pass"
This reverts commit 9f8df195b7ff2ce47eec4e9b193ff3214ebed19c.
Reason for revert: Fix for x86_64 with heap poison enabled
This case uses a temp with index `1` in the regular FieldSet case.
This is done like this due to GenerateVarHandleSet also calling
HandleFieldSet. The bug was that we were allocating only one
temp in the regular FieldSet case and therefore not having the
temp with index `1` available.
PS1 is the revert as-is.
PS2 contains the fix.
Test: art/test/testrunner/testrunner.py --host --64 --optimizing -b
Test: Same command with heap poison enabled too
Bug: 301833859
Bug: 310755375
Bug: 260843353
Change-Id: Ie2740b4c443158c4e72810ce1d8268353c5f0055
30 files changed, 956 insertions, 440 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..db5bb43c79 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,6 +2915,10 @@ void InstructionCodeGeneratorARM64::VisitArraySet(HArraySet* instruction) { MacroAssembler* masm = GetVIXLAssembler(); if (!needs_write_barrier) { + if (codegen_->ShouldCheckGCCard(value_type, instruction->GetValue(), write_barrier_kind)) { + codegen_->CheckGCCardIsValid(array); + } + DCHECK(!needs_type_check); if (index.IsConstant()) { offset += Int64FromLocation(index) << DataType::SizeShift(value_type); @@ -2922,78 +2952,85 @@ void InstructionCodeGeneratorARM64::VisitArraySet(HArraySet* 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); - } - + // We can have `value` be `Zero` here if we have a write barrier we are relying on.' + DCHECK_IMPLIES(Register(value).IsZero(), + write_barrier_kind == WriteBarrierKind::kEmitBeingReliedOn); + bool can_value_be_null = true; 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. + codegen_->MarkGCCard(array); UseScratchRegisterScope temps(masm); if (kPoisonHeapReferences) { 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 8f2f25355d..36ac7bc82c 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" @@ -1280,6 +1281,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); @@ -1293,6 +1314,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."], |