This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Support vector crypto extension C intrinsics
ClosedPublic

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

Diff Detail

Event Timeline

4vtomat created this revision.Nov 28 2022, 6:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 28 2022, 6:32 AM
4vtomat requested review of this revision.Nov 28 2022, 6:32 AM
4vtomat edited the summary of this revision. (Show Details)Nov 28 2022, 6:41 AM
asb added a comment.Nov 30 2022, 2:45 AM

Thanks for sharing these patches. I appreciate you're probably not expecting in-depth reviews at this point, but if you could update the patch description to link to the current spec for these intrinsics it would be a lot easier for anyone who is able to give some early feedback.

4vtomat updated this revision to Diff 479144.Nov 30 2022, 7:45 PM

Fixed the comment from Craig Topper and Philip Reames

4vtomat updated this revision to Diff 508539.Mar 27 2023, 2:09 AM

Updated to spec version 20230206.

4vtomat updated this revision to Diff 517113.Apr 26 2023, 3:23 AM

Update to version 0.5.1.

asb added a comment.Apr 26 2023, 7:34 AM

I just wanted to check on whether this is ready to review? Also, to what degree are these intrinsics standardised and where is the relevant specification for them? Thanks!

Sorry for late reply, this is the relevant spec for C intrinsics: https://github.com/riscv-non-isa/rvv-intrinsic-doc/pull/234
I will update the patch according to this! Thanks!

evandro removed a subscriber: evandro.Jun 14 2023, 8:04 AM
4vtomat updated this revision to Diff 535696.Jun 29 2023, 2:47 AM

Ready for review

craig.topper added inline comments.Jul 5 2023, 3:59 PM
clang/include/clang/Basic/riscv_vector.td
2386

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.

2387

Zvkb doesn't exist anymore

2431

I think I'd prefer to see the Zvbb instructions use their own class and not try to make RVVOutBuiltinSetP handle them and instructions that set HasVS.

2457

I think the Ue needs a K since its required to be a constant? Also need to update SemaChecking to verify the valid range.

I'm not sure whether we should use Ue or 'z' for constants.

2458

vaeskf2 needs K to mark the scalar as a constant. Should maybe use z instead of Ue. Need to update SemaChecking.cpp

2468

I think the Ue needs a K since its required to be a constant? Also need to update SemaChecking to verify the valid range.

I'm not sure whether we should use Ue or 'z' for constants.

2472

vsm3c needs K to mark the scalar as a constant. Should maybe use z instead of Ue. Need to update SemaChecking.cpp

clang/test/CodeGen/RISCV/rvv-intrinsics-autogenerated/non-policy/non-overloaded/vaeskf1.c
13

uimm is not used

clang/test/CodeGen/RISCV/rvv-intrinsics-autogenerated/non-policy/non-overloaded/vsm3c.c
13

uimm isn't used

clang/test/CodeGen/RISCV/rvv-intrinsics-autogenerated/policy/overloaded/vror.c
13

The intrinsic spec does not define rotates for signed types

clang/test/CodeGen/RISCV/rvv-intrinsics-autogenerated/policy/overloaded/vsm4k.c
13

uimm is unused

14

Need to update SemaChecking.cpp to check the valid range for the immediate.

clang/test/CodeGen/RISCV/rvv-intrinsics-autogenerated/policy/overloaded/vwsll.c
13

Shift amounts should always be unsigned

22

Scalar shift amount should be size_t to match other shifts. This was raised on the intrinsic PR

Can you please add a description for this patch?

michaelmaitland added inline comments.Jul 9 2023, 2:05 PM
clang/include/clang/Basic/riscv_vector.td
2389

Why do we check HasVS when assigning suffix vv? I would have expected we use HasVV. In addition, why do we need to check !eq(NAME, "vsm4r") instead of setting the HasVV for that instruction?

2408

