This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Support vector crypto extension LLVM IR
ClosedPublic

Authored by 4vtomat on Nov 28 2022, 6:30 AM.

Diff Detail

Event Timeline

4vtomat created this revision.Nov 28 2022, 6:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 28 2022, 6:30 AM
4vtomat requested review of this revision.Nov 28 2022, 6:30 AM
4vtomat edited the summary of this revision. (Show Details)Nov 28 2022, 6:40 AM
4vtomat updated this revision to Diff 479143.Nov 30 2022, 7:43 PM

Fixed the comment from Craig Topper and Philip Reames

craig.topper added inline comments.Jan 19 2023, 9:51 AM
llvm/include/llvm/IR/IntrinsicsRISCV.td
303

DefaultAttrsIntrinsic

445

DefaultAttrsIntrinsic

craig.topper added inline comments.Jan 19 2023, 9:54 AM
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
2672

This parameter is never used.

2680

I think this can use VPseudoBinaryV_VV

2682

I think this can use VPseudoBinaryV_VX

3693

I don't think using "PPat" instead of "VPat" meaningful highlights the difference between these patterns. The instructions being OP_P is an encoding detail only.

craig.topper added inline comments.Jan 19 2023, 10:01 AM
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
3871

I think this might be the same as VPatTernaryNoMaskWithPolicy

4vtomat updated this revision to Diff 493830.Jan 31 2023, 10:54 PM

Some NFCs.

4vtomat updated this revision to Diff 493833.Jan 31 2023, 11:13 PM

NFC, add accidentally deleted blank line.

4vtomat updated this revision to Diff 508481.Mar 26 2023, 10:36 PM

Updated to spec version 20230206.

4vtomat updated this revision to Diff 508537.Mar 27 2023, 2:06 AM

Some updates on last commit.

4vtomat updated this revision to Diff 517112.Apr 26 2023, 3:22 AM

Update to version 0.5.1.

4vtomat updated this revision to Diff 535695.Jun 29 2023, 2:47 AM

Ready for review

4vtomat edited the summary of this revision. (Show Details)Jun 29 2023, 2:52 AM
4vtomat added reviewers: ego, kito-cheng, craig.topper, asb.
evandro removed a subscriber: evandro.Jun 30 2023, 3:43 PM
craig.topper added inline comments.Jul 5 2023, 4:13 PM
llvm/include/llvm/IR/IntrinsicsRISCV.td
1876

Capitalize the Z in extension names in comments.

1901

I think vaeskf1 needs something like ImmArg<ArgIndex<X>> for the immediate.

Same for any other instruction with constant arguments.

michaelmaitland added inline comments.
llvm/include/llvm/IR/IntrinsicsRISCV.td
299

nit: destination vector type is the same as the source vector type

301

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?
444

Can you please add a docstring for this class?

1894

IsVS should be renamed to HasVS to match class definition. Do you need to pass HasVS since it defaults to 1?

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
984

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?

2571

Docstring for MxListVF4 says for zext/sext.vf4. Are you able to update this docstring now that it has more uses?

4411

I raise the same question about whether this should be VPatUnary as above and if P is needed, should it be suffixed?

6493

It looks like this is part of the Vector Compress Instruction section. Should we add a vector crypto comment to separate?

7105

It looks like this is part of the Vector Compress Instruction section. Should we add a vector crypto comment to separate?

4vtomat updated this revision to Diff 538589.Jul 10 2023, 5:02 AM
4vtomat marked 15 inline comments as done.
4vtomat edited the summary of this revision. (Show Details)

Updates Craig's and Michael's comments.

4vtomat marked 3 inline comments as done.Jul 10 2023, 5:03 AM
4vtomat added inline comments.
llvm/include/llvm/IR/IntrinsicsRISCV.td
301

Yeah, I think you and Craig are right, I should change it to more descriptive name, thanks!

444

Sure, I just added a comment above

1876

Sure~

1894

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.

1894

IsVS should be renamed to HasVS to match class definition. Do you need to pass HasVS since it defaults to 1?

1901

Oh, I forgot this part, thanks for reminding!

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
2571

You are right, thanks!

6493

Sure, thanks!

This revision is now accepted and ready to land.Jul 17 2023, 3:20 PM
craig.topper added inline comments.Jul 17 2023, 3:26 PM
llvm/test/CodeGen/RISCV/rvv/vwsll.ll
1459

shift amounts should be iXLen like the non widening shift

craig.topper added inline comments.Jul 17 2023, 3:28 PM
llvm/test/CodeGen/RISCV/rvv/vror.ll
2186

Shift amounts should be iXLen.

craig.topper added inline comments.Jul 17 2023, 3:33 PM
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
7116

We will need to implement a VI version of the vrol intrinsic using the vror.vi instruction by changing the amount to sew-rotamount.

7117

VPatBinaryV_VV_VX_VI defaults simm5, but vror.vi uses uimm6.

craig.topper requested changes to this revision.Jul 17 2023, 3:34 PM
This revision now requires changes to proceed.Jul 17 2023, 3:34 PM
4vtomat updated this revision to Diff 544294.Jul 26 2023, 4:08 AM
4vtomat marked 4 inline comments as done.

