Closed Bug 1448329 Opened 2 years ago Closed 2 years ago

Consider removing static typed array accesses

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox60 --- fixed
firefox61 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

MLoadTypedArrayElementStatic and MStoreTypedArrayElementStatic are only used on 32-bit x86 and only for asm.js-like code that uses singleton arrays.

Removing these instructions also means we don't have to worry about Spectre mitigations for them.
Attached patch PatchSplinter Review
Attachment #8962289 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8962289 [details] [diff] [review]
Patch

nbp is at the Rust all hands this week.
Attachment #8962289 - Flags: review?(nicolas.b.pierron) → review?(tcampbell)
Comment on attachment 8962289 [details] [diff] [review]
Patch

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

That's a nice diffstat! Argument for removal sounds reasonable.
Attachment #8962289 - Flags: review?(tcampbell) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9db236e8d1de
Remove 32-bit-x86-only static typed array access optimization. r=tcampbell
Priority: -- → P3
https://hg.mozilla.org/mozilla-central/rev/9db236e8d1de
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment on attachment 8962289 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: N/A
[User impact if declined]: We want to uplift Spectre fixes in 61 to 60.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Just removes a 32-bit-only optimization.
[String changes made/needed]: None.
Attachment #8962289 - Flags: approval-mozilla-beta?
Comment on attachment 8962289 [details] [diff] [review]
Patch

Spectre mitigation needed for 60. Approved for 60.0b11.
Attachment #8962289 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Needs a rebased patch for Beta uplift.
Flags: needinfo?(jdemooij)
You need to log in before you can comment on or make changes to this bug.