DoNumberTagD performance improvement (issue 11028115)

[abe...@chromium.org]

Reviewers: danno,

Message:
PTAL

Description:
DoNumberTagD performance improvement

Allocate heap entry untagged and tag at end to avoid having to subtract off
the tag offset before storing the value.

BUG=


Please review this at https://codereview.chromium.org/11028115/

SVN Base: git://github.com/v8/v8.git@master

Affected files:
M src/arm/lithium-codegen-arm.cc
M src/arm/macro-assembler-arm.h
M src/arm/macro-assembler-arm.cc


Index: src/arm/lithium-codegen-arm.cc
diff --git a/src/arm/lithium-codegen-arm.cc b/src/arm/lithium-codegen-arm.cc
index
b5a0f9af31919d948e1ec2fa0fc6f77e47c5013f..52864d3c2b58625ff89bfe3ccb0fbefe92b6f831
100644
--- a/src/arm/lithium-codegen-arm.cc
+++ b/src/arm/lithium-codegen-arm.cc
@@ -4491,13 +4491,15 @@ void LCodeGen::DoNumberTagD(LNumberTagD* instr) {
DeferredNumberTagD* deferred = new(zone()) DeferredNumberTagD(this,
instr);
if (FLAG_inline_new) {
__ LoadRoot(scratch, Heap::kHeapNumberMapRootIndex);
- __ AllocateHeapNumber(reg, temp1, temp2, scratch, deferred->entry());
+ // We want the untagged address first for performance
+ __ AllocateHeapNumber(reg, temp1, temp2, scratch, deferred->entry(),
false);
} else {
__ jmp(deferred->entry());
}
__ bind(deferred->exit());
- __ sub(ip, reg, Operand(kHeapObjectTag));
- __ vstr(input_reg, ip, HeapNumber::kValueOffset);
+ __ vstr(input_reg, reg, HeapNumber::kValueOffset);
+ // Now that we have finished with the object's real address tag it
+ __ add(reg, reg, Operand(kHeapObjectTag));
}


@@ -4510,6 +4512,7 @@ void LCodeGen::DoDeferredNumberTagD(LNumberTagD*
instr) {

PushSafepointRegistersScope scope(this, Safepoint::kWithRegisters);
CallRuntimeFromDeferred(Runtime::kAllocateHeapNumber, 0, instr);
+ __ sub(r0, r0, Operand(kHeapObjectTag));
__ StoreToSafepointRegisterSlot(r0, reg);
}

Index: src/arm/macro-assembler-arm.cc
diff --git a/src/arm/macro-assembler-arm.cc b/src/arm/macro-assembler-arm.cc
index
cc7fc4d3a4203706ce0a9cccee63e43d2adbdbb2..a3cb02b4f38338c4bd9de6e642bc4321a7ea3436
100644
--- a/src/arm/macro-assembler-arm.cc
+++ b/src/arm/macro-assembler-arm.cc
@@ -3122,7 +3122,8 @@ void MacroAssembler::AllocateHeapNumber(Register
result,
Register scratch1,
Register scratch2,
Register heap_number_map,
- Label* gc_required) {
+ Label* gc_required,
+ bool tagged) {
// Allocate an object in the heap for the heap number and tag it as a
heap
// object.
AllocateInNewSpace(HeapNumber::kSize,
@@ -3130,11 +3131,15 @@ void MacroAssembler::AllocateHeapNumber(Register
result,
scratch1,
scratch2,
gc_required,
- TAG_OBJECT);
+ tagged?TAG_OBJECT:NO_ALLOCATION_FLAGS);

// Store heap number map in the allocated object.
AssertRegisterIsRoot(heap_number_map, Heap::kHeapNumberMapRootIndex);
- str(heap_number_map, FieldMemOperand(result, HeapObject::kMapOffset));
+ if (tagged) {
+ str(heap_number_map, FieldMemOperand(result, HeapObject::kMapOffset));
+ } else {
+ str(heap_number_map, MemOperand(result, HeapObject::kMapOffset));
+ }
}


