Bug 196217 - WebAssembly: Fix f32.min, f64.min and f64.max operations on NaN
Summary: WebAssembly: Fix f32.min, f64.min and f64.max operations on NaN
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tadeu Zagallo
Keywords: InRadar
Depends on:
Reported: 2019-03-25 14:47 PDT by Tadeu Zagallo
Modified: 2019-03-26 12:31 PDT (History)
9 users (show)

See Also:

Patch (602.22 KB, patch)
2019-03-25 15:07 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch for landing (602.31 KB, patch)
2019-03-26 11:50 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tadeu Zagallo 2019-03-25 14:47:49 PDT
Comment 1 Tadeu Zagallo 2019-03-25 15:07:18 PDT
Created attachment 365907 [details]
Comment 2 EWS Watchlist 2019-03-25 15:10:04 PDT
This patch modifies one of the wasm.json files. Please ensure that any changes in one have been mirrored to the other. You can find the wasm.json files at "Source/JavaScriptCore/wasm/wasm.json" and "JSTests/wasm/wasm.json".
Comment 3 Saam Barati 2019-03-26 11:26:00 PDT
Comment on attachment 365907 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=365907&action=review

r=me with a few minor nits.

> Source/JavaScriptCore/ChangeLog:8
> +        Generalize the fix for f32.max from r243446 to all min and max float operations.

nit: Also worth one sentence on what r243446 fixed.

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:572
> +    enum MinOrMax { Min, Max };

nit: let's use `enum class`.

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:2792
> +    auto isEqualOp = minOrMax == Max ? andOp : orOp;

nit: this variable name is kind of a weird, since we're in the "isEqual" block, but this op isn't about equality.

I'd vote for just inlining this in the append.

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:2798
> +    auto moveOp = floatType == F32 ? MoveFloat : MoveDouble;

nit: I'd just inline this below into the append. Also, this can just be:
append(isLessThan, moveOpForValueType(floatType), ...)
Comment 4 Tadeu Zagallo 2019-03-26 11:50:29 PDT
Created attachment 365980 [details]
Patch for landing
Comment 5 WebKit Commit Bot 2019-03-26 12:29:09 PDT
Comment on attachment 365980 [details]
Patch for landing

Clearing flags on attachment: 365980

Committed r243514: <https://trac.webkit.org/changeset/243514>
Comment 6 WebKit Commit Bot 2019-03-26 12:29:11 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Radar WebKit Bug Importer 2019-03-26 12:31:29 PDT