This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Model vxrm control for vsmul, vssra, vssrl, vnclip, and vnclipu
ClosedPublic

Authored by eopXD on Jun 13 2023, 7:06 PM.

Details

Summary

Depends on D151397.

This patch follows the patch-set of D151395. This patch seeks to
update all the remaining fixed-point intrinsics to model vxrm
control, adding rounding mode control for vsmul, vssra,
vssrl, vnclip, and vnclipu.

Diff Detail

Event Timeline

eopXD created this revision.Jun 13 2023, 7:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 13 2023, 7:06 PM
eopXD requested review of this revision.Jun 13 2023, 7:06 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 13 2023, 7:06 PM
eopXD updated this revision to Diff 531157.Jun 13 2023, 8:21 PM

Resolve test case failures.

eopXD updated this revision to Diff 531178.Jun 13 2023, 10:46 PM

Resolve test case failure.

eopXD retitled this revision from [RISCV] Model vxrm control for vsmul, vssra, vssrl, vnclip, and vnclipu to [1/2][RISCV] Model vxrm control for vsmul, vssra, vssrl, vnclip, and vnclipu.Jun 15 2023, 2:09 AM
eopXD edited the summary of this revision. (Show Details)Jun 15 2023, 2:14 AM
eopXD updated this revision to Diff 531681.Jun 15 2023, 3:31 AM
eopXD edited the summary of this revision. (Show Details)

Edit codegen test case for llvm/test/CodeGen/RISCV/rvv/{vnclip.ll/vnclipu.ll} correctly.

eopXD updated this revision to Diff 531690.Jun 15 2023, 4:01 AM

Bump CI.

SemaChecking.cpp?

eopXD updated this revision to Diff 531963.Jun 15 2023, 6:26 PM

Add semanic check to the vxrm parameter.

eopXD updated this revision to Diff 531966.Jun 15 2023, 6:44 PM

Fix sema checking test case.

eopXD retitled this revision from [1/2][RISCV] Model vxrm control for vsmul, vssra, vssrl, vnclip, and vnclipu to [RISCV] Model vxrm control for vsmul, vssra, vssrl, vnclip, and vnclipu.Jun 15 2023, 7:11 PM
eopXD edited the summary of this revision. (Show Details)
craig.topper added inline comments.Jun 15 2023, 7:19 PM
llvm/include/llvm/IR/IntrinsicsRISCV.td
695

Something I just noticed today. I think we can use LLVMExtendedType<0> instead of llvm_any_vector_ty for the second input operand to make it automatically 2x the element width of the output. Does that simplify the frontend much if we don't need to use an extra IntrinsicType? It would be a separate patch of course if we did it.

This revision is now accepted and ready to land.Jun 15 2023, 7:20 PM
eopXD added inline comments.Jun 15 2023, 7:21 PM
llvm/include/llvm/IR/IntrinsicsRISCV.td
695

This won't affect the front-end. C intrinsics are directly lowered into LLVM intrinsics here.

craig.topper added inline comments.Jun 15 2023, 7:24 PM
clang/include/clang/Basic/riscv_vector.td
2198

I think we if we used LLVMTruncatedType<0> in IntrinsicsRISCV.td we wouldn't need to pass to Ops[Offset]->getType() here. Which I think makes this code identical to the sadd ManualCodegen?

2198

Oops I mean ExtendedType

eopXD updated this revision to Diff 532532.Jun 18 2023, 10:56 PM

Rebase to latest main.

eopXD updated this revision to Diff 532830.Jun 20 2023, 2:14 AM

Rebase.

Is there a way to detect whether the compiler already includes this change, preferably via preprocessor?
I tried #ifdef __RISCV_VXRM_RDN and #if __has_builtin(__RISCV_VXRM_RDN).

This is a breaking change for us. Updating code to the new required signature fails to compile on the older toolchain (which is still in use). Leaving the code in the old style (no extra VXRM argument) is also not an option because other users have already updated their LLVM.

eopXD added a comment.Jun 22 2023, 3:15 AM

Is there a way to detect whether the compiler already includes this change, preferably via preprocessor?
I tried #ifdef __RISCV_VXRM_RDN and #if __has_builtin(__RISCV_VXRM_RDN).

This is a breaking change for us. Updating code to the new required signature fails to compile on the older toolchain (which is still in use). Leaving the code in the old style (no extra VXRM argument) is also not an option because other users have already updated their LLVM.

Hi Jan,

Thanks for checking. I was planning to bump the version number of the RVV intrinsics [0] in the next LLVM release for users to identify what version of intrinsics they are using. My fault for not being aware that projects may be using the trunk upstream compiler.

Temporarily, I can add an indicator for users to identify this breaking change, however the new LLVM branch out is coming on July 4th, so I personally think we should just bump the version number for the next LLVM release.

If this is a really urgent issue, lets discuss more.

[0] https://github.com/llvm/llvm-project/blob/main/clang/lib/Basic/Targets/RISCV.cpp#L200

Thanks for your quick reply. Yes, we are tracking trunk with a short lag, and unfortunately this is mostly beyond our control.
Now that I have updated the Highway code and toolchain reference in user code, we are fine for the moment.
Please do update the intrinsics version number when there is a breaking change, though :)