Index: src/arm/macro-assembler-arm.h
diff --git a/src/arm/macro-assembler-arm.h b/src/arm/macro-assembler-arm.h
index
ff0deef0f46dff25a17eb5cbae7b1ae5c3b0f47b..448259f735322f2f0d14d15b0b84e9fe2b427eec
100644
--- a/src/arm/macro-assembler-arm.h
+++ b/src/arm/macro-assembler-arm.h
@@ -731,7 +731,8 @@ class MacroAssembler: public Assembler {
Register scratch1,
Register scratch2,
Register heap_number_map,
- Label* gc_required);
+ Label* gc_required,
+ bool tagged = true);
void AllocateHeapNumberWithValue(Register result,
DwVfpRegister value,
Register scratch1,

[da...@chromium.org]

LGTM with nits pending performance numbers


http://codereview.chromium.org/11028115/diff/1/src/arm/macro-assembler-arm.cc
File src/arm/macro-assembler-arm.cc (right):

http://codereview.chromium.org/11028115/diff/1/src/arm/macro-assembler-arm.cc#newcode3134
src/arm/macro-assembler-arm.cc:3134:
tagged?TAG_OBJECT:NO_ALLOCATION_FLAGS);
Spaces between ? and :

http://codereview.chromium.org/11028115/diff/1/src/arm/macro-assembler-arm.h
File src/arm/macro-assembler-arm.h (right):

http://codereview.chromium.org/11028115/diff/1/src/arm/macro-assembler-arm.h#newcode735
src/arm/macro-assembler-arm.h:735: bool tagged = true);
Use an enum here, like "enum TaggingMode { TAG_RESULT, DONT_TAG_RESULT
}" , otherwise at the call site it's difficult to tell what the
true/false actually means

http://codereview.chromium.org/11028115/

[abe...@chromium.org]

http://codereview.chromium.org/11028115/

[da...@chromium.org]

Almost there...


http://codereview.chromium.org/11028115/diff/6001/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):

http://codereview.chromium.org/11028115/diff/6001/src/arm/lithium-codegen-arm.cc#newcode4692
src/arm/lithium-codegen-arm.cc:4692:
ASSERT(input->Equals(instrt->result()));
I think the above change is a typo.

http://codereview.chromium.org/11028115/diff/6001/src/arm/stub-cache-arm.cc
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/11028115/diff/6001/src/arm/stub-cache-arm.cc#newcode3809
src/arm/stub-cache-arm.cc:3809:
nit: remove introduced whitespace

http://codereview.chromium.org/11028115/

[abe...@chromium.org]

http://codereview.chromium.org/11028115/diff/1/src/arm/macro-assembler-arm.cc
File src/arm/macro-assembler-arm.cc (right):

http://codereview.chromium.org/11028115/diff/1/src/arm/macro-assembler-arm.cc#newcode3134
src/arm/macro-assembler-arm.cc:3134:
tagged?TAG_OBJECT:NO_ALLOCATION_FLAGS);

On 2012/10/10 12:27:53, danno wrote:
> Spaces between ? and :

Done.

http://codereview.chromium.org/11028115/diff/1/src/arm/macro-assembler-arm.h
File src/arm/macro-assembler-arm.h (right):

http://codereview.chromium.org/11028115/diff/1/src/arm/macro-assembler-arm.h#newcode735
src/arm/macro-assembler-arm.h:735: bool tagged = true);

On 2012/10/10 12:27:53, danno wrote:
> Use an enum here, like "enum TaggingMode { TAG_RESULT, DONT_TAG_RESULT
}" ,
> otherwise at the call site it's difficult to tell what the true/false
actually
> means

Done.

http://codereview.chromium.org/11028115/diff/6001/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):

http://codereview.chromium.org/11028115/diff/6001/src/arm/lithium-codegen-arm.cc#newcode4692
src/arm/lithium-codegen-arm.cc:4692:
ASSERT(input->Equals(instrt->result()));

On 2012/10/11 13:03:57, danno wrote:
> I think the above change is a typo.

Done.

http://codereview.chromium.org/11028115/diff/6001/src/arm/stub-cache-arm.cc
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/11028115/diff/6001/src/arm/stub-cache-arm.cc#newcode3809
src/arm/stub-cache-arm.cc:3809:

On 2012/10/11 13:03:57, danno wrote:
> nit: remove introduced whitespace

Done.

http://codereview.chromium.org/11028115/