1226027 - Compress bits used in TypedRegisterSet::SetType by collapsing SIMD types

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jakob Stoklund Olesen [:jolesen]]

We are about to get a lot more supported SIMD types in IonMonkey from bug 1136226 (Int8x16, Int16x8) and bug 1160971 (Bool8x16, Bool16x8, Bool32x4).

Currently, FloatRegisters::ContentType enumerates all the types that can be stored in a float register, and TypedRegisterSet has a separate bit mask for each type. This is not going to scale well with the new SIMD types, and TypedRegisterSet won't be able to represent its bit mask in a uint64_t type.

We don't actually need to keep track of the exact data type stored in a FloatRegister. It is good enough to know how large the data type is. In particular, all of the 128-bit SIMD types could share a content type like this:

    enum ContentType {
        Single,     // 32-bit float.
        Double,     // 64-bit double.
        Simd128,    // 128-bit SIMD type (int32x4, bool16x8, etc).
        NumTypes
    };

The content type of a float register is mostly used in assertions, and when saving and restoring register sets. The saving and restoring only needs to know the data size in order to pick the right load/store instructions. It doesn't matter if the data is floating point or integers.

The NEON instruction set for ARM and the ARM64 SIMD instruction set don't have SIMD load/store instructions that care about the type of data in the SIMD registers, so they won't be affected by this change.

The SSE instruction set provides movaps, movapd, and movdqa for 4xf32, 2xf64, and integer SIMD types respectively. Using instructions from the wrong domain (e.g., xorps instead of pxor) may have a 1-cycle domain crossing penalty if the instruction is on the critical path. It is not clear that the effect would be measurable, and it will depend on the specific micro-architecture.

Comment 1

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Use Simd128 register content type.

In preparation for the addition of a new set of SIMD types, collapse all of the
128-bit SIMD types into a single content type for a FLoatRegister.

This saves bits in TypedRegisterSet and prevents us from overflowing the
uint64_t bit mask currently used.

This fixes x86 and x86-64 only. Most likely some ARM/MIPS changes are needed.

Comment 2

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Use Simd128 register content type.

In preparation for the addition of a new set of SIMD types, collapse all of the
128-bit SIMD types into a single content type for a FloatRegister.

This saves bits in TypedRegisterSet and prevents us from overflowing the
uint64_t bit mask currently used.

For consistency, provide global variables ReturnSimd128Reg and ScratchSimd128Reg.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7831ab1be80f

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8689622 [details] [diff] [review]
Use Simd128 register content type.

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

Thank you for your patch, and sorry for the long review. I trust your judgement and experience about cross domain penalties.

Should all the uses of the Simd128Register be replaced by a ScratchSimd{128}Scope instead?

I've retriggered a few tests in your try run, it seems it hadn't completed properly.

::: js/src/jit/x86-shared/Assembler-x86-shared.h
@@ +35,5 @@
>        : AutoFloatRegisterScope(masm, ScratchDoubleReg)
>      { }
>  };
>  
>  struct ScratchSimdScope : public AutoFloatRegisterScope

Perhaps worth renaming ScratchSimd128Scope?

Comment 4

[Jakob Stoklund Olesen [:jolesen]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #3)
> Should all the uses of the Simd128Register be replaced by a
> ScratchSimd{128}Scope instead?

I am not sure, but I think 'no'.

I masm code running after register allocation needs to use a scratch register, you have to guarantee that the register allocator didn't already use all the SIMD registers at that point. This means you need to reserve a scratch register so RA won't use it.

When you have a reserved register, you might as well use it directly instead of allocating it dynamically with ScratchSimdScope.

I haven't yet studied how the auto scopes, RA, and the scratch registers interact, though.

> >  struct ScratchSimdScope : public AutoFloatRegisterScope
> 
> Perhaps worth renaming ScratchSimd128Scope?

Yes, will do.

Comment 5

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Use Simd128 register content type.

Also rename ScratchSimdScope to ScratchSimd128Scope.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a5c636390e7

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/91c196b60306

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

There are compilation errors on mozilla-inbound, e.g.:

    c:/builds/moz2_slave/m-in-w64-000000000000000000000/build/src/js/src/jit/x64/Assembler-x64.cpp(61) : error C2039: 'asInt32x4' : is not a member of 'js::jit::FloatRegister'
    c:/builds/moz2_slave/m-in-w64-000000000000000000000/build/src/js/src/jit/x64/Assembler-x64.cpp(67) : error C2039: 'asFloat32x4' : is not a member of 'js::jit::FloatRegister'

Comment 8

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/8fa4b88f205c

Comment 9

[Jakob Stoklund Olesen [:jolesen]]

Thanks, Phil/David.

I missed some Windows-only code. With that fixed, here's a -p all try run:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=77e9efd88a1d

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a7a1efdcec6e

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a7a1efdcec6e