Stream: git-wasmtime

Topic: wasmtime / PR #2819 x64: lower iabs.i64x2 using a single ...


view this post on Zulip Wasmtime GitHub notifications bot (Apr 08 2021 at 17:45):

abrown opened PR #2819 from inst-format-3 to main:

This introduces the mechanism for encoding EVEX instructions to the new backend (ported with slight changes from the old) and then uses it to improve the lowering of iabs.i64x2 from 5 instructions to 1 instruction (i.e. VPABSQ).

view this post on Zulip Wasmtime GitHub notifications bot (Apr 08 2021 at 17:57):

abrown requested cfallin for a review on PR #2819.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 08 2021 at 17:57):

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

view this post on Zulip Wasmtime GitHub notifications bot (Apr 08 2021 at 17:57):

abrown requested cfallin, bnjbvr and jlb6740 for a review on PR #2819.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 13 2021 at 18:36):

abrown updated PR #2819 from inst-format-3 to main.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 13 2021 at 19:00):

abrown has marked PR #2819 as ready for review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 13 2021 at 19:05):

abrown submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 13 2021 at 19:05):

abrown created PR Review Comment:

I wish there were a way to avoid this here and instead specify it up above when necessary--suggestions welcome because I can't think of a great way to do this without rewriting this entire file.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 13 2021 at 19:28):

abrown updated PR #2819 from inst-format-3 to main.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2021 at 14:55):

bnjbvr submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2021 at 14:55):

bnjbvr submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2021 at 14:55):

bnjbvr created PR Review Comment:

since value <= mask and mask is all ones, we don't need the value & mask here and could just use value

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2021 at 14:55):

bnjbvr created PR Review Comment:

nit: followed

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2021 at 14:55):

bnjbvr created PR Review Comment:

So the way it works right now is that it goes from the RealReg hardware encoding into an u8 then into this Register structure. Could instead the methods taking Register take a RealReg instead?

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2021 at 14:55):

bnjbvr created PR Review Comment:

Isn't there already another CodeSink trait somewhere in this crate too? I wonder if we could avoid this one, to reduce the number of concepts, and just read out of the MachBuffer internal vector, for testing purposes?

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2021 at 14:55):

bnjbvr created PR Review Comment:

Instead of having a single array with all the testing tuples, we could have several ones, one for each Flag combination we'd like to test. Then we could refactor this function so that it takes flags from a parameter and an array of test tuples, or something like this?

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2021 at 14:55):

bnjbvr created PR Review Comment:

Also, maybe not something to worry too much about, since there's no actual instruction selection test here. What was your concern?

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2021 at 15:30):

abrown submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2021 at 15:30):

abrown created PR Review Comment:

Yes, here but as you can see it would force this module to implement things it has no idea about and I wasn't too sure the old backend trait would be around forever. Also, I am trying to keep this module as dependency-free as possible so that in the future it could be used elsewhere--if I want to use this in the future to just encode instructions I won't want or need the additional MachBuffer methods and fields.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2021 at 15:30):

abrown edited PR Review Comment.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2021 at 15:33):

abrown created PR Review Comment:

I was under the impression that Rust will pare away those abstractions so I didn't worry too much about overhead. I am trying to keep this encoding code dependency-free (especially of external dependencies) so, like with CodeSink, I am using slightly different types/traits.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2021 at 15:33):

abrown submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2021 at 15:36):

abrown submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2021 at 15:36):

abrown created PR Review Comment:

Yeah, I guess that's true. It just looked weird to lump the AVX512 flag in with the baseline flags. Maybe in a follow-on PR I will try to factor out the "check these instructions" code so that I can do as you suggest above.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2021 at 15:37):

abrown updated PR #2819 from inst-format-3 to main.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2021 at 15:41):

abrown updated PR #2819 from inst-format-3 to main.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2021 at 17:28):

cfallin submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2021 at 17:28):

cfallin created PR Review Comment:

Maybe we could name the trait something else -- ByteSink or something?

It might even make sense to do the factoring in the other direction -- split out ByteSink from CodeSink and make the former a constraint on the latter (trait CodeSink : ByteSink { ... } with just the additional methods), but that's probably out-of-scope for this PR...

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2021 at 17:36):

abrown updated PR #2819 from inst-format-3 to main.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2021 at 18:53):

abrown merged PR #2819.


Last updated: Dec 23 2024 at 13:07 UTC