Avoid re-tagging of the index register in optimized byte array access.

runtime/vm/intermediate_language_ia32.cc

 // Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include "vm/globals.h"  // Needed here to get TARGET_ARCH_IA32.
 #if defined(TARGET_ARCH_IA32)
 
 #include "vm/intermediate_language.h"
 
 #include "lib/error.h"
@@ skipping 1145 matching lines @@
   }
 }
 
 
 LocationSummary* LoadIndexedInstr::MakeLocationSummary() const {
   const intptr_t kNumInputs = 2;
   const intptr_t kNumTemps = 0;
   LocationSummary* locs =
       new LocationSummary(kNumInputs, kNumTemps, LocationSummary::kNoCall);
   locs->set_in(0, Location::RequiresRegister());
-  // The smi index is either untagged and tagged again at the end of the
-  // operation (element size == 1), or it is left smi tagged (for all element
-  // sizes > 1).
-  locs->set_in(1, CanBeImmediateIndex(index(), class_id())
-                    ? Location::RegisterOrSmiConstant(index())
-                    : Location::RequiresRegister());
+  // The smi index is either untagged (element size == 1), or it is left smi
+  // tagged (for all element sizes > 1).
+  if (index_scale() == 1) {
+    locs->set_in(1, CanBeImmediateIndex(index(), class_id())
+                      ? Location::WritableRegisterOrSmiConstant(index())
+                      : Location::WritableRegister());
+  } else {
+    locs->set_in(1, CanBeImmediateIndex(index(), class_id())
+                      ? Location::RegisterOrSmiConstant(index())
+                      : Location::RequiresRegister());
+  }
   if (representation() == kUnboxedDouble) {
     locs->set_out(Location::RequiresFpuRegister());
   } else {
     locs->set_out(Location::RequiresRegister());
   }
   return locs;
 }
 
 
 void LoadIndexedInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
@@ skipping 10 matching lines @@
             class_id(), index_scale(), result,
             Smi::Cast(index.constant()).Value());
     ASSERT(index_scale() == 1);
     if (index.IsRegister()) {
       __ SmiUntag(index.reg());
     }
     __ movl(result,
             FieldAddress(array, ExternalUint8Array::data_offset()));
     __ movzxb(result, element_address);
     __ SmiTag(result);
-    if (index.IsRegister()) {
-      __ SmiTag(index.reg());  // Re-tag.
-    }
     return;
   }
 
   FieldAddress element_address = index.IsRegister()
       ? FlowGraphCompiler::ElementAddressForRegIndex(
           class_id(), index_scale(), array, index.reg())
       : FlowGraphCompiler::ElementAddressForIntIndex(
           class_id(), index_scale(), array,
           Smi::Cast(index.constant()).Value());
 
@@ skipping 14 matching lines @@
         break;
       case kFloat32ArrayCid:
         // Load single precision float and promote to double.
         __ movss(result, element_address);
         __ cvtss2sd(result, locs()->out().fpu_reg());
         break;
       case kFloat64ArrayCid:
         __ movsd(result, element_address);
         break;
     }
-    if ((index_scale() == 1) && index.IsRegister()) {
-        __ SmiTag(index.reg());  // Re-tag.
-    }
     return;
   }
 
   Register result = locs()->out().reg();
   if ((index_scale() == 1) && index.IsRegister()) {
     __ SmiUntag(index.reg());
   }
   switch (class_id()) {
     case kInt8ArrayCid:
     case kUint8ArrayCid:
@@ skipping 32 matching lines @@
         __ testl(result, Immediate(0xC0000000));
         __ j(NOT_ZERO, deopt);
         __ SmiTag(result);
       }
       break;
     default:
       ASSERT((class_id() == kArrayCid) || (class_id() == kImmutableArrayCid));
       __ movl(result, element_address);
       break;
   }
