riscv64: Initial SIMD Vector Implementation #6240
Conversation
The SEW name is correct, but only for VType. We also use this type in loads/stores as the Efective Element Width, so the name isn't quite correct in that case.
afonso360
added
the
cranelift:area:riscv64
| let addr = writable_spilltmp_reg(); | ||
| LoadConstant::U64(offset as u64) | ||
| .load_constant_and_add(addr, from_reg) | ||
| .into_iter() | ||
| .for_each(|inst| inst.emit(&[], sink, emit_info, state)); |
There was a problem hiding this comment.
This ends up generating a lot of instructions, and is far from ideal. But It's also what we do in regular loads/stores, so I figured it would be best to tackle those together in a future PR.
github-actions
bot
added
cranelift
There was a problem hiding this comment.
Out of curiosity, @afonso360 do you have a planned reviewer or set-of-reviewers for the SIMD support for RISC-V? If not I'm giving my stamp of approval here as this is well designed, comprehensively tested, and follows the design of #6118 as well (at least all in my own opinion). I'm happy of course though to defer to the more seasoned Cranelift developers as well too.
Not really, I didn't have anyone in mind. So if anyone would like to volunteer it would be appreciated! Otherwise I'll merge this later today / tomorrow. |
There was a problem hiding this comment.
Oh, thanks for reminding me this is ready for review, Alex!
I have no concerns about the (quite minimal!) changes to the target-agnostic parts of Cranelift, and I think the implementation of the riscv-specific parts looks great too. I do have one question about the latter, but Afonso, I'll take your word for it either way. So feel free to merge if you're happy with it!
| .emit(&[], sink, emit_info, state); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I'm not too familiar with how islands work, so: Does the emission of this instruction need to happen after let mut start_off = sink.cur_offset(); below?
There was a problem hiding this comment.
I think calling emit before that check is correct. Since we recurse into Inst::emit for the vsetivli, we pass through that check again before emitting the instruction, and then once that is emitted we check it again before pushing the original instruction.
|
🎉 |
* riscv64: Remove unused code * riscv64: Add vector types * riscv64: Initial Vector ABI Load/Stores * riscv64: Vector Loads/Stores * riscv64: Fix `vsetvli` encoding error * riscv64: Add SIMD `iadd` runtests * riscv64: Rename `VecSew` The SEW name is correct, but only for VType. We also use this type in loads/stores as the Efective Element Width, so the name isn't quite correct in that case. * ci: Add V extension to RISC-V QEMU * riscv64: Misc Cleanups * riscv64: Check V extension in `load`/`store` for SIMD * riscv64: Fix `sumop` doc comment * cranelift: Fix comment typo * riscv64: Add convert for VType and VecElementWidth * riscv64: Remove VecElementWidth converter
👋 Hey,
This is the initial PR for implementing SIMD types with RISC-V Vectors, as described in #6118.
This PR implements only
iadd,load,storefor SIMD types, the minimum required to get aniaddruntest working.We currently only support 128bit SIMD types, but It's really easy to change that to 64bit or 32bit or any other width SIMD types as long as they fit in a single register, so I'm also planning on doing that in a future PR.
I didn't include any calling conventions changes, so we have a non standard calling convention for now. The RISC-V Calling Convention passes everything via stack and does not use vector registers to pass arguments. We've inherited the Floating Point part of the calling convention, so we are passing arguments in vector registers. I didn't want to make the initial PR too large, so I decided to make that part of a future PR.
QEMU's register size is set to 256 bits to avoid accidentally depending on 128b vector registers in the future.
None of the vector instructions are recognized by capstone so they show up as
.bytedirectives. I've been manually checking these againstllvm-objdump.