abrown opened PR #2248 from comparisons
to main
:
This change was originally intended to implement the remaining lane operations (
any_true
,all_true
, andsplat
) in order to enable thesimd_lane.wast
spec test; however, I had forgotten that comparisons were necessary for some of those sequences so this change adds those as well.
abrown updated PR #2248 from comparisons
to main
:
This change was originally intended to implement the remaining lane operations (
any_true
,all_true
, andsplat
) in order to enable thesimd_lane.wast
spec test; however, I had forgotten that comparisons were necessary for some of those sequences so this change adds those as well.
abrown updated PR #2248 from comparisons
to main
:
This change was originally intended to implement the remaining lane operations (
any_true
,all_true
, andsplat
) in order to enable thesimd_lane.wast
spec test; however, I had forgotten that comparisons were necessary for some of those sequences so this change adds those as well.
abrown requested bnjbvr for a review on PR #2248.
abrown requested cfallin and bnjbvr for a review on PR #2248.
abrown updated PR #2248 from comparisons
to main
:
This change was originally intended to implement the remaining lane operations (
any_true
,all_true
, andsplat
) in order to enable thesimd_lane.wast
spec test; however, I had forgotten that comparisons were necessary for some of those sequences so this change adds those as well.
cfallin submitted PR Review.
cfallin submitted PR Review.
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.
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
mod
s, but such that the combination of operations creates a result that is independent of the initial register value. It's thus semantically adef
, not amod
, 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.
cfallin created PR Review Comment:
s/declaring/declare/
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 :-)
cfallin created PR Review Comment:
What is
w_bit
(is itis64
as elsewhere)?
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 haveXmmUninitializedValue
that defines a register to an uninitialized value?
bnjbvr submitted PR Review.
bnjbvr submitted PR Review.
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.
abrown submitted PR Review.
abrown created PR Review Comment:
REX.W... changed.
abrown submitted PR Review.
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.
abrown updated PR #2248 from comparisons
to main
:
This change was originally intended to implement the remaining lane operations (
any_true
,all_true
, andsplat
) in order to enable thesimd_lane.wast
spec test; however, I had forgotten that comparisons were necessary for some of those sequences so this change adds those as well.
abrown submitted PR Review.
abrown created PR Review Comment:
Good point; that was the intent but I forgot to :big_smile:.
abrown updated PR #2248 from comparisons
to main
:
This change was originally intended to implement the remaining lane operations (
any_true
,all_true
, andsplat
) in order to enable thesimd_lane.wast
spec test; however, I had forgotten that comparisons were necessary for some of those sequences so this change adds those as well.
abrown updated PR #2248 from comparisons
to main
:
This change was originally intended to implement the remaining lane operations (
any_true
,all_true
, andsplat
) in order to enable thesimd_lane.wast
spec test; however, I had forgotten that comparisons were necessary for some of those sequences so this change adds those as well.
abrown merged PR #2248.
Last updated: Dec 23 2024 at 12:05 UTC