Closed Bug 1523941 Opened 10 months ago Closed 10 months ago

Enforce Scratch{Double,Float32}Scope throughout the pipeline

Categories

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

ARM64
All
enhancement

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox67 --- fixed

People

(Reporter: lth, Assigned: lth)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.

Depends on: 1524201
Depends on: 1524228

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.

Attachment #9040382 - Flags: review?(nicolas.b.pierron)

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

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);"
Attachment #9040382 - Flags: review?(nicolas.b.pierron) → review+

(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.

(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 :)

Priority: -- → P2
Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e565291ee5f6
Always use ScratchDoubleScope / ScratchFloat32Scope. r=nbp
Blocks: 1524481
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in before you can comment on or make changes to this bug.