[PATCH] D138809: [RISCV] Support vector crypto extension LLVM IR
Michael Maitland via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sun Jul 9 13:55:41 PDT 2023
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
More information about the llvm-commits
mailing list