1421993 - Wasm baseline: Clean up scratch register management further

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P1)

Description

[Lars T Hansen [:lth]]

Scratch registers were cleaned up some in bug 1419025, but while working on the ARM64 port I found a way to clean them up further via a simple subclassing scheme.  This also simplifies the initialization of scratch registers some (see bug 1419034 comment 7).

Comment 1

[Lars T Hansen [:lth]]

Attachment: Bug 1421993 - rabaldr, simplify scratch register management.

The idea here is that on quite a few platforms we will not necessarily
want to use the assembler's ScratchRegisterScope (for any type),
because the assembler uses its scratch registers very actively, as
does rabaldr.  So on those platforms we want our own.  Same on x86,
where there isn't a scratch register.

Anyhow, to make that happen we generalize the scratch register
management here so that it can use the platform's when that is
appropriate (x64 for all, x86 for float registers) and have its own
when that is appropriate (most other platforms).  This is managed with
ifdefs set up in the configuration section.

The net result is fewer, and more easily managed, platform
dependencies.

Review commit: https://reviewboard.mozilla.org/r/205040/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/205040/

Comment 2

[Lars T Hansen [:lth]]

Attachment: Bug 1421993 - rabaldr, abstract platform-specific registers and clean up ifdefs.

On most platforms we have some platform-dependent registers that are
used for specific purposes.  These need to be wrapped in the type-safe
register wrappers.  This patch factors that wrapping into a
SpecificRegs struct that is defined per-platform and included in
BaseCompiler.  This moves platform dependencies out of BaseCompiler
and toward the top of the file, where they belong.

Also, I took the opportunity to further clean up some ifdefs and
document how I use JS_64BIT vs JS_PUNBOX64.

Review commit: https://reviewboard.mozilla.org/r/205042/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/205042/

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8934090 [details]
Bug 1421993 - rabaldr, abstract platform-specific registers and clean up ifdefs.

https://reviewboard.mozilla.org/r/205042/#review210616

SHIPIT! Are there/do we expect some platforms to be JS_64BIT while not being JS_PUNBOX64?

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8934089 [details]
Bug 1421993 - rabaldr, simplify scratch register management.

https://reviewboard.mozilla.org/r/205040/#review210610

I like the idea, but I don't like #define for statically known values. Also one new macro is never defined, unless I missed something? I'd like to have another look please, otherwise the patch looks good.

::: js/src/wasm/WasmBaselineCompile.cpp:152
(Diff revision 1)
>  
> +#if defined(JS_CODEGEN_ARM64) || defined(JS_CODEGEN_NONE) || \
> +    defined(JS_CODEGEN_MIPS32) || defined(JS_CODEGEN_MIPS64)
> +# define RABALDR_SCRATCH_I32  Register::Invalid()
> +# define RABALDR_SCRATCH_F32  InvalidFloatReg
> +# define RABALDR_SCRATCH_F64  InvalidFloatReg

Instead of defines, can you make them all `static const Register/FloatRegister` or `constexpr`, please?

(I see these are used for testing in `ifdef` below, but this should be a separate defined key with no value, like the CALLOUT macro markers, I think)

::: js/src/wasm/WasmBaselineCompile.cpp:193
(Diff revision 1)
> +
> +// The compiler will not use both the F32 and F64 scratch registers at the same
> +// time and they can alias (without necessarily being equal, of course).  Try to
> +// ensure that the configuration is sane if so.
> +
> +#if defined(RABALDR_SCRATCH_F32_ALIASES_F64)

This doesn't seem to be defined anywhere? It should be on ARM32 at least.

::: js/src/wasm/WasmBaselineCompile.cpp:500
(Diff revision 1)
> +        MOZ_ASSERT(availFPU.has(RABALDR_SCRATCH_F32));
> +# endif
> +        if (RABALDR_SCRATCH_F64 != RegF64::Invalid())
> +            availFPU.take(RABALDR_SCRATCH_F64);
> +# if defined(RABALDR_SCRATCH_F32_ALIASES_F64)
> +        MOZ_ASSERT(!availFPU.has(RABALDR_SCRATCH_F32));

I think this assertion will blow up when RABALDR_SCRATCH_F64 is an invalid register.

