abrown opened PR #2279 from more-simd
to main
:
This implements packed
and
,and_not
,or
,xor
, andbitselect
and begins creatingInst
methods for these as discussed in #2252.
abrown requested bnjbvr for a review on PR #2279.
abrown updated PR #2279 from more-simd
to main
:
This implements packed
and
,and_not
,or
,xor
, andbitselect
and begins creatingInst
methods for these as discussed in #2252.
bnjbvr submitted PR Review.
bnjbvr submitted PR Review.
bnjbvr created PR Review Comment:
:+1: What do you think of putting it into a different impl of a (zero-side) struct called
InstHelper
(or dare I say,MacroAssembler
)? It'll help make a clear distinction between the low-level assembler instructions represented by Vcode, vs the slightly higher level logic that's provided here.Here and below: would it make sense to add non-SIMD types here, or put "simd" in the name if that's only achieved for SIMD instructions?
bnjbvr created PR Review Comment:
Should we refine this to
is_vector && 128 bits
?
bnjbvr created PR Review Comment:
nit: "of" is missing after "instead"
bnjbvr created PR Review Comment:
ditto (etc a few times below)
If that makes sense, we could create a helper for this particular test "is vector of 128 bits".
bnjbvr created PR Review Comment:
Can you expand the names a bit, please?
condition
,if_true
,if_false
?
bnjbvr created PR Review Comment:
Here (and below for the condition too): it's incorrect to get a writable register for input values. If the input vreg is used in more vcode instructions after this one, then its value will be clobbered by this particular vcode inst (and resulting machine code). We'll need temporaries here instead.
abrown submitted PR Review.
abrown created PR Review Comment:
I think my idea was to eventually add the scalar types here as well but the refactorings we talked about in #2252 are going to take a while so this is just the beginning.
abrown submitted PR Review.
abrown created PR Review Comment:
Um, after thinking about it a bit more I just stuffed the functions down in the
// Inst helpers.
block of theimpl Inst
so they still exist inInst
; I'll leave this PR up for you to take another look. I will add anInstHelper
if you really think it would help but I figured creating a new struct would be more confusing at this point so we should either remove these functions or discuss #2252 a bit more before refactoring. (We can always move them later--andInst::load
andInst::store
are probably in the same class).
abrown updated PR #2279 from more-simd
to main
:
This implements packed
and
,and_not
,or
,xor
, andbitselect
and begins creatingInst
methods for these as discussed in #2252.
abrown updated PR #2279 from more-simd
to main
:
This implements packed
and
,and_not
,or
,xor
, andbitselect
and begins creatingInst
methods for these as discussed in #2252.
abrown submitted PR Review.
abrown created PR Review Comment:
Ok, I think I did that correctly... I ported some more CLIF filetests to ensure no extra moves are required.
bnjbvr submitted PR Review.
bnjbvr submitted PR Review.
bnjbvr created PR Review Comment:
Could we use
input_to_reg_mem
forif_false
, then?
bnjbvr created PR Review Comment:
uber nit: can you add spaces before and after
--
, please?
abrown updated PR #2279 from more-simd
to main
:
This implements packed
and
,and_not
,or
,xor
, andbitselect
and begins creatingInst
methods for these as discussed in #2252.
abrown merged PR #2279.
Last updated: Dec 23 2024 at 12:05 UTC