This patch implements vector crypto C intrinsics regarding to https://github.com/riscv-non-isa/rvv-intrinsic-doc/pull/234.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Thanks for sharing these patches. I appreciate you're probably not expecting in-depth reviews at this point, but if you could update the patch description to link to the current spec for these intrinsics it would be a lot easier for anyone who is able to give some early feedback.
I just wanted to check on whether this is ready to review? Also, to what degree are these intrinsics standardised and where is the relevant specification for them? Thanks!
Sorry for late reply, this is the relevant spec for C intrinsics: https://github.com/riscv-non-isa/rvv-intrinsic-doc/pull/234
I will update the patch according to this! Thanks!
clang/include/clang/Basic/riscv_vector.td | ||
---|---|---|
2835 | What does P in this name mean? Is this because they use OP_P as their opcode? If so I don't think that should be part of how we name intrinsics. | |
2836 | Zvkb doesn't exist anymore | |
2880 | I think I'd prefer to see the Zvbb instructions use their own class and not try to make RVVOutBuiltinSetP handle them and instructions that set HasVS. | |
2906 | I think the Ue needs a K since its required to be a constant? Also need to update SemaChecking to verify the valid range. I'm not sure whether we should use Ue or 'z' for constants. | |
2907 | vaeskf2 needs K to mark the scalar as a constant. Should maybe use z instead of Ue. Need to update SemaChecking.cpp | |
2917 | I think the Ue needs a K since its required to be a constant? Also need to update SemaChecking to verify the valid range. I'm not sure whether we should use Ue or 'z' for constants. | |
2921 | vsm3c needs K to mark the scalar as a constant. Should maybe use z instead of Ue. Need to update SemaChecking.cpp | |
clang/test/CodeGen/RISCV/rvv-intrinsics-autogenerated/non-policy/non-overloaded/vaeskf1.c | ||
13 | uimm is not used | |
clang/test/CodeGen/RISCV/rvv-intrinsics-autogenerated/non-policy/non-overloaded/vsm3c.c | ||
13 | uimm isn't used | |
clang/test/CodeGen/RISCV/rvv-intrinsics-autogenerated/policy/overloaded/vror.c | ||
13 | The intrinsic spec does not define rotates for signed types | |
clang/test/CodeGen/RISCV/rvv-intrinsics-autogenerated/policy/overloaded/vsm4k.c | ||
13 | uimm is unused | |
14 | Need to update SemaChecking.cpp to check the valid range for the immediate. | |
clang/test/CodeGen/RISCV/rvv-intrinsics-autogenerated/policy/overloaded/vwsll.c | ||
13 | Shift amounts should always be unsigned | |
22 | Scalar shift amount should be size_t to match other shifts. This was raised on the intrinsic PR |
clang/include/clang/Basic/riscv_vector.td | ||
---|---|---|
2838 | Why do we check HasVS when assigning suffix vv? I would have expected we use HasVV. In addition, why do we need to check !eq(NAME, "vsm4r") instead of setting the HasVV for that instruction? | |
2857 | Why not set HasVS=1 and HasVV=0 for vaesz instead of checking !if(!eq(NAME, "vgmul"),...? Also, do you mean to be discussing vaesz in the comment but use vgmul below? |
clang/include/clang/Basic/riscv_vector.td | ||
---|---|---|
2835 | Sure, I will rename by appending Zvk suffix for clarifying it's meaning~ | |
2838 | We check HasVS when assigning suffix vv because when the instruction has both VV and VS version, it uses double v as suffix, but if it just has VS version, it will use single v as suffix, that's why we check if it also has VS version in HasVV statement. | |
2857 | The reason we set these condition to check vaesz and vgmul is because they are exceptions. | |
2880 | It's a good idea, it will be much clearer and reduce the code size, thanks! | |
2906 | I'm not sure what's the difference between e and z, but seems both of them works fine. |
clang/lib/Sema/SemaChecking.cpp | ||
---|---|---|
4705 | Valid range of vaeskf1, vaeskf2 seems to be 0 to 15. [0] [0] https://github.com/riscv/riscv-crypto/blob/master/doc/vector/insns/vaeskf1.adoc |
clang/lib/Sema/SemaChecking.cpp | ||
---|---|---|
4705 | I think the field in the instruction is 5 bits, but vaeskf1 and vaeskf2 ignore bit 4. The true valid range is 1-10. The other values are aliased to one of the valid values. Should the intrinsic interface expose all 32 possible values or just 1-10? | |
clang/test/CodeGen/RISCV/rvv-intrinsics-autogenerated/non-policy/non-overloaded/vwsll.c | ||
13 | It doesn't make sense for op2 to be signed. It's a shift amount, its always a positive number | |
23 | scalar shift amounts should be size_t |
clang/include/clang/Basic/riscv_vector.td | ||
---|---|---|
2862 | Can we split this into two classes and get rid of IsVV? |
clang/test/CodeGen/RISCV/rvv-intrinsics-autogenerated/non-policy/non-overloaded/vwsll.c | ||
---|---|---|
13 | The spec doesn't define signed versions of these instrinsics from what I can see. |
clang/lib/Sema/SemaChecking.cpp | ||
---|---|---|
4705 | 1-10 is the valid range for vaeskf1. vaeskf2 is 2-14. |
clang/lib/Sema/SemaChecking.cpp | ||
---|---|---|
4705 | Let's leave it 0-31 for now and maybe start a conversation on crypto or intrinsic github. |
After discusstion in https://github.com/riscv-non-isa/rvv-intrinsic-doc/pull/234,
we will have multiple lmuls for scalar type operands.
Currently the test cases for those instructions are only presented in vaesdf,
all other instructions test cases will be copied from rvv-intrinsic-doc once
they're ready.
Also added test case for sema checking for checking valid lmul.
clang/lib/Sema/SemaChecking.cpp | ||
---|---|---|
4517 | InValid -> Invalid | |
4520 | Can this be a plain array or a std::array instead of SmallVector since the size is fixed. | |
4719 | Are there tu versions of these builtins? | |
clang/test/Sema/zvk-invalid.c | ||
20 | Test a _vs intrinsic since the vs operand has a different type than the result? |
This update does a few things:
- Update Craig's comments.
- Make LMUL=8 valid(https://github.com/riscv-non-isa/rvv-intrinsic-doc/pull/234#discussion_r1282568330).
- Rebase.
clang/include/clang/Basic/riscv_vector.td | ||
---|---|---|
2862 | Sure, good idea~ | |
clang/lib/Sema/SemaChecking.cpp | ||
4719 | Yes, there are, thanks for reminding! | |
clang/test/CodeGen/RISCV/rvv-intrinsics-autogenerated/non-policy/non-overloaded/vwsll.c | ||
13 | You are right, maybe it should only be signed version! | |
clang/test/Sema/zvk-invalid.c | ||
20 | Sure~ |
This update does a few things:
- Update test cases based on rvv-intrinsic-doc.
- Add checks for "zvknh[a|b]" instructions.
- Rename and restructure the function from CheckInvalidEGW to CheckInvalidVLENandLMUL.
The test you added clang/test/Sema/zvk-invalid.c is failing on bots, for example https://lab.llvm.org/buildbot/#/builders/139/builds/47055.
Is the test missing a REQUIRES: riscv-registered-target line by any chance?
What does P in this name mean? Is this because they use OP_P as their opcode? If so I don't think that should be part of how we name intrinsics.