::: js/src/wasm/WasmBaselineCompile.cpp:510
(Diff revision 1)
>          allGPR = availGPR;
>          allFPU = availFPU;
>  #endif
>      }
>  
> +    enum class Scratch {

maybe ScratchKind, or ScratchRegisterKind? or even ScratchRegisterType?

::: js/src/wasm/WasmBaselineCompile.cpp:517
(Diff revision 1)
> +        F32 = 2,
> +        F64 = 4
> +    };
> +
>  #ifdef DEBUG
> -    bool scratchRegisterTaken() const {
> +    bool scratchRegisterTaken(Scratch s) const {

nit: can you prefix with `is`, since this method returns a bool, please?

Comment 5

[Lars T Hansen [:lth]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #3)
> Comment on attachment 8934090 [details]
> Bug 1421993 - Abstract platform-specific registers, cleanup ifdefs.
> 
> https://reviewboard.mozilla.org/r/205042/#review210616
> 
> SHIPIT! Are there/do we expect some platforms to be JS_64BIT while not being
> JS_PUNBOX64?

That's a question I've been asking myself too.  Technically JS_PUNBOX64 relates to the "reg" vs "high"/"low" registers in a Register64.  And JS_PUNBOX64 almost certainly implies JS_64BIT, but that does not give us much.

You could imagine a 32-bit architecture with a provision for "extended" 64-bit registers, in fact the "x32" architecture that sometimes comes up in discussions would be that (essentially, running 32-bit code on x64 to have access to more registers and other goodies, https://en.wikipedia.org/wiki/X32_ABI).  This 32-bit architecture would have different Register64 layout and would not be !JS_PUNBOX64, but it would also be !JS_64BIT.  Were we to support it we would only need to look at the bits of code that are !JS_PUNBOX64.

Indeed most 64-bit CPUs I know have good support for 32-bit code and could use a similar scheme for data compactness.  Even if x32 seems to have mostly died it's not particularly far-fetched.  One could argue Wasm is one such scheme :)

Comment 6

[Lars T Hansen [:lth]]

Comment on attachment 8934089 [details]
Bug 1421993 - rabaldr, simplify scratch register management.

https://reviewboard.mozilla.org/r/205040/#review211432

::: js/src/wasm/WasmBaselineCompile.cpp:193
(Diff revision 1)
> +
> +// The compiler will not use both the F32 and F64 scratch registers at the same
> +// time and they can alias (without necessarily being equal, of course).  Try to
> +// ensure that the configuration is sane if so.
> +
> +#if defined(RABALDR_SCRATCH_F32_ALIASES_F64)

This construct actually comes in from the ARM64 WIP and it's a little tricky.

This define is only meant to be defined if the *private* scratch registers are defined and alias; it has no bearing on the standard scratch registers (which we use on x86, x64, and ARM32).

I have removed this thing since it's not currently in use, it's easy to add it back later and we know it can be done.

Speaking of ARM32, while it is true that the standard float scratch registers alias here, it is far from obvious.  The reason is that the float32 scratch register defn on ARM references a double register 'd30' that does not have a single component, so that's silly.  But: other parts of the code seem to assume this should be 's30', which would be right, and the 'd30' register has a bit value that does not actually fit in the code_ field of a VFPRegister but would silently coerce to the value of 's30'...  I guess I'll investigate and file a followup bug.

::: js/src/wasm/WasmBaselineCompile.cpp:500
(Diff revision 1)
> +        MOZ_ASSERT(availFPU.has(RABALDR_SCRATCH_F32));
> +# endif
> +        if (RABALDR_SCRATCH_F64 != RegF64::Invalid())
> +            availFPU.take(RABALDR_SCRATCH_F64);
> +# if defined(RABALDR_SCRATCH_F32_ALIASES_F64)
> +        MOZ_ASSERT(!availFPU.has(RABALDR_SCRATCH_F32));

Issue went away because the handling of aliases was removed, for now.

::: js/src/wasm/WasmBaselineCompile.cpp:510
(Diff revision 1)
>          allGPR = availGPR;
>          allFPU = availFPU;
>  #endif
>      }
>  
> +    enum class Scratch {

Oy, verbose.  I guess "ScratchKind" is manageable.

::: js/src/wasm/WasmBaselineCompile.cpp:517
(Diff revision 1)
> +        F32 = 2,
> +        F64 = 4
> +    };
> +
>  #ifdef DEBUG
> -    bool scratchRegisterTaken() const {
> +    bool scratchRegisterTaken(Scratch s) const {

Yes.

Comment 7

[Lars T Hansen [:lth]]

Comment on attachment 8934089 [details]
Bug 1421993 - rabaldr, simplify scratch register management.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/205040/diff/1-2/

Comment 8

[Lars T Hansen [:lth]]

Comment on attachment 8934090 [details]
Bug 1421993 - rabaldr, abstract platform-specific registers and clean up ifdefs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/205042/diff/1-2/

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8934089 [details]
Bug 1421993 - rabaldr, simplify scratch register management.

https://reviewboard.mozilla.org/r/205040/#review211452

Thanks!

::: js/src/wasm/WasmBaselineCompile.cpp:195
(Diff revision 2)
>  # define RABALDR_INT_DIV_I64_CALLOUT
>  # define RABALDR_I64_TO_FLOAT_CALLOUT
>  # define RABALDR_FLOAT_TO_I64_CALLOUT
>  #endif
>  
> +

nit: blank line

Comment 10

[Lars T Hansen [:lth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=42eb187a51cbe685a46b1384dc2a86fa2d3fc96f

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9aa0390c430
rabaldr, simplify scratch register management. r=bbouvier
https://hg.mozilla.org/integration/mozilla-inbound/rev/e49d8769f50a
rabaldr, abstract platform-specific registers and clean up ifdefs. r=bbouvier

Comment 12

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/b9aa0390c430
https://hg.mozilla.org/mozilla-central/rev/e49d8769f50a