[PATCH] D138809: [RISCV] Support vector crypto extension LLVM IR

[Michael Maitland via Phabricator via llvm-commits]

michaelmaitland added inline comments.


================
Comment at: llvm/include/llvm/IR/IntrinsicsRISCV.td:316
   }
+  // For destination vector type is the same as source vector.
+  // Input: (passthru, vector_in, vl, policy)
----------------
nit: `destination vector type is the same as the source vector type`


================
Comment at: llvm/include/llvm/IR/IntrinsicsRISCV.td:318
+  // Input: (passthru, vector_in, vl, policy)
+  class RISCVUnaryAAUnMaskedP<bit IsVS>
+        : Intrinsic<[llvm_anyvector_ty],
----------------
>From @craig.topper's comment https://reviews.llvm.org/D138810#inline-1494421

> 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.

Can we drop `P` here too and reuse `RISCVUnaryAAUnMasked`? After adding `DefaultAttrsIntrinsic` it may be the same? If not, I have two questions:

1. Is `P` the most descriptive suffix to use?
2. If so, can we at least document where `P` comes from?


================
Comment at: llvm/include/llvm/IR/IntrinsicsRISCV.td:429
   }
+  class RISCVBinaryAAXUnMaskedP
+        : DefaultAttrsIntrinsic<[llvm_anyvector_ty],
----------------
Can you please add a docstring for this class?


================
Comment at: llvm/include/llvm/IR/IntrinsicsRISCV.td:1715
+  def int_riscv_vghsh    : RISCVBinaryAAXUnMaskedP;
+  def int_riscv_vgmul_vv : RISCVUnaryAAUnMaskedP</*IsVS*/1>;
+
----------------
`IsVS` should be renamed to `HasVS` to match class definition. Do you need to pass `HasVS` since it defaults to 1? 


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td:1061
 
+class PPseudoUnaryNoMask<DAGOperand RetClass, VReg OpClass, string Constraint = ""> :
+      Pseudo<(outs RetClass:$rd),
----------------
Is there a reason behind using `PPseudo` instead of `VPseudo`? If its the same `P` as the other parts of this patch, should we keep it a suffix instead of prefix?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td:2716
+multiclass PPseudoVALU_V_NoMask<string Constraint = ""> {
+  foreach m = MxListVF4 in {
+    defvar mx = m.MX;
----------------
Docstring for MxListVF4 says for `zext/sext.vf4`. Are you able to update this docstring now that it has more uses?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td:4761
 
+multiclass PPatUnaryV_V_NoMask<string intrinsic, string instruction,
+			       list<VTypeInfo> vtilist> {
----------------
I raise the same question about whether this should be `VPatUnary` as above and if `P` is needed, should it be suffixed?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td:6680
 
+let Predicates = [HasStdExtZvbb] in {
+  defm PseudoVANDN  : VPseudoVALU_VV_VX;
----------------
It looks like this is part of the Vector Compress Instruction section. Should we add a vector crypto comment to separate?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td:7340
 
+let Predicates = [HasStdExtZvbb] in {
+  defm : VPatBinaryV_VV_VX<"int_riscv_vandn", "PseudoVANDN", AllIntegerVectors>;
----------------
It looks like this is part of the Vector Compress Instruction section. Should we add a vector crypto comment to separate?




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138809