950703 - 32-bit stack slots and 32-bit moves

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Dan Gohman [:sunfish]]

Building on 949668, the following is a series of patches which implements proper 32-bit moves (e.g. movss, movl on x86), along with the use of 32-bit stack slots for 32-bit values on 64-bit platforms.

This is nice to do on its own, and it will help bug 947711.

Comment 1

[Dan Gohman [:sunfish]]

Attachment: lallocation-kinds.patch

The direction we're moving here is to have LDefinition::Type hold type information, so LAllocation::Kind can be independent of the type. This patch removes some last remaining type information from LAllocation::Kind.

This nicely simplifies code which doesn't care about the type, and it also eliminates some naming ambiguity when using 64-bit stack slots for purposes other than double-precision floating-point values.

Comment 2

[Dan Gohman [:sunfish]]

Attachment: moveop-rename-kind.patch

This patch renames MoveOp::Kind to MoveOp::Type, to emphasize the analogy LAllocation::Kind : LDefinition::Type :: Move::Operand::Kind : MoveOp::Type.

Comment 3

[Dan Gohman [:sunfish]]

Attachment: float-reg.patch

Following up on bug 949668 comment 9, this patch adds an isFloatReg() predicate for testing whether an allocation is in a floating-point register class. This also renames an old isDouble() predicate to isFloatReg() so that register class queries all use the same name.

I had discussed with some people naming this predicate isFloatRegClass(), to emphasize that it's testing the register class rather than the type, but I'm leaving it as isFloatReg() for now because it's consistent with the toFloatReg() method. Also, with isDouble() predicates going away, there are fewer opportunities for ambiguity.

Comment 4

[Dan Gohman [:sunfish]]

Attachment: rename-float-to-float32.patch

This patch just renames a bunch of things with "Float" in their name to use "Float32" if they are referring to the float32 type, to reduce naming ambiguity.

Comment 5

[Dan Gohman [:sunfish]]

Attachment: float32-move.patch

This patch implements the use of 32-bit moves, such as movss on x86, for float32 values.

Comment 6

[Dan Gohman [:sunfish]]

Attachment: stack-slot-allocate-type.patch

This patch implements using 32-bit stack slots instead of 64-bit stack slots for float32 values, at least on 32-bit platforms. On 64-bit platforms, STACK_SLOT_SIZE is still 8, so float32 values still use a full slot, though a subsequent patch here will fix that.

This also implements the beginning of a higher-level interface for StackSlotAllocator.

Comment 7

[Dan Gohman [:sunfish]]

Attachment: 32-bit-slots.patch

This patch implements using 4-byte stack slots for 32-bit values on 64-bit platforms. It makes STACK_SLOT_SIZE 4 on all platforms.

Also, this patch resolves all the divergences between these files:

js/src/jit/arm/IonFrames-arm.h
js/src/jit/shared/IonFrames-x86-shared.h

so that the only difference is the name of the include guard macro.

Comment 8

[Dan Gohman [:sunfish]]

Attachment: merge-ion-frames.patch

The previous patch resolved all the differences between IonFrames-arm.h and IonFrames-x86-shared.h. This patch eliminates these files and puts their contents into IonFrames.h and IonFrames.cpp.

Comment 9

[Dan Gohman [:sunfish]]

Attachment: int32-move.patch

This patch implements using 32-bit moves (e.g. movl on x86) for int32 values.

Comment 10

[Dan Gohman [:sunfish]]

Attachment: slots-bytes.patch

An earlier patch changed STACK_SLOT_SIZE from 8 to 4 on x64, and much of the changes involved removing uses of STACK_SLOT_SIZE. This patch removes all of the remaining uses of STACK_SLOT_SIZE. It makes local stack slots indexed by the byte. This simplifies code which was already using the slot index as a kind of differently-encoded byte index.

Comment 11

[Dan Gohman [:sunfish]]

Attachment: ionframe-float32.patch

This patch just changes the Snapshot reader to read float32 values with regular float32 loads, instead of doing 64-bit loads and punning them to float32.

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 8348081 [details] [diff] [review]
lallocation-kinds.patch

Review of attachment 8348081 [details] [diff] [review]:
-----------------------------------------------------------------

So pretty. r=me with nits addressed.

::: js/src/jit/CodeGenerator.cpp
@@ +979,5 @@
>  {
>      MTableSwitch *mir = ins->mir();
>      Label *defaultcase = mir->getDefault()->lir()->label();
>      const LAllocation *temp;
> + 

Nit: some trailing whitespace here.

::: js/src/jit/LinearScan.cpp
@@ +889,5 @@
>                  finishedDoubleSlots_.append(interval);
> +#ifdef JS_NUNBOX32
> +            else if (IsNunbox(mine))
> +                finishedNunboxSlots_.append(interval);
> +#endif

There's an outer "if (!IsNunbox(mine)) {", so this branch will never be taken and should be removed.

Comment 13

[Jan de Mooij [:jandem]]

Comment on attachment 8348086 [details] [diff] [review]
rename-float-to-float32.patch

Review of attachment 8348086 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/arm/MacroAssembler-arm.h
@@ +54,1 @@
>                                Condition c = Always);

Nit: fix indentation, add two spaces.

Comment 14

[Jan de Mooij [:jandem]]

Comment on attachment 8348088 [details] [diff] [review]
float32-move.patch

Review of attachment 8348088 [details] [diff] [review]:
-----------------------------------------------------------------

Yay.

Comment 15

[Jan de Mooij [:jandem]]

Comment on attachment 8348097 [details] [diff] [review]
merge-ion-frames.patch

Review of attachment 8348097 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!

::: js/src/jit/IonFrames.h
@@ +301,5 @@
>      JS_ASSERT(iter.isScripted());
>      return iter.script();
>  }
>  
> +#define ION_FRAME_DOMGETTER ((IonCode *)0x1)

While you're here, maybe turn these #ifdefs into |static const IonCode *... = | ?

Comment 16

[Jan de Mooij [:jandem]]

Comment on attachment 8348100 [details] [diff] [review]
int32-move.patch

Review of attachment 8348100 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/LIR.h
@@ +529,5 @@
>            case MIRType_Boolean:
>            case MIRType_Int32:
> +            // The stack slot allocator doesn't currently support allocating
> +            // 1-byte slots, so for now we lower MIRType_Boolean into INT32.
> +            JS_STATIC_ASSERT(sizeof(bool) <= sizeof(int32_t));

Nit: use static_assert()

Comment 17

[Dan Gohman [:sunfish]]

Comments addressed; checked in here:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=afe7a6c1c9b2

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/58b632847678
https://hg.mozilla.org/mozilla-central/rev/2b312f9d2449
https://hg.mozilla.org/mozilla-central/rev/d4767787f13d
https://hg.mozilla.org/mozilla-central/rev/ccafed92bd10
https://hg.mozilla.org/mozilla-central/rev/981fc6e3f13f
https://hg.mozilla.org/mozilla-central/rev/48a718d4df56
https://hg.mozilla.org/mozilla-central/rev/f6097fcbd39b
https://hg.mozilla.org/mozilla-central/rev/02ead7ab22c4
https://hg.mozilla.org/mozilla-central/rev/16f4eae9ae00
https://hg.mozilla.org/mozilla-central/rev/88a94ece2346
https://hg.mozilla.org/mozilla-central/rev/afe7a6c1c9b2