Page MenuHomePhabricator

[RISCV] Add vadd with mask and without mask builtin.
ClosedPublic

Authored by HsiangKai on Dec 16 2020, 11:38 PM.

Details

Summary

Demonstrate how to add RISC-V V builtins and lower them to IR intrinsics for V extension.

Authored-by: Roger Ferrer Ibanez <rofirrim@gmail.com>
Co-Authored-by: Hsiangkai Wang <kai.wang@sifive.com>

Diff Detail

Event Timeline

HsiangKai created this revision.Dec 16 2020, 11:38 PM
HsiangKai requested review of this revision.Dec 16 2020, 11:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 16 2020, 11:38 PM

Update test cases to include rv32 target.

khchen added a subscriber: khchen.Dec 17 2020, 10:46 AM
craig.topper added inline comments.Dec 18 2020, 12:36 PM
clang/lib/CodeGen/CGBuiltin.cpp
17940

I think this whole sequence is equivalent to

std::rotate(Ops.begin(), Ops.begin() + 1, Ops.end() - 1);
HsiangKai updated this revision to Diff 312931.Dec 19 2020, 5:37 AM

Address @craig.topper's comments.

khchen added inline comments.Dec 19 2020, 7:03 AM
clang/lib/Basic/Targets/RISCV.cpp
283

HasRISCVVTypes is an undefined variable?

HsiangKai added inline comments.Dec 19 2020, 4:36 PM
clang/lib/Basic/Targets/RISCV.cpp
283

I should move the snippet to D92715.

HsiangKai updated this revision to Diff 313014.Dec 20 2020, 8:00 PM

Address @khchen's comments.

D95016 focuses on the Clang RVV builtin generator and it depends on this commit. I know it is clumsy to list the combinations for vadd plainly, but it is a simple way to demonstrate how to add RVV builtins in Clang. Could we accept this patch and D92715 first if there is no concern about these two patches?

Jim added inline comments.Feb 3 2021, 7:21 PM
clang/test/CodeGen/RISCV/vadd.c
23
jrtc27 added inline comments.Feb 3 2021, 7:28 PM
clang/test/CodeGen/RISCV/vadd.c
23

rvvintrin.h or whatever it's called will define wrappers. Providing things under a name that's not prefixed by __ can cause collisions with existing code that's not trying to use these intrinsics, since the names are technically available for it to use, though in practice that seems extremely unlikely for these names. However, using proper __-prefixed names is convention, as is using __builtin_ for anything that's a compiler builtin. This is purely an implementation detail that the user should not need to care about unless they want to not include the header they're meant to include, at which point we provide no guarantees.

Jim added inline comments.Mon, Feb 8, 10:06 PM
clang/include/clang/Basic/DiagnosticSemaKinds.td
11139

Add blank line.
Is it "err_riscvv_builtin_not_enabled"?

clang/lib/Basic/Targets/RISCV.cpp
89

Builtins for other extension don't have "__builtin_rvv_" prefix.

khchen added inline comments.Tue, Feb 9, 7:42 AM
clang/lib/Basic/Targets/RISCV.cpp
89

maybe we could rename BuiltinsRISCV.def as BuiltinsRVV.def, and other extension defines their own .def file?

@Jim do you have any suggestion?

craig.topper added inline comments.Tue, Feb 9, 1:22 PM
clang/lib/Basic/Targets/RISCV.cpp
89

Don't most targets pass the full name with the prefix to the BUILTIN macro?

Jim added inline comments.Tue, Feb 16, 6:46 PM
clang/lib/Basic/Targets/RISCV.cpp
89

Most of targets pass the full name with the prefix to the BUILTIN macro.
It can define the full name with the prefix in BuiltinsRISCV.def

Address @Jim's comments.

HsiangKai added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
11139

I use the name that is defined in BSC version. It makes sense to me that it means "the risc-v V builtins are not useable."

@rogfer01, do you have any opinion on it?

HsiangKai marked 6 inline comments as done.Tue, Feb 16, 11:31 PM
lenary removed a subscriber: lenary.Wed, Feb 17, 12:30 AM
Jim added inline comments.Wed, Feb 17, 1:12 AM
clang/lib/Basic/Targets/RISCV.cpp
89

Should it put together with getTargetBuiltins function?

HsiangKai updated this revision to Diff 324485.Wed, Feb 17, 5:56 PM

Move BuiltinInfo closed to getTargetBuiltins.

HsiangKai marked an inline comment as done.Wed, Feb 17, 5:56 PM
craig.topper added inline comments.Mon, Feb 22, 6:19 PM
clang/include/clang/Basic/BuiltinsRISCV.def
2

Need copyright header

clang/include/clang/Basic/DiagnosticSemaKinds.td
11139

I'm not sure that's the right spelling of useable. It can also be spelled usable. But it doesn't appear in the message so maybe the name should be closer to wording of the message? Maybe err_riscv_builtin_requires_v. That makes it like err_mips_builtin_requires_dsp

HsiangKai updated this revision to Diff 325671.Mon, Feb 22, 9:52 PM

Address @craig.topper's comments.

HsiangKai marked 2 inline comments as done.Mon, Feb 22, 9:53 PM
This revision is now accepted and ready to land.Tue, Feb 23, 2:28 PM
This revision was automatically updated to reflect the committed changes.