commit 0dda8c84938d6bb4ce5a1707e5e109ea187fc33d
author Vladimir Marko <vmarko@google.com> Thu May 16 12:47:40 2019 +0000
committer Vladimir Marko <vmarko@google.com> Thu May 16 12:47:40 2019 +0000
tree 3d0b4f35ef4d00aa18eba0e417655d43bc44bf5a
parent 0ece86491008837a9814f7a2e0d7961c74ef4195

Revert "Improve ArraySet codegen."

This reverts commit 0ece86491008837a9814f7a2e0d7961c74ef4195.

Reason for revert: Breaks heap poisoning tests.

Bug: 32489401
Change-Id: Ied4150829eea848d0f967866d87c6aa7dafd39a1

compiler/optimizing/code_generator_x86.cc

diff --git a/compiler/optimizing/code_generator_x86.cc b/compiler/optimizing/code_generator_x86.cc
index 334f2b4..ca1723b 100644
--- a/compiler/optimizing/code_generator_x86.cc
+++ b/compiler/optimizing/code_generator_x86.cc
@@ -5781,11 +5781,13 @@
 
   bool needs_write_barrier =
       CodeGenerator::StoreNeedsWriteBarrier(value_type, instruction->GetValue());
-  bool needs_type_check = instruction->NeedsTypeCheck();
+  bool may_need_runtime_call_for_type_check = instruction->NeedsTypeCheck();
 
   LocationSummary* locations = new (GetGraph()->GetAllocator()) LocationSummary(
       instruction,
-      needs_type_check ? LocationSummary::kCallOnSlowPath : LocationSummary::kNoCall);
+      may_need_runtime_call_for_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.
@@ -5816,7 +5818,10 @@
   Location index = locations->InAt(1);
   Location value = locations->InAt(2);
   DataType::Type value_type = instruction->GetComponentType();
-  bool needs_type_check = instruction->NeedsTypeCheck();
+  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_write_barrier =
       CodeGenerator::StoreNeedsWriteBarrier(value_type, instruction->GetValue());
 
@@ -5859,30 +5864,30 @@
         __ movl(address, Immediate(0));
         codegen_->MaybeRecordImplicitNullCheck(instruction);
         DCHECK(!needs_write_barrier);
-        DCHECK(!needs_type_check);
+        DCHECK(!may_need_runtime_call_for_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>();
-
-      bool can_value_be_null = instruction->GetValueCanBeNull();
-      NearLabel do_store;
-      if (can_value_be_null) {
-        __ testl(register_value, register_value);
-        __ j(kEqual, &do_store);
-      }
-
-      if (needs_type_check) {
-        SlowPathCode* slow_path =
-            new (codegen_->GetScopedAllocator()) ArraySetSlowPathX86(instruction);
+      if (may_need_runtime_call_for_type_check) {
+        slow_path = new (codegen_->GetScopedAllocator()) ArraySetSlowPathX86(instruction);
         codegen_->AddSlowPath(slow_path);
-
-        const uint32_t class_offset = mirror::Object::ClassOffset().Int32Value();
-        const uint32_t super_offset = mirror::Class::SuperClassOffset().Int32Value();
-        const uint32_t component_offset = mirror::Class::ComponentTypeOffset().Int32Value();
+        if (instruction->GetValueCanBeNull()) {
+          __ testl(register_value, register_value);
+          __ j(kNotEqual, &not_null);
+          __ movl(address, Immediate(0));
+          codegen_->MaybeRecordImplicitNullCheck(instruction);
+          __ jmp(&done);
+          __ Bind(&not_null);
+        }
 
         // Note that when Baker read barriers are enabled, the type
         // checks are performed without read barriers.  This is fine,
@@ -5905,7 +5910,6 @@
         __ cmpl(temp, Address(register_value, class_offset));
 
         if (instruction->StaticTypeOfArrayIsObjectArray()) {
-          NearLabel do_put;  // Use a dedicated NearLabel instead of the slow_path->GetExitLabel().
           __ j(kEqual, &do_put);
           // If heap poisoning is enabled, the `temp` reference has
           // not been unpoisoned yet; unpoison it now.
@@ -5920,29 +5924,26 @@
         } else {
           __ j(kNotEqual, slow_path->GetEntryLabel());
         }
-        __ Bind(slow_path->GetExitLabel());
+      }
+
+      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);
       }
 
       Register card = locations->GetTemp(1).AsRegister<Register>();
       codegen_->MarkGCCard(
-          temp, card, array, value.AsRegister<Register>(), /* value_can_be_null= */ false);
+          temp, card, array, value.AsRegister<Register>(), instruction->GetValueCanBeNull());
+      __ Bind(&done);
 
-      Register source = register_value;
-      if (kPoisonHeapReferences) {
-        __ movl(temp, register_value);
-        __ PoisonHeapReference(temp);
-        source = temp;
-      }
-
-      if (can_value_be_null) {
-        DCHECK(do_store.IsLinked());
-        __ Bind(&do_store);
-      }
-
-      __ movl(address, source);
-
-      if (can_value_be_null || !needs_type_check) {
-        codegen_->MaybeRecordImplicitNullCheck(instruction);
+      if (slow_path != nullptr) {
+        __ Bind(slow_path->GetExitLabel());
       }
 
       break;