[webkit-reviews] review granted: [Bug 196217] WebAssembly: Fix f32.min, f64.min and f64.max operations on NaN : [Attachment 365907] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Tadeu Zagallo
<tzagallo at apple.com>'s request for review:
Bug 196217: WebAssembly: Fix f32.min, f64.min and f64.max operations on NaN
https://bugs.webkit.org/show_bug.cgi?id=196217

Attachment 365907: Patch

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




--- Comment #3 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 365907
  --> https://bugs.webkit.org/attachment.cgi?id=365907
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), ...)