From 7e8678bbaaedaf8ee915e8af28799d3296b07090 Mon Sep 17 00:00:00 2001 From: "bmeurer@chromium.org" Date: Tue, 27 May 2014 07:17:08 +0000 Subject: [PATCH] Avoid HeapObject check in HStoreNamedField. This way an HStoreNamedField instruction can never deoptimize itself, which is another important step towards a working store elimination. R=jarin@chromium.org Review URL: https://codereview.chromium.org/299373005 git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@21509 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/arm/lithium-arm.cc | 8 +------- src/arm/lithium-codegen-arm.cc | 25 +++++++------------------ src/arm/macro-assembler-arm.cc | 5 +++++ src/arm64/lithium-arm64.cc | 8 +------- src/arm64/lithium-codegen-arm64.cc | 24 +++++++++--------------- src/arm64/macro-assembler-arm64.cc | 5 +++++ src/hydrogen-instructions.cc | 2 +- src/hydrogen-instructions.h | 6 ++++++ src/hydrogen.cc | 11 ++++------- src/ia32/lithium-codegen-ia32.cc | 30 ++++++------------------------ src/ia32/lithium-ia32.cc | 11 +---------- src/ia32/macro-assembler-ia32.cc | 5 +++++ src/x64/lithium-codegen-x64.cc | 29 ++++++----------------------- src/x64/lithium-x64.cc | 10 +--------- src/x64/macro-assembler-x64.cc | 5 +++++ 15 files changed, 63 insertions(+), 121 deletions(-) diff --git a/src/arm/lithium-arm.cc b/src/arm/lithium-arm.cc index 5e4b23efd2..f494df8c05 100644 --- a/src/arm/lithium-arm.cc +++ b/src/arm/lithium-arm.cc @@ -2316,13 +2316,7 @@ LInstruction* LChunkBuilder::DoStoreNamedField(HStoreNamedField* instr) { // We need a temporary register for write barrier of the map field. LOperand* temp = needs_write_barrier_for_map ? TempRegister() : NULL; - LInstruction* result = new(zone()) LStoreNamedField(obj, val, temp); - if (!instr->access().IsExternalMemory() && - instr->field_representation().IsHeapObject() && - !instr->value()->type().IsHeapObject()) { - result = AssignEnvironment(result); - } - return result; + return new(zone()) LStoreNamedField(obj, val, temp); } diff --git a/src/arm/lithium-codegen-arm.cc b/src/arm/lithium-codegen-arm.cc index b417c801d5..5f249c2905 100644 --- a/src/arm/lithium-codegen-arm.cc +++ b/src/arm/lithium-codegen-arm.cc @@ -4062,23 +4062,12 @@ void LCodeGen::DoStoreNamedField(LStoreNamedField* instr) { return; } - SmiCheck check_needed = - instr->hydrogen()->value()->IsHeapObject() - ? OMIT_SMI_CHECK : INLINE_SMI_CHECK; + __ AssertNotSmi(object); - ASSERT(!(representation.IsSmi() && - instr->value()->IsConstantOperand() && - !IsSmi(LConstantOperand::cast(instr->value())))); - if (representation.IsHeapObject()) { - Register value = ToRegister(instr->value()); - if (!instr->hydrogen()->value()->type().IsHeapObject()) { - __ SmiTst(value); - DeoptimizeIf(eq, instr->environment()); - - // We know now that value is not a smi, so we can omit the check below. - check_needed = OMIT_SMI_CHECK; - } - } else if (representation.IsDouble()) { + ASSERT(!representation.IsSmi() || + !instr->value()->IsConstantOperand() || + IsSmi(LConstantOperand::cast(instr->value()))); + if (representation.IsDouble()) { ASSERT(access.IsInobject()); ASSERT(!instr->hydrogen()->has_transition()); ASSERT(!instr->hydrogen()->NeedsWriteBarrier()); @@ -4120,7 +4109,7 @@ void LCodeGen::DoStoreNamedField(LStoreNamedField* instr) { GetLinkRegisterState(), kSaveFPRegs, EMIT_REMEMBERED_SET, - check_needed); + instr->hydrogen()->SmiCheckForWriteBarrier()); } } else { __ ldr(scratch, FieldMemOperand(object, JSObject::kPropertiesOffset)); @@ -4136,7 +4125,7 @@ void LCodeGen::DoStoreNamedField(LStoreNamedField* instr) { GetLinkRegisterState(), kSaveFPRegs, EMIT_REMEMBERED_SET, - check_needed); + instr->hydrogen()->SmiCheckForWriteBarrier()); } } } diff --git a/src/arm/macro-assembler-arm.cc b/src/arm/macro-assembler-arm.cc index 7b3028fbe6..28ce3de955 100644 --- a/src/arm/macro-assembler-arm.cc +++ b/src/arm/macro-assembler-arm.cc @@ -401,6 +401,11 @@ void MacroAssembler::Store(Register src, } else if (r.IsInteger16() || r.IsUInteger16()) { strh(src, dst); } else { + if (r.IsHeapObject()) { + AssertNotSmi(src); + } else if (r.IsSmi()) { + AssertSmi(src); + } str(src, dst); } } diff --git a/src/arm64/lithium-arm64.cc b/src/arm64/lithium-arm64.cc index 50e0f44cfe..f434e466cd 100644 --- a/src/arm64/lithium-arm64.cc +++ b/src/arm64/lithium-arm64.cc @@ -2399,13 +2399,7 @@ LInstruction* LChunkBuilder::DoStoreNamedField(HStoreNamedField* instr) { temp0 = TempRegister(); } - LStoreNamedField* result = - new(zone()) LStoreNamedField(object, value, temp0, temp1); - if (instr->field_representation().IsHeapObject() && - !instr->value()->type().IsHeapObject()) { - return AssignEnvironment(result); - } - return result; + return new(zone()) LStoreNamedField(object, value, temp0, temp1); } diff --git a/src/arm64/lithium-codegen-arm64.cc b/src/arm64/lithium-codegen-arm64.cc index 36605514b7..8ffbbc5636 100644 --- a/src/arm64/lithium-codegen-arm64.cc +++ b/src/arm64/lithium-codegen-arm64.cc @@ -5320,7 +5320,11 @@ void LCodeGen::DoStoreNamedField(LStoreNamedField* instr) { Register value = ToRegister(instr->value()); __ Store(value, MemOperand(object, offset), representation); return; - } else if (representation.IsDouble()) { + } + + __ AssertNotSmi(object); + + if (representation.IsDouble()) { ASSERT(access.IsInobject()); ASSERT(!instr->hydrogen()->has_transition()); ASSERT(!instr->hydrogen()->NeedsWriteBarrier()); @@ -5331,19 +5335,9 @@ void LCodeGen::DoStoreNamedField(LStoreNamedField* instr) { Register value = ToRegister(instr->value()); - SmiCheck check_needed = instr->hydrogen()->value()->IsHeapObject() - ? OMIT_SMI_CHECK : INLINE_SMI_CHECK; - - ASSERT(!(representation.IsSmi() && - instr->value()->IsConstantOperand() && - !IsInteger32Constant(LConstantOperand::cast(instr->value())))); - if (representation.IsHeapObject() && - !instr->hydrogen()->value()->type().IsHeapObject()) { - DeoptimizeIfSmi(value, instr->environment()); - - // We know now that value is not a smi, so we can omit the check below. - check_needed = OMIT_SMI_CHECK; - } + ASSERT(!representation.IsSmi() || + !instr->value()->IsConstantOperand() || + IsInteger32Constant(LConstantOperand::cast(instr->value()))); if (instr->hydrogen()->has_transition()) { Handle transition = instr->hydrogen()->transition_map(); @@ -5403,7 +5397,7 @@ void LCodeGen::DoStoreNamedField(LStoreNamedField* instr) { GetLinkRegisterState(), kSaveFPRegs, EMIT_REMEMBERED_SET, - check_needed); + instr->hydrogen()->SmiCheckForWriteBarrier()); } } diff --git a/src/arm64/macro-assembler-arm64.cc b/src/arm64/macro-assembler-arm64.cc index 75fd2b62be..83c45a68d3 100644 --- a/src/arm64/macro-assembler-arm64.cc +++ b/src/arm64/macro-assembler-arm64.cc @@ -559,6 +559,11 @@ void MacroAssembler::Store(const Register& rt, Str(rt.W(), addr); } else { ASSERT(rt.Is64Bits()); + if (r.IsHeapObject()) { + AssertNotSmi(rt); + } else if (r.IsSmi()) { + AssertSmi(rt); + } Str(rt, addr); } } diff --git a/src/hydrogen-instructions.cc b/src/hydrogen-instructions.cc index 2f8c2da592..388f92f58f 100644 --- a/src/hydrogen-instructions.cc +++ b/src/hydrogen-instructions.cc @@ -878,6 +878,7 @@ bool HInstruction::CanDeoptimize() { case HValue::kSeqStringGetChar: case HValue::kStoreCodeEntry: case HValue::kStoreKeyed: + case HValue::kStoreNamedField: case HValue::kStoreNamedGeneric: case HValue::kStringCharCodeAt: case HValue::kStringCharFromCode: @@ -926,7 +927,6 @@ bool HInstruction::CanDeoptimize() { case HValue::kStoreContextSlot: case HValue::kStoreGlobalCell: case HValue::kStoreKeyedGeneric: - case HValue::kStoreNamedField: case HValue::kStringAdd: case HValue::kStringCompareAndBranch: case HValue::kSub: diff --git a/src/hydrogen-instructions.h b/src/hydrogen-instructions.h index 4f83aafa72..ccc12dc5b2 100644 --- a/src/hydrogen-instructions.h +++ b/src/hydrogen-instructions.h @@ -6816,6 +6816,12 @@ class HStoreNamedField V8_FINAL : public HTemplateInstruction<3> { new_space_dominator()); } + SmiCheck SmiCheckForWriteBarrier() const { + if (field_representation().IsHeapObject()) return OMIT_SMI_CHECK; + if (value()->IsHeapObject()) return OMIT_SMI_CHECK; + return INLINE_SMI_CHECK; + } + Representation field_representation() const { return access_.representation(); } diff --git a/src/hydrogen.cc b/src/hydrogen.cc index bc37d450dc..750edbf615 100644 --- a/src/hydrogen.cc +++ b/src/hydrogen.cc @@ -5505,16 +5505,13 @@ HInstruction* HOptimizedGraphBuilder::BuildStoreNamedField( value, STORE_TO_INITIALIZED_ENTRY); } } else { + if (field_access.representation().IsHeapObject()) { + BuildCheckHeapObject(value); + } + if (!info->field_maps()->is_empty()) { ASSERT(field_access.representation().IsHeapObject()); - BuildCheckHeapObject(value); value = Add(value, info->field_maps()); - - // TODO(bmeurer): This is a dirty hack to avoid repeating the smi check - // that was already performed by the HCheckHeapObject above in the - // HStoreNamedField below. We should really do this right instead and - // make Crankshaft aware of Representation::HeapObject(). - field_access = field_access.WithRepresentation(Representation::Tagged()); } // This is a normal store. diff --git a/src/ia32/lithium-codegen-ia32.cc b/src/ia32/lithium-codegen-ia32.cc index 9fd0ae532c..525944a3db 100644 --- a/src/ia32/lithium-codegen-ia32.cc +++ b/src/ia32/lithium-codegen-ia32.cc @@ -3978,30 +3978,12 @@ void LCodeGen::DoStoreNamedField(LStoreNamedField* instr) { } Register object = ToRegister(instr->object()); - SmiCheck check_needed = - instr->hydrogen()->value()->IsHeapObject() - ? OMIT_SMI_CHECK : INLINE_SMI_CHECK; + __ AssertNotSmi(object); - ASSERT(!(representation.IsSmi() && - instr->value()->IsConstantOperand() && - !IsSmi(LConstantOperand::cast(instr->value())))); - if (representation.IsHeapObject()) { - if (instr->value()->IsConstantOperand()) { - LConstantOperand* operand_value = LConstantOperand::cast(instr->value()); - if (chunk_->LookupConstant(operand_value)->HasSmiValue()) { - DeoptimizeIf(no_condition, instr->environment()); - } - } else { - if (!instr->hydrogen()->value()->type().IsHeapObject()) { - Register value = ToRegister(instr->value()); - __ test(value, Immediate(kSmiTagMask)); - DeoptimizeIf(zero, instr->environment()); - - // We know now that value is not a smi, so we can omit the check below. - check_needed = OMIT_SMI_CHECK; - } - } - } else if (representation.IsDouble()) { + ASSERT(!representation.IsSmi() || + !instr->value()->IsConstantOperand() || + IsSmi(LConstantOperand::cast(instr->value()))); + if (representation.IsDouble()) { ASSERT(access.IsInobject()); ASSERT(!instr->hydrogen()->has_transition()); ASSERT(!instr->hydrogen()->NeedsWriteBarrier()); @@ -4068,7 +4050,7 @@ void LCodeGen::DoStoreNamedField(LStoreNamedField* instr) { temp, kSaveFPRegs, EMIT_REMEMBERED_SET, - check_needed); + instr->hydrogen()->SmiCheckForWriteBarrier()); } } diff --git a/src/ia32/lithium-ia32.cc b/src/ia32/lithium-ia32.cc index 3ebceea66a..7e587c1d45 100644 --- a/src/ia32/lithium-ia32.cc +++ b/src/ia32/lithium-ia32.cc @@ -2394,16 +2394,7 @@ LInstruction* LChunkBuilder::DoStoreNamedField(HStoreNamedField* instr) { // We need a temporary register for write barrier of the map field. LOperand* temp_map = needs_write_barrier_for_map ? TempRegister() : NULL; - LInstruction* result = - new(zone()) LStoreNamedField(obj, val, temp, temp_map); - if (!instr->access().IsExternalMemory() && - instr->field_representation().IsHeapObject() && - (val->IsConstantOperand() - ? HConstant::cast(instr->value())->HasSmiValue() - : !instr->value()->type().IsHeapObject())) { - result = AssignEnvironment(result); - } - return result; + return new(zone()) LStoreNamedField(obj, val, temp, temp_map); } diff --git a/src/ia32/macro-assembler-ia32.cc b/src/ia32/macro-assembler-ia32.cc index a2d2280d2c..9f10b82b41 100644 --- a/src/ia32/macro-assembler-ia32.cc +++ b/src/ia32/macro-assembler-ia32.cc @@ -55,6 +55,11 @@ void MacroAssembler::Store(Register src, const Operand& dst, Representation r) { } else if (r.IsInteger16() || r.IsUInteger16()) { mov_w(dst, src); } else { + if (r.IsHeapObject()) { + AssertNotSmi(src); + } else if (r.IsSmi()) { + AssertSmi(src); + } mov(dst, src); } } diff --git a/src/x64/lithium-codegen-x64.cc b/src/x64/lithium-codegen-x64.cc index 5d72524cfe..879fbb82df 100644 --- a/src/x64/lithium-codegen-x64.cc +++ b/src/x64/lithium-codegen-x64.cc @@ -4000,29 +4000,12 @@ void LCodeGen::DoStoreNamedField(LStoreNamedField* instr) { } Register object = ToRegister(instr->object()); - SmiCheck check_needed = hinstr->value()->IsHeapObject() - ? OMIT_SMI_CHECK : INLINE_SMI_CHECK; + __ AssertNotSmi(object); - ASSERT(!(representation.IsSmi() && - instr->value()->IsConstantOperand() && - !IsInteger32Constant(LConstantOperand::cast(instr->value())))); - if (representation.IsHeapObject()) { - if (instr->value()->IsConstantOperand()) { - LConstantOperand* operand_value = LConstantOperand::cast(instr->value()); - if (chunk_->LookupConstant(operand_value)->HasSmiValue()) { - DeoptimizeIf(no_condition, instr->environment()); - } - } else { - if (!hinstr->value()->type().IsHeapObject()) { - Register value = ToRegister(instr->value()); - Condition cc = masm()->CheckSmi(value); - DeoptimizeIf(cc, instr->environment()); - - // We know now that value is not a smi, so we can omit the check below. - check_needed = OMIT_SMI_CHECK; - } - } - } else if (representation.IsDouble()) { + ASSERT(!representation.IsSmi() || + !instr->value()->IsConstantOperand() || + IsInteger32Constant(LConstantOperand::cast(instr->value()))); + if (representation.IsDouble()) { ASSERT(access.IsInobject()); ASSERT(!hinstr->has_transition()); ASSERT(!hinstr->NeedsWriteBarrier()); @@ -4107,7 +4090,7 @@ void LCodeGen::DoStoreNamedField(LStoreNamedField* instr) { temp, kSaveFPRegs, EMIT_REMEMBERED_SET, - check_needed); + hinstr->SmiCheckForWriteBarrier()); } } diff --git a/src/x64/lithium-x64.cc b/src/x64/lithium-x64.cc index e15d4a6987..e1b9e5f159 100644 --- a/src/x64/lithium-x64.cc +++ b/src/x64/lithium-x64.cc @@ -2327,15 +2327,7 @@ LInstruction* LChunkBuilder::DoStoreNamedField(HStoreNamedField* instr) { LOperand* temp = (!is_in_object || needs_write_barrier || needs_write_barrier_for_map) ? TempRegister() : NULL; - LInstruction* result = new(zone()) LStoreNamedField(obj, val, temp); - if (!instr->access().IsExternalMemory() && - instr->field_representation().IsHeapObject() && - (val->IsConstantOperand() - ? HConstant::cast(instr->value())->HasSmiValue() - : !instr->value()->type().IsHeapObject())) { - result = AssignEnvironment(result); - } - return result; + return new(zone()) LStoreNamedField(obj, val, temp); } diff --git a/src/x64/macro-assembler-x64.cc b/src/x64/macro-assembler-x64.cc index 6873efff67..f773a61bd5 100644 --- a/src/x64/macro-assembler-x64.cc +++ b/src/x64/macro-assembler-x64.cc @@ -924,6 +924,11 @@ void MacroAssembler::Store(const Operand& dst, Register src, Representation r) { } else if (r.IsInteger32()) { movl(dst, src); } else { + if (r.IsHeapObject()) { + AssertNotSmi(src); + } else if (r.IsSmi()) { + AssertSmi(src); + } movp(dst, src); } }