Closed Bug 1435369 Opened 4 years ago Closed 4 years ago

Implement non-trapping floating-point conversion proposal


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




Tracking Status
firefox60 --- fixed


(Reporter: sunfish, Assigned: sunfish)




(1 file, 5 obsolete files)

Attached patch nontrapping.patch (obsolete) — 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.

Attached patch nontrapping.patch (obsolete) — Splinter Review
Updated with ARM and x86 support. I'll run some more tests before requesting review.
Attachment #8947957 - Attachment is obsolete: true
Attached patch nontrapping.patch (obsolete) — Splinter Review
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: nobody → sunfish
Attachment #8948038 - Attachment is obsolete: true
Dan, is this blocking any meta bug?
Priority: -- → P3
It's similar to the sign-ext proposal, which blocks bug 1188259, so I'll do that here too.
Blocks: wasm
Comment on attachment 8949481 [details] [diff] [review]

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+
Attached patch nontrapping.patch (obsolete) — Splinter Review
Review comments addressed. Carrying forward r+.
Attachment #8949481 - Attachment is obsolete: true
Attachment #8951082 - Flags: review+
Keywords: checkin-needed
:sunfish the patch failed to apply, could you please provide an updated patch?
Flags: needinfo?(sunfish)
Attachment #8951082 - Attachment is obsolete: true
Flags: needinfo?(sunfish)
Attachment #8951330 - Flags: review+
Pushed by
Implement non-trapping float-to-int conversions for WebAssembly r=luke
Keywords: checkin-needed
Duplicate of this bug: 1438540
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.