Stream: git-wasmtime

Topic: wasmtime / PR #1905 Legalize fmin/fmax with NaN quieting ...


view this post on Zulip Wasmtime GitHub notifications bot (Jun 19 2020 at 18:52):

abrown requested bnjbvr for a review on PR #1905.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 19 2020 at 18:52):

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

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

view this post on Zulip Wasmtime GitHub notifications bot (Jun 19 2020 at 19:09):

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

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

view this post on Zulip Wasmtime GitHub notifications bot (Jun 19 2020 at 19:10):

bjorn3 submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 19 2020 at 19:10):

bjorn3 created PR Review Comment:

Can you use expand_minmax? If not, the reason is not clear from the doc comment on expand_minmax_vector.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 19 2020 at 19:10):

bjorn3 edited PR Review Comment.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 19 2020 at 19:15):

abrown submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 19 2020 at 19:15):

abrown created PR Review Comment:

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.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 25 2020 at 15:34):

bnjbvr submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 25 2020 at 15:34):

bnjbvr submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 25 2020 at 15:34):

bnjbvr created PR Review Comment:

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

view this post on Zulip 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?

view this post on Zulip 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.

view this post on Zulip 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?

view this post on Zulip Wasmtime GitHub notifications bot (Jun 25 2020 at 15:34):

bnjbvr created PR Review Comment:

Please add more tests here too.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 25 2020 at 15:34):

bnjbvr created PR Review Comment:

Can you add a few more tests, please:

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 25 2020 at 20:18):

abrown submitted PR Review.

view this post on Zulip 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).

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 25 2020 at 20:49):

abrown submitted PR Review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 25 2020 at 21:48):

abrown submitted PR Review.

view this post on Zulip 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

view this post on Zulip Wasmtime GitHub notifications bot (Jun 25 2020 at 21:48):

abrown merged PR #1905.


Last updated: Dec 23 2024 at 13:07 UTC