Why not set HasVS=1 and HasVV=0 for vaesz instead of checking !if(!eq(NAME, "vgmul"),...?

Also, do you mean to be discussing vaesz in the comment but use vgmul below?

4vtomat edited the summary of this revision. (Show Details)Jul 10 2023, 6:09 AM
4vtomat updated this revision to Diff 539402.Jul 11 2023, 11:33 PM
4vtomat marked 12 inline comments as done.

Resolved Craig's and Michael's comments.

4vtomat marked 2 inline comments as done.Jul 12 2023, 12:59 AM
4vtomat added inline comments.
clang/include/clang/Basic/riscv_vector.td
2386

Sure, I will rename by appending Zvk suffix for clarifying it's meaning~

2389

We check HasVS when assigning suffix vv because when the instruction has both VV and VS version, it uses double v as suffix, but if it just has VS version, it will use single v as suffix, that's why we check if it also has VS version in HasVV statement.
As per spec, the instruction name is vsm4r.v(single v), so it's an exception for HasVV case.

2408

The reason we set these condition to check vaesz and vgmul is because they are exceptions.
They don't need suffix in their overloaded name since they only have one version.
HasVS=1 and HasVV=0 is not enough to determine this situation.

2431

It's a good idea, it will be much clearer and reduce the code size, thanks!

2457

I'm not sure what's the difference between e and z, but seems both of them works fine.
However K is needed.

eopXD added inline comments.Jul 17 2023, 11:12 AM
clang/lib/Sema/SemaChecking.cpp
4576 ↗(On Diff #539402)

Valid range of vaeskf1, vaeskf2 seems to be 0 to 15. [0]
Valid range of vsm4k seems to be 0 to 7 [1].

[0] https://github.com/riscv/riscv-crypto/blob/master/doc/vector/insns/vaeskf1.adoc
[1] https://github.com/riscv/riscv-crypto/blob/master/doc/vector/insns/vsm4k.adoc

craig.topper added inline comments.Jul 17 2023, 6:11 PM
clang/lib/Sema/SemaChecking.cpp
4576 ↗(On Diff #539402)

I think the field in the instruction is 5 bits, but vaeskf1 and vaeskf2 ignore bit 4. The true valid range is 1-10. The other values are aliased to one of the valid values. Should the intrinsic interface expose all 32 possible values or just 1-10?

clang/test/CodeGen/RISCV/rvv-intrinsics-autogenerated/non-policy/non-overloaded/vwsll.c
13

It doesn't make sense for op2 to be signed. It's a shift amount, its always a positive number

23

scalar shift amounts should be size_t

craig.topper added inline comments.Jul 17 2023, 6:15 PM
clang/include/clang/Basic/riscv_vector.td
2413

Can we split this into two classes and get rid of IsVV?

craig.topper added inline comments.Jul 17 2023, 6:16 PM
clang/test/CodeGen/RISCV/rvv-intrinsics-autogenerated/non-policy/non-overloaded/vwsll.c
13

The spec doesn't define signed versions of these instrinsics from what I can see.

craig.topper added inline comments.Jul 17 2023, 6:18 PM
clang/lib/Sema/SemaChecking.cpp
4576 ↗(On Diff #539402)

1-10 is the valid range for vaeskf1. vaeskf2 is 2-14.

craig.topper added inline comments.Jul 18 2023, 5:15 PM
clang/lib/Sema/SemaChecking.cpp
4576 ↗(On Diff #539402)

Let's leave it 0-31 for now and maybe start a conversation on crypto or intrinsic github.

4vtomat updated this revision to Diff 544624.Jul 26 2023, 11:58 PM
4vtomat marked 5 inline comments as done.

After discusstion in https://github.com/riscv-non-isa/rvv-intrinsic-doc/pull/234,
we will have multiple lmuls for scalar type operands.
Currently the test cases for those instructions are only presented in vaesdf,
all other instructions test cases will be copied from rvv-intrinsic-doc once
they're ready.
Also added test case for sema checking for checking valid lmul.

craig.topper added inline comments.Aug 2 2023, 10:53 AM
clang/lib/Sema/SemaChecking.cpp
4496 ↗(On Diff #544624)

InValid -> Invalid

4499 ↗(On Diff #544624)

Can this be a plain array or a std::array instead of SmallVector since the size is fixed.

4693 ↗(On Diff #544624)

Are there tu versions of these builtins?

clang/test/Sema/zvk-invalid.c
19 ↗(On Diff #544624)

Test a _vs intrinsic since the vs operand has a different type than the result?

4vtomat updated this revision to Diff 546671.Aug 2 2023, 7:31 PM
4vtomat marked 3 inline comments as done.

This update does a few things:

  1. Update Craig's comments.
  2. Make LMUL=8 valid(https://github.com/riscv-non-isa/rvv-intrinsic-doc/pull/234#discussion_r1282568330).
  3. Rebase.
clang/include/clang/Basic/riscv_vector.td
2413

Sure, good idea~

clang/lib/Sema/SemaChecking.cpp
4693 ↗(On Diff #544624)

Yes, there are, thanks for reminding!

clang/test/CodeGen/RISCV/rvv-intrinsics-autogenerated/non-policy/non-overloaded/vwsll.c
13

You are right, maybe it should only be signed version!

clang/test/Sema/zvk-invalid.c
19 ↗(On Diff #544624)

Sure~

4vtomat updated this revision to Diff 546677.Aug 2 2023, 8:17 PM

Update missing comment.

4vtomat marked an inline comment as done.Aug 2 2023, 8:18 PM
4vtomat updated this revision to Diff 546746.Aug 3 2023, 1:46 AM

This update does a few things:

  1. Update test cases based on rvv-intrinsic-doc.
  2. Add checks for "zvknh[a|b]" instructions.
  3. Rename and restructure the function from CheckInvalidEGW to CheckInvalidVLENandLMUL.
This revision is now accepted and ready to land.Aug 8 2023, 4:24 PM
This revision was landed with ongoing or failed builds.Aug 8 2023, 5:10 PM
This revision was automatically updated to reflect the committed changes.
dyung added a subscriber: dyung.Aug 8 2023, 6:37 PM

The test you added clang/test/Sema/zvk-invalid.c is failing on bots, for example https://lab.llvm.org/buildbot/#/builders/139/builds/47055.

Is the test missing a REQUIRES: riscv-registered-target line by any chance?

The test you added clang/test/Sema/zvk-invalid.c is failing on bots, for example https://lab.llvm.org/buildbot/#/builders/139/builds/47055.

Is the test missing a REQUIRES: riscv-registered-target line by any chance?

Yeah, you are right, I will have a patch to fix it, thanks!