craig.topper added inline comments.
================
Comment at: clang/include/clang/Basic/riscv_vector.td:2378
+
+multiclass RVVOutBuiltinSetP<bit HasVV = 1, bit HasVS = 1, bit HasSigned = 0,
+ string type_range = "i", bit IsZvkb = 0> {
----------------
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.
================
Comment at: clang/include/clang/Basic/riscv_vector.td:2379
+multiclass RVVOutBuiltinSetP<bit HasVV = 1, bit HasVS = 1, bit HasSigned = 0,
+ string type_range = "i", bit IsZvkb = 0> {
+ if HasVV then {
----------------
Zvkb doesn't exist anymore
================
Comment at: clang/include/clang/Basic/riscv_vector.td:2423
+ defm vandn : RVVIntBinBuiltinSet;
+ defm vbrev : RVVOutBuiltinSetP<1, 0, 1, "csil", 1>;
+ defm vbrev8 : RVVOutBuiltinSetP<1, 0, 1, "csil", 1>;
----------------
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.
================
Comment at: clang/include/clang/Basic/riscv_vector.td:2449
+ let UnMaskedPolicyScheme = HasPassthruOperand in
+ defm vaeskf1 : RVVOutOp1BuiltinSet<"vaeskf1", "i", [["vi", "Uv", "UvUvUe"]]>;
+ defm vaeskf2 : RVVOutOp1BuiltinSetP<0>;
----------------
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.
================
Comment at: clang/include/clang/Basic/riscv_vector.td:2450
+ defm vaeskf1 : RVVOutOp1BuiltinSet<"vaeskf1", "i", [["vi", "Uv", "UvUvUe"]]>;
+ defm vaeskf2 : RVVOutOp1BuiltinSetP<0>;
+ defm vaesz : RVVOutBuiltinSetP<0>;
----------------
vaeskf2 needs `K` to mark the scalar as a constant. Should maybe use `z`
instead of `Ue`. Need to update SemaChecking.cpp
================
Comment at: clang/include/clang/Basic/riscv_vector.td:2460
+ let UnMaskedPolicyScheme = HasPassthruOperand in
+ defm vsm4k : RVVOutOp1BuiltinSet<"vsm4k", "i", [["vi", "Uv", "UvUvUe"]]>;
+ defm vsm4r : RVVOutBuiltinSetP;
----------------
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.
================
Comment at: clang/include/clang/Basic/riscv_vector.td:2464
+ // zvksh
+ defm vsm3c : RVVOutOp1BuiltinSetP<0>;
+ let UnMaskedPolicyScheme = HasPassthruOperand in
----------------
vsm3c needs `K` to mark the scalar as a constant. Should maybe use `z` instead
of `Ue`. Need to update SemaChecking.cpp
================
Comment at:
clang/test/CodeGen/RISCV/rvv-intrinsics-autogenerated/non-policy/non-overloaded/vaeskf1.c:12
+//
+vuint32mf2_t test_vaeskf1_vi_u32mf2(vuint32mf2_t vs2, size_t uimm, size_t vl) {
+ return __riscv_vaeskf1_vi_u32mf2(vs2, 0, vl);
----------------
`uimm` is not used
================
Comment at:
clang/test/CodeGen/RISCV/rvv-intrinsics-autogenerated/non-policy/non-overloaded/vsm3c.c:12
+//
+vuint32mf2_t test_vsm3c_vi_u32mf2(vuint32mf2_t vd, vuint32mf2_t vs2, size_t
uimm, size_t vl) {
+ return __riscv_vsm3c_vi_u32mf2(vd, vs2, 0, vl);
----------------
`uimm` isn't used
================
Comment at:
clang/test/CodeGen/RISCV/rvv-intrinsics-autogenerated/policy/overloaded/vror.c:12
+//
+vint8mf8_t test_vror_vv_i8mf8_tu(vint8mf8_t maskedoff, vint8mf8_t vs2,
vint8mf8_t vs1, size_t vl) {
+ return __riscv_vror_tu(maskedoff, vs2, vs1, vl);
----------------
The intrinsic spec does not define rotates for signed types
================
Comment at:
clang/test/CodeGen/RISCV/rvv-intrinsics-autogenerated/policy/overloaded/vsm4k.c:12
+//
+vuint32mf2_t test_vsm4k_vi_u32mf2_tu(vuint32mf2_t maskedoff, vuint32mf2_t vs2,
size_t uimm, size_t vl) {
+ return __riscv_vsm4k_tu(maskedoff, vs2, 0, vl);
----------------
`uimm` is unused
================
Comment at:
clang/test/CodeGen/RISCV/rvv-intrinsics-autogenerated/policy/overloaded/vsm4k.c:13
+vuint32mf2_t test_vsm4k_vi_u32mf2_tu(vuint32mf2_t maskedoff, vuint32mf2_t vs2,
size_t uimm, size_t vl) {
+ return __riscv_vsm4k_tu(maskedoff, vs2, 0, vl);
+}
----------------
Need to update SemaChecking.cpp to check the valid range for the immediate.
================
Comment at:
clang/test/CodeGen/RISCV/rvv-intrinsics-autogenerated/policy/overloaded/vwsll.c:12
+//
+vint16mf4_t test_vwsll_vv_i16mf4_tu(vint16mf4_t maskedoff, vint8mf8_t op1,
vint8mf8_t op2, size_t vl) {
+ return __riscv_vwsll_vv_tu(maskedoff, op1, op2, vl);
----------------
Shift amounts should always be unsigned
================
Comment at:
clang/test/CodeGen/RISCV/rvv-intrinsics-autogenerated/policy/overloaded/vwsll.c:21
+//
+vint16mf4_t test_vwsll_vx_i16mf4_tu(vint16mf4_t maskedoff, vint8mf8_t op1,
int8_t op2, size_t vl) {
+ return __riscv_vwsll_vx_tu(maskedoff, op1, op2, vl);
----------------
Scalar shift amount should be size_t to match other shifts. This was raised on
the intrinsic PR
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D138810/new/
https://reviews.llvm.org/D138810
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
