598491 - JM: Faster x64 Value storing.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Sean Stangl [:sstangl]]

Attachment: Define and use storeValueFromComponents().

This is very similar to patch 1 from bug 587444, reviewed in https://bugzilla.mozilla.org/show_bug.cgi?id=587444#c10. It incorporates the changes mentioned in the review, specifically:

FrameState::storeTo() receives an x64 optimization, and is called from appropriate places. The implementations for x64 and x86/arm differ: since this is currently the only function that differs based on architecture, I just added #ifdefs. During changes to the syncing code, we may determine that code organization would be better suited by the existence of FrameState{32,64}.cpp, but such a separation is currently premature.

Assembler::storeValue(ValueRemat vr, ...) is defined, which eliminates emitStore() in MIC code, and eliminates redundancy in some parts.

Assembler::storeValueForIC() is finally killed off.

At this point, the large amount of #ifdefs in PIC-related code in Compiler.cpp exist solely to prevent unused variable warnings.

This patch gives a 1.4% speedup overall on SS-0.9.1. nsieve-bits and fannkuch are made 7% faster. spectral-norm is made 9% faster. The majority of the benefit of storeValueFromComponents() is received from a follow-up patch that affects syncing code. So this is pretty good.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 477335 [details] [diff] [review]
Define and use storeValueFromComponents().

>         if (fe->isConstant()) {
>             masm.storeValue(fe->getValue(), slot);
>+        } else if (fe->isTypeKnown()) {
>+            masm.storeValueFromComponents(ImmType(fe->getKnownType()),
>+                                          frame.tempRegForData(fe), slot);
>         } else {
>+#if defined JS_NUNBOX32
>+            masm.storeTypeTag(frame.tempRegForType(fe), slot);
>             masm.storePayload(frame.tempRegForData(fe), slot);
>-            if (fe->isTypeKnown())
>-                masm.storeTypeTag(ImmType(fe->getKnownType()), slot);
>-            else
>-                masm.storeTypeTag(frame.tempRegForType(fe), slot);
>+#elif defined JS_PUNBOX64
>+            RegisterID dreg = frame.tempRegForData(fe);
>+            frame.pinReg(dreg);
>+            masm.storeValueFromComponents(frame.tempRegForType(fe), dreg, slot);
>+            frame.unpinReg(dreg);
>+#endif

Is this basically what FrameState::storeTo() does?

>+#if defined JS_PUNBOX64
>+    if (fe->type.inMemory() && fe->data.inMemory()) {
>+        /* NB: Track that the Value is in a register. */

No need for "NB:" here.

>+    /* Get a register for the payload. */
>+    bool wasInRegister = fe->data.inRegister();
>+    RegisterID dreg = Registers::ReturnReg;

MaybeRegisterID instead

>+    /* Store the Value. */
>+    if (fe->type.inRegister()) {
>+        masm.storeValueFromComponents(fe->type.reg(), dreg, address);
>+    } else if (fe->isTypeKnown()) {
>+        masm.storeValueFromComponents(ImmType(fe->getKnownType()), dreg, address);
>+    } else {
>+        JS_ASSERT(fe->type.inMemory());
>+        RegisterID treg = popped ? allocReg() : allocReg(fe, RematInfo::TYPE);

Can this allocReg() spill the data reg? Unlike the x86 version, the data reg hasn't been stored yet.

r=me with these fixed

Comment 2

[Sean Stangl [:sstangl]]

http://hg.mozilla.org/tracemonkey/rev/3e13d9c176ac

storeTo() duplication mentioned in comment 1 exists because slot is BaseIndex, not Address. It will be addressed in an unrelated patch where we templatize the Assemblers.

Comment 3

[Sean Stangl [:sstangl]]

http://hg.mozilla.org/tracemonkey/rev/2a2c7bfd651d

jsval_layout is defined differently on x86 opt builds. Fixes build bustage.

Comment 4

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/3e13d9c176ac