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
320

DefaultAttrsIntrinsic

430

DefaultAttrsIntrinsic

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

This parameter is never used.

2939

I think this can use VPseudoBinaryV_VV

2941

I think this can use VPseudoBinaryV_VX

4096

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
4315

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
1620

Capitalize the Z in extension names in comments.

1645

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
316

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

318

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

Can you please add a docstring for this class?

1638

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
1043

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?

2695

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

4701

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

6426

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

7053

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
318

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

429

Sure, I just added a comment above

1620

Sure~

1638

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.

1638

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

1645

Oh, I forgot this part, thanks for reminding!

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

You are right, thanks!

6426

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
1458 ↗(On Diff #538589)

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
7064

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

7065

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
7121 ↗(On Diff #544294)

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

7261 ↗(On Diff #544294)

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
678 ↗(On Diff #544621)

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
7261 ↗(On Diff #544294)

Oh, ok. I just changed it to report_fatal_error~

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

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

llvm/lib/Target/RISCV/RISCVInstrInfoZvk.td
678 ↗(On Diff #544621)

Oh, correct, thanks!

craig.topper added inline comments.Jul 31 2023, 12:29 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
7135 ↗(On Diff #545025)

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 ↗(On Diff #545025)

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 ↗(On Diff #545025)

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 ↗(On Diff #545025)

<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 ↗(On Diff #545025)

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
7187 ↗(On Diff #546000)

Use RVVBitsPerBlock instead of 64

craig.topper added inline comments.Aug 1 2023, 5:56 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
7187 ↗(On Diff #546000)

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
7187 ↗(On Diff #546000)

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
7187 ↗(On Diff #546000)

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
7187 ↗(On Diff #546000)

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 ↗(On Diff #546534)

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 ↗(On Diff #546534)

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