Page MenuHomePhabricator

[RISCV][Clang] Add RVV vleff intrinsic functions.

Authored by HsiangKai on Mar 23 2021, 12:49 AM.

Diff Detail

Event Timeline

HsiangKai created this revision.Mar 23 2021, 12:49 AM
HsiangKai requested review of this revision.Mar 23 2021, 12:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2021, 12:49 AM
HsiangKai updated this revision to Diff 332851.Mar 23 2021, 7:20 PM

Refine implementation.

maybe we could merge different test into one vleff.c?

9 ↗(On Diff #332851)

nit: skip the temporary file and remove +experimental-zfh and -fallow-half-arguments-and-returns.

jrtc27 added inline comments.Mar 24 2021, 6:22 AM
9 ↗(On Diff #332851)

Can we please stop putting ASM tests in Clang, and if they really are needed, splitting them out into their own files separate from the IR tests?

craig.topper added inline comments.Mar 24 2021, 8:59 AM
9 ↗(On Diff #332851)

For more background, this approach was copied from AArch64 SVE where they said this was beneficial to catch warnings from TypeSize's implicit conversion to uint64_t.

jrtc27 added inline comments.Mar 24 2021, 10:06 AM
9 ↗(On Diff #332851)

You can have the ASM tests in a separate .test file though so you can still run the IR tests without the backend if there's value in having the end-to-end tests

9 ↗(On Diff #332851)

(well s/tests/RUN lines/)

HsiangKai updated this revision to Diff 333663.Mar 27 2021, 2:30 AM

Address comments.

HsiangKai marked 4 inline comments as done.Mar 27 2021, 2:31 AM
khchen added inline comments.Mar 27 2021, 5:40 AM
9 ↗(On Diff #332851)

IMO, this is also useful to catch mismatch in generated IR intrinsic with backend because we have llvm_any_ty type in intrinsic IR . In the downstream we met the same bug several time which generated intrinsic from builtin can not be selected in backend, and it
was passed the builtin->IR test because we didn't have ASM test.

Do you mean we need to add the IR tests which are generated by clang builtin?
Currently we have the IR tests but they are generated by a script, do you mean we need to overwrite them?
BTW, they are a little different to IR generated by builtin, for example:
IR intrinsic is llvm.riscv.vadd.nxv1i8.nxv1i8 in RV32 and RV64
IR intrinsic is llvm.riscv.vadd.nxv1i8.nxv1i8.i32 in RV32, llvm.riscv.vadd.nxv1i8.nxv1i8.i64 in RV64.

I don't like to add too much IR tests, so I'm OK to remove ASM test. But in the future, reviewer maybe need to verify the generated IR can generate asm without any error.

craig.topper added inline comments.Mar 27 2021, 12:31 PM
9 ↗(On Diff #332851)

I'm conflicted on this.

Given how the IR intrinsics are making heavy use of any_ty we lose a lot of type checking in the intrinsic creation. We've had multiple case where the intrinsics the types the frontend generated were different than what the backend tests were testing. So from that perspective, the end-to-end tests have value.

I don't really like the idea of having two copies of every test. Maybe we could have multiple test files that use a common Input file?

I'm not completely convinced it is important to be able to test the IR gen without the backend being built. The most likely changes that would cause the -emit-llvm tests to fail or need to be updated are changes to RISCV specific code in the .td file, our tablegen backend, or CGBuiltin.cpp. I would expect a RISCV developer touching those files to be compiling the RISCV backend.

HsiangKai updated this revision to Diff 333758.Mar 28 2021, 8:36 PM

Add back ASM tests.

We use lots of any_ty to model IR intrinsics. Using end-to-end tests could help us verify the correctness of C builtins.

LGTM, thanks

khchen accepted this revision.Apr 8 2021, 11:44 PM


This revision is now accepted and ready to land.Apr 8 2021, 11:44 PM
This revision was automatically updated to reflect the committed changes.