[PATCH] D138810: [RISCV] Support vector crypto extension C intrinsics

[Craig Topper via Phabricator via cfe-commits]

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