This update includes:

  1. Add the constraint: LMUL*VLEN < EGW during lowering instructions.
  2. Enumerate over all of lmul less or equal to vd type's lmul for those instructions that have scalar operand.
craig.topper added inline comments.Jul 26 2023, 9:15 AM
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
1551 ↗(On Diff #544294)

This seems unnecessarily complex. https://reviews.llvm.org/D155439 already did this for non-intrinsics using an SDNodeXForm in tablegen

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
7133

This is dead code in release builds and will give a build warning.

7273

This assert isn't helpful to compiler users. It's not part of release builds.

4vtomat updated this revision to Diff 544621.Jul 26 2023, 11:39 PM
4vtomat marked 2 inline comments as done.

Resolved Craig's comments.

craig.topper added inline comments.Jul 27 2023, 8:56 AM
llvm/lib/Target/RISCV/RISCVInstrInfoZvk.td
690

timm5 uses isInt<5>, but I think you want isUInt<5>?

4vtomat updated this revision to Diff 545025.Jul 27 2023, 11:15 PM
4vtomat marked an inline comment as done.

Resolved Craig's comment.

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
1551 ↗(On Diff #544294)

Oh, that's much better, thanks!

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
7273

Oh, ok. I just changed it to report_fatal_error~

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
7117

Oh, I forgot this instruction has different encoding of immediate, thanks!

llvm/lib/Target/RISCV/RISCVInstrInfoZvk.td
690

Oh, correct, thanks!

craig.topper added inline comments.Jul 31 2023, 12:29 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
7135

Doesn't this need to consider the value of vscale or VLEN? getKnownMinValue() always returns 64 for <vscale x 2 x i32>.

llvm/lib/Target/RISCV/RISCVInstrInfoZvk.td
758

NegImm64 no longer exists. I changed it in https://reviews.llvm.org/D156348

4vtomat added inline comments.Jul 31 2023, 10:34 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
7135

My thought was: For each <vscale x N x M>, if N * M >= EGS * M == [4|8] * M, namely if N >= EGS, then it's valid.
So, for vaesdf.vv vd, vs1, vs2, <vscale x 2 x i32> is invalid, however, <vscale x 4 x i32> and larger is valid.

4vtomat updated this revision to Diff 545917.Jul 31 2023, 10:43 PM

Resolved Craig's comment.

craig.topper added inline comments.Jul 31 2023, 11:36 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
7135

<vscale x 4 x i32> is LMUL=2. I thought based on our conversation here https://github.com/riscv-non-isa/rvv-intrinsic-doc/pull/234#discussion_r1271843685 that we wanted to allow LMUL=1 with Zvl128b?

4vtomat updated this revision to Diff 546000.Aug 1 2023, 4:10 AM

Resolved Craig's comment.

4vtomat marked 2 inline comments as done.Aug 1 2023, 4:10 AM
4vtomat added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
7135

Oh, I misunderstood how LMUL is calculated, I thought it was VLEN / (N x M), so if zvl128b, <vscale x 4 x i32> would have LMUL=1.
But seems it's not calculated this way, the VLEN here is always 64. So if we want to support LMUL=1, <vscale x 2 x i32> should be valid.

craig.topper added inline comments.Aug 1 2023, 5:52 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
7137

Use RVVBitsPerBlock instead of 64

craig.topper added inline comments.Aug 1 2023, 5:56 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
7137

Please add extra parentheses to Subtarget.getRealMinVLen() * VT.getSizeInBits().getKnownMinValue() / 64 to make the operator precedence clear

4vtomat marked an inline comment as done.Aug 1 2023, 6:07 PM
4vtomat added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
7137

The reason I didn't add parentheses between VT.getSizeInBits().getKnownMinValue() / 64 is because if I do so, I need to case VT.getSizeInBits().getKnownMinValue() to either double or float since it could be fractional. Do you think it's better to do it this way?

craig.topper added inline comments.Aug 1 2023, 6:11 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
7137

Use (Subtarget.getRealMinVLen() * VT.getSizeInBits().getKnownMinValue()) / 64. It's the operator precedence the compiler will use for the current code. I just don't want to think about the operator precedence when reading the code.

4vtomat added inline comments.Aug 1 2023, 6:12 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
7137

Oh, I see~

4vtomat updated this revision to Diff 546285.Aug 1 2023, 6:14 PM

Update Craig's comment.

This revision is now accepted and ready to land.Aug 2 2023, 10:19 AM
This revision was landed with ongoing or failed builds.Aug 2 2023, 10:25 AM
This revision was automatically updated to reflect the committed changes.
zixuan-wu added inline comments.
llvm/lib/Target/RISCV/RISCVInstrInfo.td
240

Should it be [{return isUInt<6>(Imm);}]? @4vtomat

4vtomat added inline comments.Aug 3 2023, 1:51 AM
llvm/lib/Target/RISCV/RISCVInstrInfo.td
240

Yeah, you are right, let me have a patch to fix it, thanks!