Closed
Bug 1435369
Opened 6 years ago
Closed 6 years ago
Implement non-trapping floating-point conversion proposal
Categories
(Core :: JavaScript Engine: JIT, enhancement, P3)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: sunfish, Assigned: sunfish)
References
Details
Attachments
(1 file, 5 obsolete files)
136.87 KB,
patch
|
sunfish
:
review+
|
Details | Diff | Splinter Review |
Attached is a work-in-progress patch for the non-trapping float-to-int conversion proposal [0], which is in the Implementation Phase. So far, this includes platform-independent code and x64 support (Baseline and Ion). The feature is controlled by a compile-time flag which is only enabled on Nightly. [0] https://github.com/WebAssembly/nontrapping-float-to-int-conversions/
Comment 1•6 years ago
|
||
\o/
Assignee | ||
Comment 2•6 years ago
|
||
Updated with ARM and x86 support. I'll run some more tests before requesting review.
Attachment #8947957 -
Attachment is obsolete: true
Assignee | ||
Comment 3•6 years ago
|
||
Updated with various cleanups. This version takes advantage of the fact that ARM's vcvt performs the desired saturation directly and doesn't need OOL code.
Assignee | ||
Comment 5•6 years ago
|
||
It's similar to the sign-ext proposal, which blocks bug 1188259, so I'll do that here too.
Blocks: wasm
Assignee | ||
Comment 6•6 years ago
|
||
Rebased. https://treeherder.mozilla.org/#/jobs?repo=try&revision=5550c5aab217420783a901c542bac6ad442b46f7
Attachment #8948416 -
Attachment is obsolete: true
Attachment #8949481 -
Flags: review?(luke)
Comment 7•6 years ago
|
||
Comment on attachment 8949481 [details] [diff] [review] nontrapping.patch Review of attachment 8949481 [details] [diff] [review]: ----------------------------------------------------------------- (Sorry for taking so long to review.) Wow, that was a lot of work, with all the modes in all the backends in all the tiers in all the directions. Thanks! Only nits: ::: js/src/jit/MacroAssembler.h @@ +20,5 @@ > +static const TruncFlags TRUNC_UNSIGNED = TruncFlags(1) << 0; > +static const TruncFlags TRUNC_SATURATING = TruncFlags(1) << 1; > + > +} // namespace jit > +} // namespace js nit: could this hunk be moved into IonTypes.h or some other place? It seems a bit awkward in the middle of the #includes. ::: js/src/wasm/WasmBaselineCompile.cpp @@ +7331,5 @@ > + if (callee == SymbolicAddress::TruncateDoubleToUint64) > + flags |= TRUNC_UNSIGNED; > + if (callee == SymbolicAddress::SaturatingTruncateDoubleToInt64 || > + callee == SymbolicAddress::SaturatingTruncateDoubleToUint64) > + flags |= TRUNC_SATURATING; nit: { on \n after end of condition and before 'flags |=' ::: js/src/wasm/WasmBinaryConstants.h @@ +332,5 @@ > > inline bool > IsPrefixByte(uint8_t b) > { > + return b >= uint8_t(Op::NumericPrefix); nit: maybe add an explicit "FirstPrefix" enumerator to Op and use that here? @@ +337,3 @@ > } > > +// Opcodes from numeric proposal as of Feb 1, 2018 nit: If the proposal changes or fails to reach the final stage we'll change/remove these, so I think no need to date (dates in comments always look increasingly, well, out-of-date ;) or mention "proposal".
Attachment #8949481 -
Flags: review?(luke) → review+
Assignee | ||
Comment 8•6 years ago
|
||
Review comments addressed. Carrying forward r+.
Attachment #8949481 -
Attachment is obsolete: true
Attachment #8951082 -
Flags: review+
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 9•6 years ago
|
||
:sunfish the patch failed to apply, could you please provide an updated patch?
Flags: needinfo?(sunfish)
Assignee | ||
Comment 10•6 years ago
|
||
Rebased.
Attachment #8951082 -
Attachment is obsolete: true
Flags: needinfo?(sunfish)
Attachment #8951330 -
Flags: review+
Comment 11•6 years ago
|
||
Pushed by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/mozilla-inbound/rev/25900f3b9936 Implement non-trapping float-to-int conversions for WebAssembly r=luke
Keywords: checkin-needed
Comment 13•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/25900f3b9936
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in
before you can comment on or make changes to this bug.
Description
•