wasmtime / PR #1905 Legalize fmin/fmax with NaN quieting ... · git-wasmtime

Oh, good point: the documentation doesn't make sense (copied from the narrow_avx instructions). I'll change it.

However, it still might make sense to keep this as a separate function from a code clarity perspective: these are completely different codegen approaches and expand_minmax is already complex enough as-is.

Wasmtime GitHub notifications bot (Jun 19 2020 at 19:16):

abrown updated PR #1905 from legalize-fmin-fmax-slow to master:

This is identical to #1821 but without the guarantee flags.

Wasmtime GitHub notifications bot (Jun 25 2020 at 15:34):

bnjbvr submitted PR Review.

Wasmtime GitHub notifications bot (Jun 25 2020 at 15:34):

bnjbvr submitted PR Review.

Wasmtime GitHub notifications bot (Jun 25 2020 at 15:34):

bnjbvr created PR Review Comment:

nit: use unimplemented!() or todo!() here

Wasmtime GitHub notifications bot (Jun 25 2020 at 15:34):

bnjbvr created PR Review Comment:

nit: Can we get rid of is_max by just looking at the opcode, instead of the one use of is_max?

Wasmtime GitHub notifications bot (Jun 25 2020 at 15:34):

bnjbvr created PR Review Comment:

Can you point in the wasm SIMD spec document (in a Github comment) where it is stated that it's to lose the NaN's sign, please? I couldn't find anything in the proposal design doc.

Wasmtime GitHub notifications bot (Jun 25 2020 at 15:34):

bnjbvr created PR Review Comment:

Can you move these definitions (as well as block comment) closer to where they're used below?

Wasmtime GitHub notifications bot (Jun 25 2020 at 15:34):

bnjbvr created PR Review Comment:

Please add more tests here too.

Wasmtime GitHub notifications bot (Jun 25 2020 at 15:34):

bnjbvr created PR Review Comment:

Can you add a few more tests, please:

a test with only one NaN in one lane of one input (should return cNaN for this lane)

a test with an arithmetic (non-canonical) NaN in one lane of one input (should cNaN too), hopefully our parser supports this :)

a test with -NaN in one lane of one vector, and +1 in the same lane of the other vector?

Wasmtime GitHub notifications bot (Jun 25 2020 at 18:48):

alexcrichton edited PR #1905 from legalize-fmin-fmax-slow to main:

This is identical to #1821 but without the guarantee flags.

Wasmtime GitHub notifications bot (Jun 25 2020 at 20:18):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Jun 25 2020 at 20:18):

abrown created PR Review Comment:

We could but then it would be let (x, y, x86_opcode, clif_opcode) and we would have to do the opcode comparison twice (once in the match and below in the current if using is_max).

Wasmtime GitHub notifications bot (Jun 25 2020 at 20:47):

abrown updated PR #1905 from legalize-fmin-fmax-slow to main:

This is identical to #1821 but without the guarantee flags.

Wasmtime GitHub notifications bot (Jun 25 2020 at 20:49):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Jun 25 2020 at 20:49):

abrown created PR Review Comment:

I'm going to merge this as-is but I can make a subsequent change to let (x, y, x86_opcode, clif_opcode) if you prefer that.

Wasmtime GitHub notifications bot (Jun 25 2020 at 21:48):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Jun 25 2020 at 21:48):

abrown created PR Review Comment:

Yeah, I added the link here and in enc_tables.rs but here it is as well: https://webassembly.github.io/spec/core/bikeshed/index.html#nan-propagation%E2%91%A0

Wasmtime GitHub notifications bot (Jun 25 2020 at 21:48):

abrown merged PR #1905.

Last updated: Oct 23 2024 at 20:03 UTC