[RISCV] Support vector crypto extension LLVM IR

[4vtomat]

Summary

Depends on D141672

[craig.topper]

llvm/include/llvm/IR/IntrinsicsRISCV.td

1617

Capitalize the Z in extension names in comments.

1642

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

Same for any other instruction with constant arguments.

[michaelmaitland]

llvm/include/llvm/IR/IntrinsicsRISCV.td

332

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

334

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?

447

Can you please add a docstring for this class?

1635

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

1031

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?

2545

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

4044

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

5691

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

6310

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

[4vtomat]

Updates Craig's and Michael's comments.

[4vtomat]

llvm/include/llvm/IR/IntrinsicsRISCV.td

334

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

447

Sure, I just added a comment above

1617

Sure~

1635

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.

1635

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

1642

Oh, I forgot this part, thanks for reminding!

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td

2545

You are right, thanks!

5691

Sure, thanks!

[michaelmaitland]

LGTM.

[craig.topper]

llvm/test/CodeGen/RISCV/rvv/vwsll.ll

1458 ↗

(On Diff #538589)

shift amounts should be iXLen like the non widening shift

[craig.topper]

llvm/test/CodeGen/RISCV/rvv/vror.ll

2186

Shift amounts should be iXLen.

[craig.topper]

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td

6321

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

6322

VPatBinaryV_VV_VX_VI defaults simm5, but vror.vi uses uimm6.

[4vtomat]

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]

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]

Resolved Craig's comments.

[craig.topper]

llvm/lib/Target/RISCV/RISCVInstrInfoZvk.td

678 ↗

(On Diff #544621)

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

[4vtomat]

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

6322

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]

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]

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]

Resolved Craig's comment.

[craig.topper]

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]

Resolved Craig's comment.

[4vtomat]

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]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

7187 ↗

(On Diff #546000)

Use RVVBitsPerBlock instead of 64

[craig.topper]

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]

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]

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]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

7187 ↗

(On Diff #546000)

Oh, I see~

[4vtomat]

Update Craig's comment.

[craig.topper]

LGTM

[zixuan-wu]

llvm/lib/Target/RISCV/RISCVInstrInfo.td

240 ↗

(On Diff #546534)

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

[4vtomat]

llvm/lib/Target/RISCV/RISCVInstrInfo.td

240 ↗

(On Diff #546534)

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