-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
riscv64: Implement a few SIMD arithmetic ops #6268
riscv64: Implement a few SIMD arithmetic ops #6268
Conversation
These were accidentally reversed from what we declare in the isle emit helper
Looks like x86 does not implement it
c6eeba1
to
09447be
Compare
VecAluOpRRR::Vadd => write!(f, "vadd.vv"), | ||
VecAluOpRRR::Vsub => write!(f, "vsub.vv"), | ||
VecAluOpRRR::Vmul => write!(f, "vmul.vv"), | ||
VecAluOpRRR::Vmulh => write!(f, "vmulh.vv"), | ||
VecAluOpRRR::Vmulhu => write!(f, "vmulhu.vv"), | ||
VecAluOpRRR::Vand => write!(f, "vand.vv"), | ||
VecAluOpRRR::Vor => write!(f, "vor.vv"), | ||
VecAluOpRRR::Vxor => write!(f, "vxor.vv"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If these all follow the same pattern, one thing I found helpful for AVX was do do something like:
let mut s = format!("{self:?}");
s.make_ascii_lowercase();
s.push_str(".vv");
f.write_str(&s)
which can cut down on codegen times and additionally make this a bit easier to maintain. If the instructions have all sorts of different names though this may not work out well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also have another variant for all of these, which is v*.vx
. However, it should still be fairly easy to fit into that scheme when we do need it.
Edit: Hmm, It might not be so easy, because I was planning on renaming these enum arms into VaddVV
and VaddVX
which would then break that. But I don't mind doing it this way for now and figuring it out later.
target aarch64 | ||
target s390x | ||
set enable_simd | ||
target x86_64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mind copying over the target x86_64 has_sse41=false
line as well from the original test? (maybe lost through a rebase by accident)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also makes me idly think that perhaps there should be a ; skip: riscv
directive for filetests or similar, but not something to be added in this PR of course.
* riscv64: Swap order of `VecAluRRR` source registers These were accidentally reversed from what we declare in the isle emit helper * riscv64: Add SIMD `isub` * riscv64: Add SIMD `imul` * riscv64: Add `{u,s}mulhi` * riscv64: Add `b{and,or,xor}` * cranelift: Move `imul.i8x16` runtest to separate file Looks like x86 does not implement it * riscv64: Better formatting for `VecAluOpRRR` * cranelift: Enable x86 SIMD tests with `has_sse41=false`
👋 Hey,
This PR Implements a few more arithmetic ops, I didn't want to implement too many since I want to get the wasmtime testsuite working to get better test coverage.
Similarly to #6266, I've also switched the scalar rules to match only scalars, since most of them used
fits_in_64
which also matches vectors.I also had accidentally switched the order of the registers in
VecAluRRR
, which I only noticed when implementingisub
.Implemented ops are:
isub
imul
smulhi
umulhi
band
bor
bxor
This PR is based on #6266, I'll rebase when that lands.