-  if ((index_scale() == 1) && index.IsRegister()) {
-      __ SmiTag(index.reg());  // Re-tag.
-  }
 }
 
 
 Representation StoreIndexedInstr::RequiredInputRepresentation(
     intptr_t idx) const {
   if ((idx == 0) || (idx == 1)) return kTagged;
   ASSERT(idx == 2);
   switch (class_id_) {
     case kArrayCid:
     case kInt8ArrayCid:
@@ skipping 14 matching lines @@
   }
 }
 
 
 LocationSummary* StoreIndexedInstr::MakeLocationSummary() const {
   const intptr_t kNumInputs = 3;
   const intptr_t kNumTemps = 0;
   LocationSummary* locs =
       new LocationSummary(kNumInputs, kNumTemps, LocationSummary::kNoCall);
   locs->set_in(0, Location::RequiresRegister());
-  // The smi index is either untagged and tagged again at the end of the
-  // operation (element size == 1), or it is left smi tagged (for all element
-  // sizes > 1).
-  locs->set_in(1, CanBeImmediateIndex(index(), class_id())
-                    ? Location::RegisterOrSmiConstant(index())
-                    : Location::RequiresRegister());
+  // The smi index is either untagged (element size == 1), or it is left smi
+  // tagged (for all element sizes > 1).
+  intptr_t index_scale = FlowGraphCompiler::ElementSizeFor(class_id());
+  if (index_scale == 1) {
+    locs->set_in(1, CanBeImmediateIndex(index(), class_id())
+                      ? Location::WritableRegisterOrSmiConstant(index())

[Vyacheslav Egorov (Google), 2013/02/13 17:43:04]

I think if CanBeImmediateIndex returns true then the value is guaranteed to be
constant. 

So you don't need WriableReisterOrSmiConstant at this place.

+                      : Location::WritableRegister());
+  } else {
+    locs->set_in(1, CanBeImmediateIndex(index(), class_id())
+                      ? Location::RegisterOrSmiConstant(index())

[Vyacheslav Egorov (Google), 2013/02/13 17:43:04]

ditto

+                      : Location::RequiresRegister());
+  }
   switch (class_id()) {
     case kArrayCid:
       locs->set_in(2, ShouldEmitStoreBarrier()
                         ? Location::WritableRegister()
                         : Location::RegisterOrConstant(value()));
       break;
     case kInt8ArrayCid:
     case kUint8ArrayCid:
     case kUint8ClampedArrayCid:
       // TODO(fschneider): Add location constraint for byte registers (EAX,
@@ skipping 55 matching lines @@
       }
       if (locs()->in(2).IsConstant()) {
         const Smi& constant = Smi::Cast(locs()->in(2).constant());
         __ movb(element_address,
                 Immediate(static_cast<int8_t>(constant.Value())));
       } else {
         ASSERT(locs()->in(2).reg() == EAX);
         __ SmiUntag(EAX);
         __ movb(element_address, AL);
       }
-      if (index.IsRegister()) {
-        __ SmiTag(index.reg());  // Re-tag.
-      }
       break;
     case kUint8ClampedArrayCid: {
       if (index.IsRegister()) {
         __ SmiUntag(index.reg());
       }
       if (locs()->in(2).IsConstant()) {
         const Smi& constant = Smi::Cast(locs()->in(2).constant());
         intptr_t value = constant.Value();
         // Clamp to 0x0 or 0xFF respectively.
         if (value > 0xFF) {
@@ skipping 11 matching lines @@
         __ j(BELOW_EQUAL, &store_value, Assembler::kNearJump);
         // Clamp to 0x0 or 0xFF respectively.
         __ j(GREATER, &store_0xff);
         __ xorl(EAX, EAX);
         __ jmp(&store_value, Assembler::kNearJump);
         __ Bind(&store_0xff);
         __ movl(EAX, Immediate(0xFF));
         __ Bind(&store_value);
         __ movb(element_address, AL);
       }
-      if (index.IsRegister()) {
-        __ SmiTag(index.reg());  // Re-tag.
-      }
       break;
     }
     case kInt16ArrayCid:
     case kUint16ArrayCid: {
       Register value = locs()->in(2).reg();
       __ SmiUntag(value);
       __ movw(element_address, value);
       break;
     }
     case kInt32ArrayCid:
@@ skipping 888 matching lines @@
                                slow_path->entry_label(),
                                Assembler::kFarJump,
                                out_reg);
   __ Bind(slow_path->exit_label());
   __ movsd(FieldAddress(out_reg, Double::value_offset()), value);
 }
 
 
 LocationSummary* UnboxDoubleInstr::MakeLocationSummary() const {
   const intptr_t kNumInputs = 1;
-  const intptr_t kNumTemps = CanDeoptimize() ? 1 : 0;
+  const intptr_t value_cid = value()->ResultCid();
+  const bool needs_temp = ((value_cid != kSmiCid) && (value_cid != kDoubleCid));
+  const bool needs_writable_input = (value_cid == kSmiCid);
+  const intptr_t kNumTemps = needs_temp ? 1 : 0;
   LocationSummary* summary =
       new LocationSummary(kNumInputs, kNumTemps, LocationSummary::kNoCall);
-  summary->set_in(0, Location::RequiresRegister());
-  if (CanDeoptimize()) summary->set_temp(0, Location::RequiresRegister());
+  summary->set_in(0, needs_writable_input
+                     ? Location::WritableRegister()
+                     : Location::RequiresRegister());
+  if (needs_temp) summary->set_temp(0, Location::RequiresRegister());
   summary->set_out(Location::RequiresFpuRegister());
   return summary;
 }
 
 
 void UnboxDoubleInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
   const intptr_t value_cid = value()->ResultCid();
   const Register value = locs()->in(0).reg();
   const XmmRegister result = locs()->out().fpu_reg();
 
   if (value_cid == kDoubleCid) {
     __ movsd(result, FieldAddress(value, Double::value_offset()));
   } else if (value_cid == kSmiCid) {
     __ SmiUntag(value);  // Untag input before conversion.
     __ cvtsi2sd(result, value);
-    __ SmiTag(value);  // Restore input register.
   } else {
     Label* deopt = compiler->AddDeoptStub(deopt_id_, kDeoptBinaryDoubleOp);
-    compiler->LoadDoubleOrSmiToFpu(result,
-                                   value,
-                                   locs()->temp(0).reg(),
-                                   deopt);
+    Register temp = locs()->temp(0).reg();
+    Label is_smi, done;
+    __ testl(value, Immediate(kSmiTagMask));
+    __ j(ZERO, &is_smi);
+    __ CompareClassId(value, kDoubleCid, temp);
+    __ j(NOT_EQUAL, deopt);
+    __ movsd(result, FieldAddress(value, Double::value_offset()));
+    __ jmp(&done);
+    __ Bind(&is_smi);
+    __ movl(temp, value);
+    __ SmiUntag(temp);
+    __ cvtsi2sd(result, temp);
+    __ Bind(&done);
   }
 }
 
 
 LocationSummary* BinaryDoubleOpInstr::MakeLocationSummary() const {
   const intptr_t kNumInputs = 2;
   const intptr_t kNumTemps = 0;
   LocationSummary* summary =
       new LocationSummary(kNumInputs, kNumTemps, LocationSummary::kNoCall);
   summary->set_in(0, Location::RequiresFpuRegister());
@@ skipping 408 matching lines @@
     Register length = locs()->in(0).reg();
     Register index = locs()->in(1).reg();
     __ cmpl(index, length);
     __ j(ABOVE_EQUAL, deopt);
   }
 }
 
 
 LocationSummary* UnboxIntegerInstr::MakeLocationSummary() const {
   const intptr_t kNumInputs = 1;
-  const intptr_t kNumTemps = CanDeoptimize() ? 1 : 0;
+  const intptr_t value_cid = value()->ResultCid();
+  const bool needs_temp = ((value_cid != kSmiCid) && (value_cid != kMintCid));
+  const bool needs_writable_input = (value_cid == kSmiCid);
+  const intptr_t kNumTemps = needs_temp ? 1 : 0;
   LocationSummary* summary =
       new LocationSummary(kNumInputs, kNumTemps, LocationSummary::kNoCall);
-  summary->set_in(0, Location::RequiresRegister());
-  if (CanDeoptimize()) summary->set_temp(0, Location::RequiresRegister());
+  summary->set_in(0, needs_writable_input
+                     ? Location::WritableRegister()
+                     : Location::RequiresRegister());
+  if (needs_temp) summary->set_temp(0, Location::RequiresRegister());
   summary->set_out(Location::RequiresFpuRegister());
   return summary;
 }
 
 
 void UnboxIntegerInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
   const intptr_t value_cid = value()->ResultCid();
   const Register value = locs()->in(0).reg();
   const XmmRegister result = locs()->out().fpu_reg();
 
   if (value_cid == kMintCid) {
     __ movsd(result, FieldAddress(value, Mint::value_offset()));
   } else if (value_cid == kSmiCid) {
     __ SmiUntag(value);  // Untag input before conversion.
     __ movd(result, value);
     __ pmovsxdq(result, result);
-    __ SmiTag(value);  // Restore input register.
   } else {
     Register temp = locs()->temp(0).reg();
     Label* deopt = compiler->AddDeoptStub(deopt_id_, kDeoptUnboxInteger);
     Label is_smi, done;
     __ testl(value, Immediate(kSmiTagMask));
     __ j(ZERO, &is_smi);
     __ CompareClassId(value, kMintCid, temp);
     __ j(NOT_EQUAL, deopt);
     __ movsd(result, FieldAddress(value, Mint::value_offset()));
     __ jmp(&done);
@@ skipping 573 matching lines @@
                          PcDescriptors::kOther,
                          locs());
   __ Drop(2);  // Discard type arguments and receiver.
 }
 
 }  // namespace dart
 
 #undef __
 
 #endif  // defined TARGET_ARCH_IA32