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

[Brandon Wu via Phabricator via llvm-commits]

4vtomat marked 3 inline comments as done.
4vtomat added inline comments.


================
Comment at: llvm/include/llvm/IR/IntrinsicsRISCV.td:318
+  // Input: (passthru, vector_in, vl, policy)
+  class RISCVUnaryAAUnMaskedP<bit IsVS>
+        : Intrinsic<[llvm_anyvector_ty],
----------------
michaelmaitland wrote:
> 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?
Yeah, I think you and Craig are right, I should change it to more descriptive name, thanks!


================
Comment at: llvm/include/llvm/IR/IntrinsicsRISCV.td:429
   }
+  class RISCVBinaryAAXUnMaskedP
+        : DefaultAttrsIntrinsic<[llvm_anyvector_ty],
----------------
michaelmaitland wrote:
> Can you please add a docstring for this class?
Sure, I just added a comment above


================
Comment at: llvm/include/llvm/IR/IntrinsicsRISCV.td:1697
+let TargetPrefix = "riscv" in {
+  // zvbb
+  defm vandn             : RISCVBinaryAAX;
----------------
craig.topper wrote:
> Capitalize the Z in extension names in comments.
Sure~


================
Comment at: llvm/include/llvm/IR/IntrinsicsRISCV.td:1715
+  def int_riscv_vghsh    : RISCVBinaryAAXUnMaskedP;
+  def int_riscv_vgmul_vv : RISCVUnaryAAUnMaskedP</*IsVS*/1>;
+
----------------
michaelmaitland wrote:
> `IsVS` should be renamed to `HasVS` to match class definition. Do you need to pass `HasVS` since it defaults to 1? 
For vgmul,
it uses the **def** which chooses the **class** instead of **multiclass** definition:

```
class RISCVUnaryAAUnMaskedP<bit IsVS>
```

And for the **class** definition, argument **IsVS** means: If this intrinsic **is a VS** intrinsic, then it will use `llvm_anyvector_ty` as it's 2nd input arg type and use `LLVMMatchType<2>` as 4th input arg type.

However for **multiclass** definition, argument **HasVS** means, If this intrinsic **has VS**, then define one for it.



================
Comment at: llvm/include/llvm/IR/IntrinsicsRISCV.td:1715
+  def int_riscv_vghsh    : RISCVBinaryAAXUnMaskedP;
+  def int_riscv_vgmul_vv : RISCVUnaryAAUnMaskedP</*IsVS*/1>;
+
----------------
4vtomat wrote:
> michaelmaitland wrote:
> > `IsVS` should be renamed to `HasVS` to match class definition. Do you need to pass `HasVS` since it defaults to 1? 
> For vgmul,
> it uses the **def** which chooses the **class** instead of **multiclass** definition:
> 
> ```
> class RISCVUnaryAAUnMaskedP<bit IsVS>
> ```
> 
> And for the **class** definition, argument **IsVS** means: If this intrinsic **is a VS** intrinsic, then it will use `llvm_anyvector_ty` as it's 2nd input arg type and use `LLVMMatchType<2>` as 4th input arg type.
> 
> However for **multiclass** definition, argument **HasVS** means, If this intrinsic **has VS**, then define one for it.
> 
> `IsVS` should be renamed to `HasVS` to match class definition. Do you need to pass `HasVS` since it defaults to 1? 




================
Comment at: llvm/include/llvm/IR/IntrinsicsRISCV.td:1722
+  defm vaesem            : RISCVUnaryAAUnMaskedP;
+  def int_riscv_vaeskf1  : RISCVBinaryAAXUnMasked;
+  def int_riscv_vaeskf2  : RISCVBinaryAAXUnMaskedP;
----------------
craig.topper wrote:
> I think vaeskf1 needs something like `ImmArg<ArgIndex<X>>` for the immediate. 
> 
> Same for any other instruction with constant arguments.
Oh, I forgot this part, thanks for reminding!


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138809