1523941 - Enforce Scratch{Double,Float32}Scope throughout the pipeline

Status: RESOLVED FIXED

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

Description

[Lars T Hansen [:lth]]

The code is needlessly brittle and might be hiding bugs. Longer term (not too long) we should require the scopes everywhere and hide ScratchDoubleReg / ScratchFloat32Reg completely, but for now I'll settle for platform-independent code and ARM64 code.

Comment 1

[Lars T Hansen [:lth]]

Attachment: bug1523941-use-fp-scopes.patch

Passes all tests locally but running everything on try now. This change only did find the one bug (bug 1524201). But if you think this is acceptable, then once I land it I will file another bug to hide ScratchFloat32Reg + ScratchDoubleReg on tier-1 platforms so that the abuse does not continue.

Comment 2

[Lars T Hansen [:lth]]

Note, needs minor adjusment to compile for nojit (ScratchDoubleScope and ScratchFloat32Scope need to be defined).

Comment 3

[Nicolas B. Pierron [:nbp]]

Comment on attachment 9040382 [details] [diff] [review]
bug1523941-use-fp-scopes.patch

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

::: js/src/jit/MacroAssembler.cpp
@@ +2285,3 @@
>    if (outputType == MIRType::Float32 && hasMultiAlias()) {
> +    ScratchDoubleScope tmp(*this);
> +    unboxDouble(value, tmp);

Missing call to "convertDoubleToFloat32(tmp, output);"

Comment 4

[Lars T Hansen [:lth]]

(In reply to Nicolas B. Pierron [:nbp] from comment #3)

Comment on attachment 9040382 [details] [diff] [review]
bug1523941-use-fp-scopes.patch

Review of attachment 9040382 [details] [diff] [review]:

::: js/src/jit/MacroAssembler.cpp
@@ +2285,3 @@

if (outputType == MIRType::Float32 && hasMultiAlias()) {

• ScratchDoubleScope tmp(*this);
• unboxDouble(value, tmp);

Missing call to "convertDoubleToFloat32(tmp, output);"

No, it's actually there, look at the patch again - it's unchanged from before.

Comment 5

[Nicolas B. Pierron [:nbp]]

(In reply to Lars T Hansen [:lth] from comment #4)

(In reply to Nicolas B. Pierron [:nbp] from comment #3)

Missing call to "convertDoubleToFloat32(tmp, output);"

No, it's actually there, look at the patch again - it's unchanged from before.

Ok, then I have nothing else to say, except Thank you for doing this work :)

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e565291ee5f6
Always use ScratchDoubleScope / ScratchFloat32Scope. r=nbp

Comment 7

[Oana Pop-Rus]

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