Bug 196217 – WebAssembly: Fix f32.min, f64.min and f64.max operations on NaN

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Tadeu Zagallo]

...

[Tadeu Zagallo]

Created attachment 365907 [details]
Patch

[EWS Watchlist]

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

[Saam Barati]

Comment on attachment 365907 [details]
Patch

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), ...)

[Tadeu Zagallo]

Created attachment 365980 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 365980 [details]
Patch for landing

Clearing flags on attachment: 365980

Committed r243514: <https://trac.webkit.org/changeset/243514>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/49285760>