Revert^2 "Improve ArraySet codegen."
This reverts commit 0dda8c84938d6bb4ce5a1707e5e109ea187fc33d.
The original change had two issues that have been fixed.
First, for heap poisoning, the null branch skipped over the
reference poisoning instructions which copy and poison the
value, thus writing whatever was left in the register.
Second, the change erroneously assumed that the slow path
performed only the assignability check and bound the slow
path exit label before the actual array store, unnecessarily
re-doing the store.
Change-Id: I9f380efa12aa807b4f566a932dbc9dae824fb25a
Test: m test-art-host-gtest
Test: testrunner.py --host --optimizing
Test: aosp_taimen-userdebug boots.
Test: testrunner.py --target --optimizing
Test: Repeat the above with
ART_USE_READ_BARRIER=false ART_HEAP_POISONING=true
Bug: 32489401
diff --git a/compiler/optimizing/code_generator_x86.cc b/compiler/optimizing/code_generator_x86.cc
index 7a73569..d71b694 100644
--- a/compiler/optimizing/code_generator_x86.cc
+++ b/compiler/optimizing/code_generator_x86.cc
@@ -5790,13 +5790,11 @@
bool needs_write_barrier =
CodeGenerator::StoreNeedsWriteBarrier(value_type, instruction->GetValue());
- bool may_need_runtime_call_for_type_check = instruction->NeedsTypeCheck();
+ bool needs_type_check = instruction->NeedsTypeCheck();
LocationSummary* locations = new (GetGraph()->GetAllocator()) LocationSummary(
instruction,
- may_need_runtime_call_for_type_check ?
- LocationSummary::kCallOnSlowPath :
- LocationSummary::kNoCall);
+ needs_type_check ? LocationSummary::kCallOnSlowPath : LocationSummary::kNoCall);
bool is_byte_type = DataType::Size(value_type) == 1u;
// We need the inputs to be different than the output in case of long operation.
@@ -5827,10 +5825,7 @@
Location index = locations->InAt(1);
Location value = locations->InAt(2);
DataType::Type value_type = instruction->GetComponentType();
- uint32_t class_offset = mirror::Object::ClassOffset().Int32Value();
- uint32_t super_offset = mirror::Class::SuperClassOffset().Int32Value();
- uint32_t component_offset = mirror::Class::ComponentTypeOffset().Int32Value();
- bool may_need_runtime_call_for_type_check = instruction->NeedsTypeCheck();
+ bool needs_type_check = instruction->NeedsTypeCheck();
bool needs_write_barrier =
CodeGenerator::StoreNeedsWriteBarrier(value_type, instruction->GetValue());
@@ -5873,30 +5868,30 @@
__ movl(address, Immediate(0));
codegen_->MaybeRecordImplicitNullCheck(instruction);
DCHECK(!needs_write_barrier);
- DCHECK(!may_need_runtime_call_for_type_check);
+ DCHECK(!needs_type_check);
break;
}
DCHECK(needs_write_barrier);
Register register_value = value.AsRegister<Register>();
- // We cannot use a NearLabel for `done`, as its range may be too
- // short when Baker read barriers are enabled.
- Label done;
- NearLabel not_null, do_put;
- SlowPathCode* slow_path = nullptr;
Location temp_loc = locations->GetTemp(0);
Register temp = temp_loc.AsRegister<Register>();
- if (may_need_runtime_call_for_type_check) {
+
+ bool can_value_be_null = instruction->GetValueCanBeNull();
+ NearLabel do_store;
+ if (can_value_be_null) {
+ __ testl(register_value, register_value);
+ __ j(kEqual, &do_store);
+ }
+
+ SlowPathCode* slow_path = nullptr;
+ if (needs_type_check) {
slow_path = new (codegen_->GetScopedAllocator()) ArraySetSlowPathX86(instruction);
codegen_->AddSlowPath(slow_path);
- if (instruction->GetValueCanBeNull()) {
- __ testl(register_value, register_value);
- __ j(kNotEqual, ¬_null);
- __ movl(address, Immediate(0));
- codegen_->MaybeRecordImplicitNullCheck(instruction);
- __ jmp(&done);
- __ Bind(¬_null);
- }
+
+ 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,
@@ -5919,6 +5914,7 @@
__ cmpl(temp, Address(register_value, class_offset));
if (instruction->StaticTypeOfArrayIsObjectArray()) {
+ NearLabel do_put;
__ j(kEqual, &do_put);
// If heap poisoning is enabled, the `temp` reference has
// not been unpoisoned yet; unpoison it now.
@@ -5935,21 +5931,27 @@
}
}
+ Register card = locations->GetTemp(1).AsRegister<Register>();
+ codegen_->MarkGCCard(
+ temp, card, array, value.AsRegister<Register>(), /* value_can_be_null= */ false);
+
+ if (can_value_be_null) {
+ DCHECK(do_store.IsLinked());
+ __ Bind(&do_store);
+ }
+
+ Register source = register_value;
if (kPoisonHeapReferences) {
__ movl(temp, register_value);
__ PoisonHeapReference(temp);
- __ movl(address, temp);
- } else {
- __ movl(address, register_value);
- }
- if (!may_need_runtime_call_for_type_check) {
- codegen_->MaybeRecordImplicitNullCheck(instruction);
+ source = temp;
}
- Register card = locations->GetTemp(1).AsRegister<Register>();
- codegen_->MarkGCCard(
- temp, card, array, value.AsRegister<Register>(), instruction->GetValueCanBeNull());
- __ Bind(&done);
+ __ movl(address, source);
+
+ if (can_value_be_null || !needs_type_check) {
+ codegen_->MaybeRecordImplicitNullCheck(instruction);
+ }
if (slow_path != nullptr) {
__ Bind(slow_path->GetExitLabel());