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

[Brandon Wu via Phabricator via cfe-commits]

4vtomat marked 2 inline comments as done.
4vtomat 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> {
----------------
craig.topper wrote:
> 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.
Sure, I will rename by appending Zvk suffix for clarifying it's meaning~


================
Comment at: clang/include/clang/Basic/riscv_vector.td:2381
+  if HasVV then {
+    defvar suffix = !if(!or(HasVS, !eq(NAME, "vsm4r")), "vv", "v");
+    // We don't need suffix in Zvkb extension since it's consider as normal
----------------
michaelmaitland wrote:
> 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?
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.
As per spec, the instruction name is vsm4r.v(single v), so it's an exception for `HasVV` case.


================
Comment at: clang/include/clang/Basic/riscv_vector.td:2400
+    // mnemonics into its intrinsic function name.
+    defvar suffix = !if(!eq(NAME, "vgmul"), "vv", "vs");
+    defvar name = NAME # !if(!or(IsZvkb, !or(!eq(NAME, "vaesz"),
----------------
michaelmaitland wrote:
> 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?
The reason we set these condition to check `vaesz` and `vgmul` is because they are exceptions.
They don't need suffix in their overloaded name since they only have one version.
`HasVS=1` and `HasVV=0` is not enough to determine this situation.


================
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>;
----------------
craig.topper wrote:
> 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.
It's a good idea, it will be much clearer and reduce the code size, thanks!


================
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>;
----------------
craig.topper wrote:
> 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.
I'm not sure what's the difference between `e` and `z`, but seems both of them works fine.
However K is needed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138810/new/

https://reviews.llvm.org/D138810