Wasmtime GitHub notifications bot (May 02 2023 at 13:54): Wasmtime GitHub notifications bot (May 02 2023 at 13:54):afonso360 opened PR #6325 from afonso360:riscv-vec-imm to bytecodealliance:main:
:wave: Hey,
This PR adds a new vector instruction format
Vector+Imm. The encoding is exactly the same asv*.vvbut the second register is instead a 5 bit field. This immediate is signed for most ops, but unsigned for some.
Wasmtime GitHub notifications bot (May 02 2023 at 13:54):afonso360 requested elliottt for a review on PR #6325.
Wasmtime GitHub notifications bot (May 02 2023 at 13:54):afonso360 requested wasmtime-compiler-reviewers for a review on PR #6325.
Wasmtime GitHub notifications bot (May 03 2023 at 18:17):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (May 03 2023 at 18:17):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (May 03 2023 at 18:17):alexcrichton created PR review comment:
Should this do
i8::try_from(arg0 as i64).ok()??Otherwise I think
i64::MINwould accidentally get truncated to 0?
Wasmtime GitHub notifications bot (May 03 2023 at 18:17):alexcrichton created PR review comment:
The mask here shouldn't be required, right? Or otherwise this would be ok to perhaps
debug_assert!that the masked-out bits are all not set?(similar for
funct6above too maybe?)
Wasmtime GitHub notifications bot (May 03 2023 at 18:17):alexcrichton created PR review comment:
Oh I see now my thoughts are very much related to https://github.com/bytecodealliance/wasmtime/pull/6324#discussion_r1182744349
Wasmtime GitHub notifications bot (May 03 2023 at 18:17):alexcrichton created PR review comment:
I don't know much about RISC-V, but should this
AluOpbe different thanVecAluOpRRRor the same? The single data point ofVaddhas the encodings the same, but I'm not sure if the pattern holds up elsewhere.
Wasmtime GitHub notifications bot (May 03 2023 at 18:17):alexcrichton created PR review comment:
My curiosity has been piqued if you're willing to oblige -- according to this doc I see a bunch of V/X/I columns but I'm not sure how that translates to the encoding. Is there a different document that explains how these columns relate to the encoding?
Wasmtime GitHub notifications bot (May 03 2023 at 19:03):afonso360 created PR review comment:
That table does not seem to be explained anywhere, which is weird the closest thing is 10. Vector Arithmetic Instruction Formats which only really explains the format, but not the encoding in that table. The way I understand it is the following.
There are 3 columns, which correspond to the 3 columns for the minor opcodes at the top (i.e. OPIVV, OPIVX, OPMVX, etc..). The minor opcode corresponds to the funct3 field. (The exact encoding is specified here)
The minor opcodes correspond to the operands that instruction takes.
OPIVVis for Vector/Vector instructions (i.e.vadd.vv),OPIVXis for Vector/X Register instructions (i.e.vadd.vx) andOPIVIis for Vector/Immediate instruction (i.e.vadd.vi) which is what is implemented in this PR.I'm only planning on having 2 instruction formats, since
OP*VV,OP*VXandOPFVFcan share the same instruction format in cranelift. Just with an assert that we are using the correct regclass.OPIVIis the only special one here.The big table displays the funct6 field. Take for example the
vrsubinstruction. It is in the leftmost column, and only hasXandIformats. So we know that it only takes the minor opcode formats ofOPIVXandOPIVI.vrsubitself has a funct6 of0b000011with both of those minor opcodes.There are additional encoding spaces, at the bottom, but my understanding is that these are all OPIVI instructions and that encoding gets put in the immediate field (not 100% sure on this yet).
Wasmtime GitHub notifications bot (May 03 2023 at 19:03):afonso360 edited PR review comment.
Wasmtime GitHub notifications bot (May 03 2023 at 19:06):afonso360 created PR review comment:
Yeah, I probably don't need the masking everywhere. I think I might take the approach that @jameysharp is suggesting and switch to
debug_assertand try to figure out how to get better type safety in the future.
Wasmtime GitHub notifications bot (May 03 2023 at 19:06):afonso360 edited PR review comment.
Wasmtime GitHub notifications bot (May 03 2023 at 19:09):afonso360 created PR review comment:
Well, It is the same, but not all Opcodes work on both formats.
vaddworks for both, butvsubdoes not have avsub.viformat. It would be nice if we could share the funct6 mapping somehow though since it is exactly the same on both.
Wasmtime GitHub notifications bot (May 03 2023 at 19:09):afonso360 edited PR review comment.
Wasmtime GitHub notifications bot (May 03 2023 at 19:10):afonso360 edited PR review comment.
Wasmtime GitHub notifications bot (May 03 2023 at 20:36):afonso360 edited PR review comment.
Wasmtime GitHub notifications bot (May 03 2023 at 20:36):afonso360 edited PR review comment.
Wasmtime GitHub notifications bot (May 03 2023 at 20:39):afonso360 edited PR review comment.
Wasmtime GitHub notifications bot (May 03 2023 at 21:10):afonso360 created PR review comment:
I've now put these into a
VecOpCategorystruct so that their encoding is somewhat centralized.
Wasmtime GitHub notifications bot (May 03 2023 at 21:10):afonso360 updated PR #6325.
Wasmtime GitHub notifications bot (May 03 2023 at 21:11):afonso360 updated PR #6325.
Wasmtime GitHub notifications bot (May 03 2023 at 21:18):afonso360 edited PR review comment.
Wasmtime GitHub notifications bot (May 03 2023 at 21:26):afonso360 updated PR #6325.
Wasmtime GitHub notifications bot (May 04 2023 at 20:09):alexcrichton created PR review comment:
Ah ok makes sense, in that case sounds good to me to have a minor amount of duplication :+1:
Wasmtime GitHub notifications bot (May 04 2023 at 20:13):alexcrichton submitted PR review:
Thanks for taking the time to explain! That all sounds good to me and everything looks good to me as well :+1:
Wasmtime GitHub notifications bot (May 04 2023 at 22:31):afonso360 merged PR #6325.
Last updated: Dec 23 2024 at 13:07 UTC