Wasmtime GitHub notifications bot (Apr 08 2021 at 17:45): 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.i64x2from 5 instructions to 1 instruction (i.e.VPABSQ).
Wasmtime GitHub notifications bot (Apr 08 2021 at 17:57):abrown requested cfallin for a review on PR #2819.
Wasmtime GitHub notifications bot (Apr 08 2021 at 17:57):abrown requested cfallin and bnjbvr for a review on PR #2819.
Wasmtime GitHub notifications bot (Apr 08 2021 at 17:57):abrown requested cfallin, bnjbvr and jlb6740 for a review on PR #2819.
Wasmtime GitHub notifications bot (Apr 13 2021 at 18:36):abrown updated PR #2819 from inst-format-3 to main.
Wasmtime GitHub notifications bot (Apr 13 2021 at 19:00):abrown has marked PR #2819 as ready for review.
Wasmtime GitHub notifications bot (Apr 13 2021 at 19:05):abrown submitted PR Review.
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.
Wasmtime GitHub notifications bot (Apr 13 2021 at 19:28):abrown updated PR #2819 from inst-format-3 to main.
Wasmtime GitHub notifications bot (Apr 15 2021 at 14:55):bnjbvr submitted PR Review.
Wasmtime GitHub notifications bot (Apr 15 2021 at 14:55):bnjbvr submitted PR Review.
Wasmtime GitHub notifications bot (Apr 15 2021 at 14:55):bnjbvr created PR Review Comment:
since
value <= maskandmaskis all ones, we don't need thevalue & maskhere and could just usevalue
Wasmtime GitHub notifications bot (Apr 15 2021 at 14:55):bnjbvr created PR Review Comment:
nit:
followed
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
RealReghardware encoding into an u8 then into thisRegisterstructure. Could instead the methods takingRegistertake aRealReginstead?
Wasmtime GitHub notifications bot (Apr 15 2021 at 14:55):bnjbvr created PR Review Comment:
Isn't there already another
CodeSinktrait somewhere in this crate too? I wonder if we could avoid this one, to reduce the number of concepts, and just read out of theMachBufferinternal vector, for testing purposes?
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
flagsfrom a parameter and an array of test tuples, or something like this?
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?
Wasmtime GitHub notifications bot (Apr 15 2021 at 15:30):abrown submitted PR Review.
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
MachBuffermethods and fields.
Wasmtime GitHub notifications bot (Apr 15 2021 at 15:30):abrown edited PR Review Comment.
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.
Wasmtime GitHub notifications bot (Apr 15 2021 at 15:33):abrown submitted PR Review.
Wasmtime GitHub notifications bot (Apr 15 2021 at 15:36):abrown submitted PR Review.
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.
Wasmtime GitHub notifications bot (Apr 15 2021 at 15:37):abrown updated PR #2819 from inst-format-3 to main.
Wasmtime GitHub notifications bot (Apr 15 2021 at 15:41):abrown updated PR #2819 from inst-format-3 to main.
Wasmtime GitHub notifications bot (Apr 15 2021 at 17:28):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Apr 15 2021 at 17:28):cfallin created PR Review Comment:
Maybe we could name the trait something else --
ByteSinkor something?It might even make sense to do the factoring in the other direction -- split out
ByteSinkfromCodeSinkand 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...
Wasmtime GitHub notifications bot (Apr 15 2021 at 17:36):abrown updated PR #2819 from inst-format-3 to main.
Wasmtime GitHub notifications bot (Apr 15 2021 at 18:53):abrown merged PR #2819.
Last updated: Oct 23 2024 at 20:03 UTC