Closed Bug 950703 Opened 6 years ago Closed 6 years ago

32-bit stack slots and 32-bit moves

Categories

(Core :: JavaScript Engine: JIT, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: sunfish, Assigned: sunfish)

References

Details

Attachments

(11 files)

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.
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.
Attachment #8348081 - Flags: review?(jdemooij)
This patch renames MoveOp::Kind to MoveOp::Type, to emphasize the analogy LAllocation::Kind : LDefinition::Type :: Move::Operand::Kind : MoveOp::Type.
Attachment #8348082 - Flags: review?(jdemooij)
Attached patch float-reg.patchSplinter Review
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.
Attachment #8348083 - Flags: review?(jdemooij)
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.
Attachment #8348086 - Flags: review?(jdemooij)
This patch implements the use of 32-bit moves, such as movss on x86, for float32 values.
Attachment #8348088 - Flags: review?(jdemooij)
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.
Attachment #8348090 - Flags: review?(jdemooij)
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.
Attachment #8348095 - Flags: review?(jdemooij)
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.
Attachment #8348097 - Flags: review?(jdemooij)
Attached patch int32-move.patchSplinter Review
This patch implements using 32-bit moves (e.g. movl on x86) for int32 values.
Attachment #8348100 - Flags: review?(jdemooij)
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.
Attachment #8348102 - Flags: review?(jdemooij)
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.
Attachment #8348104 - Flags: review?(jdemooij)
Blocks: 947711
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.
Attachment #8348081 - Flags: review?(jdemooij) → review+
Attachment #8348082 - Flags: review?(jdemooij) → review+
Attachment #8348083 - Flags: review?(jdemooij) → review+
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.
Attachment #8348086 - Flags: review?(jdemooij) → review+
Comment on attachment 8348088 [details] [diff] [review]
float32-move.patch

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

Yay.
Attachment #8348088 - Flags: review?(jdemooij) → review+
Attachment #8348090 - Flags: review?(jdemooij) → review+
Attachment #8348095 - Flags: review?(jdemooij) → review+
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 *... = | ?
Attachment #8348097 - Flags: review?(jdemooij) → review+
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()
Attachment #8348100 - Flags: review?(jdemooij) → review+
Attachment #8348102 - Flags: review?(jdemooij) → review+
Attachment #8348104 - Flags: review?(jdemooij) → review+
Duplicate of this bug: 931434
Depends on: 951527
Depends on: 951913
Depends on: 952286
Duplicate of this bug: 948853
You need to log in before you can comment on or make changes to this bug.