Stream: git-wasmtime

Topic: wasmtime / PR #2279 [machinst x64]: add packed bitwise op...

view this post on Zulip Wasmtime GitHub notifications bot (Oct 07 2020 at 22:57):

abrown opened PR #2279 from more-simd to main:

This implements packed and, and_not, or, xor, and bitselect and begins creating Inst methods for these as discussed in #2252.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 07 2020 at 22:57):

abrown requested bnjbvr for a review on PR #2279.

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

abrown updated PR #2279 from more-simd to main:

This implements packed and, and_not, or, xor, and bitselect and begins creating Inst methods for these as discussed in #2252.

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

bnjbvr submitted PR Review.

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

bnjbvr submitted PR Review.

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

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?

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

bnjbvr created PR Review Comment:

Should we refine this to is_vector && 128 bits?

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

bnjbvr created PR Review Comment:

nit: "of" is missing after "instead"

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

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".

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

bnjbvr created PR Review Comment:

Can you expand the names a bit, please? condition, if_true, if_false?

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

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.

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

abrown submitted PR Review.

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

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.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 08 2020 at 22:04):

abrown submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 08 2020 at 22:04):

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 the impl Inst so they still exist in Inst; I'll leave this PR up for you to take another look. I will add an InstHelper 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--and Inst::load and Inst::store are probably in the same class).

view this post on Zulip Wasmtime GitHub notifications bot (Oct 08 2020 at 22:12):

abrown updated PR #2279 from more-simd to main:

This implements packed and, and_not, or, xor, and bitselect and begins creating Inst methods for these as discussed in #2252.

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

abrown updated PR #2279 from more-simd to main:

This implements packed and, and_not, or, xor, and bitselect and begins creating Inst methods for these as discussed in #2252.

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

abrown submitted PR Review.

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

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.

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

bnjbvr submitted PR Review.

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

bnjbvr submitted PR Review.

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

bnjbvr created PR Review Comment:

Could we use input_to_reg_mem for if_false, then?

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

bnjbvr created PR Review Comment:

uber nit: can you add spaces before and after --, please?

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

abrown updated PR #2279 from more-simd to main:

This implements packed and, and_not, or, xor, and bitselect and begins creating Inst methods for these as discussed in #2252.

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

abrown merged PR #2279.

Last updated: Nov 22 2024 at 17:03 UTC