Stream: git-wasmtime

Topic: wasmtime / PR #2248 [machinst x64]: enable packed integer...

view this post on Zulip Wasmtime GitHub notifications bot (Sep 30 2020 at 18:08):

abrown opened PR #2248 from comparisons to main:

This change was originally intended to implement the remaining lane operations (any_true, all_true, and splat) in order to enable the simd_lane.wast spec test; however, I had forgotten that comparisons were necessary for some of those sequences so this change adds those as well.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 30 2020 at 18:11):

abrown updated PR #2248 from comparisons to main:

This change was originally intended to implement the remaining lane operations (any_true, all_true, and splat) in order to enable the simd_lane.wast spec test; however, I had forgotten that comparisons were necessary for some of those sequences so this change adds those as well.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 30 2020 at 19:20):

abrown updated PR #2248 from comparisons to main:

This change was originally intended to implement the remaining lane operations (any_true, all_true, and splat) in order to enable the simd_lane.wast spec test; however, I had forgotten that comparisons were necessary for some of those sequences so this change adds those as well.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 30 2020 at 19:21):

abrown requested bnjbvr for a review on PR #2248.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 30 2020 at 19:21):

abrown requested cfallin and bnjbvr for a review on PR #2248.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 30 2020 at 19:46):

abrown updated PR #2248 from comparisons to main:

This change was originally intended to implement the remaining lane operations (any_true, all_true, and splat) in order to enable the simd_lane.wast spec test; however, I had forgotten that comparisons were necessary for some of those sequences so this change adds those as well.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 30 2020 at 23:26):

cfallin submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 30 2020 at 23:26):

cfallin submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 30 2020 at 23:26):

cfallin created PR Review Comment:

I'm a little uneasy about the hidden semantics here -- it feels like a leaky abstraction. (I know these are pre-existing, but the confusion is appearing now that I see this comment...). Could we perhaps have an enum indicating whether the output of a r/m, r instruction depends on the initial value or not (i.e., IsModOrDef::Def vs. IsModOrDef::Mod)? That would make things more clear, I think.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 30 2020 at 23:26):

cfallin created PR Review Comment:

I think we should have a bit more here to describe when this might be useful: basically, when we have a sequence of instructions whose register usages are nominally mods, but such that the combination of operations creates a result that is independent of the initial register value. It's thus semantically a def, not a mod, when all the instructions are taken together, so we want to ensure the register is defined (its live-range starts) prior to the sequence to keep analyses happy.

It's worth briefly mentioning the alternative as well, namely a compound instruction that somehow encapsulates the others and reports its own defs/uses/mods; this adds complexity (the instruction list is no longer flat) and requires knowledge about semantics and initial-value independence anyway.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 30 2020 at 23:26):

cfallin created PR Review Comment:

s/declaring/declare/

view this post on Zulip Wasmtime GitHub notifications bot (Sep 30 2020 at 23:26):

cfallin created PR Review Comment:

Let's pull this out into a toplevel helper; I'd rather keep the main match from exploding in size with nested functions :-)

view this post on Zulip Wasmtime GitHub notifications bot (Sep 30 2020 at 23:26):

cfallin created PR Review Comment:

What is w_bit (is it is64 as elsewhere)?

view this post on Zulip Wasmtime GitHub notifications bot (Sep 30 2020 at 23:26):

cfallin created PR Review Comment:

Also, I think it might be good to choose a better name. In the spirit of Rust's mem::uninitialized(), can we have XmmUninitializedValue that defines a register to an uninitialized value?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 01 2020 at 09:31):

bnjbvr submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 01 2020 at 09:31):

bnjbvr submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 01 2020 at 09:31):

bnjbvr created PR Review Comment:

Not a huge fan of this; instead could we just xorps the destination register, in the place where it's used? (There's already special handling of xorps X X in regalloc.) The slow down caused by the additional instruction in the pipeline may be recouped by the speedup caused by the lane dependency clearing.

If we really really have to resort to a vcode instruction for this, please put it at the bottom of this list (there's a section for instructions that don't generate new code); Chris's name suggestion sounds good to me.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 01 2020 at 18:45):

abrown submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 01 2020 at 18:45):

abrown created PR Review Comment:

REX.W... changed.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 01 2020 at 19:06):

abrown submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 01 2020 at 19:06):

abrown created PR Review Comment:

I agree that this is a leaky abstraction but I'm not confident IsModOrDef plugs the hole; let me open an issue to discuss: https://github.com/bytecodealliance/wasmtime/issues/2252.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 01 2020 at 19:24):

abrown updated PR #2248 from comparisons to main:

This change was originally intended to implement the remaining lane operations (any_true, all_true, and splat) in order to enable the simd_lane.wast spec test; however, I had forgotten that comparisons were necessary for some of those sequences so this change adds those as well.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 01 2020 at 19:25):

abrown submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 01 2020 at 19:25):

abrown created PR Review Comment:

Good point; that was the intent but I forgot to :big_smile:.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 01 2020 at 19:28):

abrown updated PR #2248 from comparisons to main:

This change was originally intended to implement the remaining lane operations (any_true, all_true, and splat) in order to enable the simd_lane.wast spec test; however, I had forgotten that comparisons were necessary for some of those sequences so this change adds those as well.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 01 2020 at 19:30):

abrown updated PR #2248 from comparisons to main:

This change was originally intended to implement the remaining lane operations (any_true, all_true, and splat) in order to enable the simd_lane.wast spec test; however, I had forgotten that comparisons were necessary for some of those sequences so this change adds those as well.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 02 2020 at 15:29):

abrown merged PR #2248.

Last updated: Oct 23 2024 at 20:03 UTC