This is an archive of the discontinued LLVM Phabricator instance.

Differential D112613

[Clang][RISCV] Change TARGET_BUILTIN to require zve32x for vector instruction
ClosedPublic

Authored by eopXD on Oct 27 2021, 4:47 AM.

Details

Reviewers
kito.cheng
khchen
craig.topper
kito-cheng
Commits
rG14c5fd920b0e: [Clang][RISCV] Change TARGET_BUILTIN to require zve32x for vector instruction
Summary

According to v-spec v1.0, zve-32x is the new minimum extension to include
to have vector instructions.

Diff Detail

Unit TestsFailed

TimeTest
520 msx64 debian > AddressSanitizer-x86_64-linux-dynamic.TestCases::exitcode.cpp
820 msx64 debian > AddressSanitizer-x86_64-linux.TestCases::exitcode.cpp

Event Timeline

eopXD created this revision.Oct 27 2021, 4:47 AM
Herald added subscribers: achieveartificialintelligence, vkmr, frasercrmck and 26 others. · View Herald TranscriptOct 27 2021, 4:47 AM
eopXD requested review of this revision.Oct 27 2021, 4:47 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptOct 27 2021, 4:47 AM
Herald added subscribers: llvm-commits, cfe-commits, MaskRay. · View Herald Transcript
Harbormaster completed remote builds in B130918: Diff 382610.Oct 27 2021, 4:47 AM
eopXD added a parent revision: D112408: [RISCV] Add the zve extension according to the v1.0 spec.Oct 27 2021, 4:51 AM
eopXD retitled this revision from [RISCV][RVV] Change TARGET_BUILTIN require to zve32x to [WIP][RISCV] Change TARGET_BUILTIN require to zve32x for vector instruction.
eopXD edited the summary of this revision. (Show Details)Oct 27 2021, 4:55 AM
eopXD updated this revision to Diff 382615.Oct 27 2021, 5:06 AM

Update code.

Harbormaster completed remote builds in B130920: Diff 382615.Oct 27 2021, 5:50 AM
eopXD updated this revision to Diff 383977.Nov 1 2021, 11:07 PM

Rebase.

Herald added subscribers: VincentWu, luke957. · View Herald TranscriptNov 1 2021, 11:07 PM
Harbormaster completed remote builds in B131886: Diff 383977.Nov 1 2021, 11:47 PM
eopXD updated this revision to Diff 383983.Nov 1 2021, 11:52 PM

Follow clang-format.

eopXD added a child revision: D112986: [Clang][RISCV] Restrict rvv builtins with zve macros.Nov 1 2021, 11:57 PM
eopXD retitled this revision from [WIP][RISCV] Change TARGET_BUILTIN require to zve32x for vector instruction to [RISCV] Change TARGET_BUILTIN require to zve32x for vector instruction.Nov 2 2021, 12:09 AM
eopXD edited the summary of this revision. (Show Details)
Harbormaster completed remote builds in B131892: Diff 383983.Nov 2 2021, 12:32 AM
craig.topper added a comment.Nov 5 2021, 1:09 PM

The title of this patch isn't descriptive of the changes in this patch. Based on the title I would only expect changes to RISCVEmitter.cpp. Can you clarify the title?

craig.topper added inline comments.Nov 5 2021, 1:17 PM
clang/test/CodeGen/RISCV/rvv-intrinsics-overloaded/vaadd.c
3 ↗(On Diff #383983)

Why do we need to add zve32x and zve64x to the command line? Doesn't zve64x imply zve32x?

eopXD edited parent revisions, added: D113336: [RISCV] Imply extensions in RISCVTargetInfo::initFeatureMap; removed: D112408: [RISCV] Add the zve extension according to the v1.0 spec.Nov 6 2021, 2:12 AM
eopXD added inline comments.Nov 6 2021, 2:14 AM
clang/test/CodeGen/RISCV/rvv-intrinsics-overloaded/vaadd.c
3 ↗(On Diff #383983)

Just added D113336 to resolve this. I will rebase this patch and change test cases.

eopXD updated this revision to Diff 385276.Nov 6 2021, 7:47 AM

Rebase.

Harbormaster completed remote builds in B132843: Diff 385276.Nov 6 2021, 7:48 AM
eopXD updated this revision to Diff 385278.Nov 6 2021, 7:55 AM

Update code.

eopXD marked an inline comment as done.Nov 6 2021, 7:56 AM
eopXD added inline comments.
clang/test/CodeGen/RISCV/rvv-intrinsics-overloaded/vaadd.c
3 ↗(On Diff #383983)

Rebased.

Harbormaster completed remote builds in B132845: Diff 385278.Nov 6 2021, 8:57 AM
eopXD edited child revisions, added: D112987: [RISCV] Bump rvv-related extensions from 0.10 to 1.0; removed: D112986: [Clang][RISCV] Restrict rvv builtins with zve macros.Jan 19 2022, 11:57 PM
eopXD mentioned this in D113336: [RISCV] Imply extensions in RISCVTargetInfo::initFeatureMap.Jan 20 2022, 12:51 AM
eopXD edited the summary of this revision. (Show Details)Jan 20 2022, 1:36 AM
eopXD updated this revision to Diff 401549.Jan 20 2022, 1:37 AM

Rebase.

eopXD removed a parent revision: D113336: [RISCV] Imply extensions in RISCVTargetInfo::initFeatureMap.Jan 20 2022, 1:48 AM
eopXD updated this revision to Diff 401554.Jan 20 2022, 1:56 AM

Rebase.

frasercrmck added a comment.Jan 20 2022, 3:00 AM

LGTM but I don't have full scope on the builtins side of things. I'd also add a missing word to the title: "Change TARGET_BUILTIN to require ..."

Harbormaster completed remote builds in B144518: Diff 401554.Jan 20 2022, 3:04 AM
eopXD retitled this revision from [RISCV] Change TARGET_BUILTIN require to zve32x for vector instruction to [RISCV] Change TARGET_BUILTIN to require zve32x for vector instruction.Jan 20 2022, 4:04 AM

Apply @frasercrmck 's suggested title, thanks!

eopXD retitled this revision from [RISCV] Change TARGET_BUILTIN to require zve32x for vector instruction to [Clang][RISCV] Change TARGET_BUILTIN to require zve32x for vector instruction.Jan 20 2022, 4:09 AM
kito-cheng accepted this revision.Jan 20 2022, 6:35 AM

LGTM

This revision is now accepted and ready to land.Jan 20 2022, 6:35 AM
Closed by commit rG14c5fd920b0e: [Clang][RISCV] Change TARGET_BUILTIN to require zve32x for vector instruction (authored by eopXD). · Explain WhyJan 20 2022, 6:53 AM
This revision was automatically updated to reflect the committed changes.
eopXD added a commit: rG14c5fd920b0e: [Clang][RISCV] Change TARGET_BUILTIN to require zve32x for vector instruction.

Revision Contents

PathSize
clang/
utils/
TableGen/
2 lines

Diff 385278

clang/utils/TableGen/RISCVVEmitter.cpp

Show First 20 Lines • Show All 1,015 Lines • ▼ Show 20 Linesvoid RVVEmitter::createBuiltins(raw_ostream &OS) {
std::vector<std::unique_ptr<RVVIntrinsic>> Defs; std::vector<std::unique_ptr<RVVIntrinsic>> Defs;
createRVVIntrinsics(Defs); createRVVIntrinsics(Defs);
// Map to keep track of which builtin names have already been emitted. // Map to keep track of which builtin names have already been emitted.
StringMap<RVVIntrinsic *> BuiltinMap; StringMap<RVVIntrinsic *> BuiltinMap;
OS << "#if defined(TARGET_BUILTIN) && !defined(RISCVV_BUILTIN)\n"; OS << "#if defined(TARGET_BUILTIN) && !defined(RISCVV_BUILTIN)\n";
OS << "#define RISCVV_BUILTIN(ID, TYPE, ATTRS) TARGET_BUILTIN(ID, TYPE, " OS << "#define RISCVV_BUILTIN(ID, TYPE, ATTRS) TARGET_BUILTIN(ID, TYPE, "
"ATTRS, \"experimental-v\")\n"; "ATTRS, \"experimental-zve32x\")\n";
OS << "#endif\n"; OS << "#endif\n";
for (auto &Def : Defs) { for (auto &Def : Defs) {
auto P = auto P =
BuiltinMap.insert(std::make_pair(Def->getBuiltinName(), Def.get())); BuiltinMap.insert(std::make_pair(Def->getBuiltinName(), Def.get()));
if (!P.second) { if (!P.second) {
// Verify that this would have produced the same builtin definition. // Verify that this would have produced the same builtin definition.
if (P.first->second->hasAutoDef() != Def->hasAutoDef()) { if (P.first->second->hasAutoDef() != Def->hasAutoDef()) {
PrintFatalError("Builtin with same name has different hasAutoDef"); PrintFatalError("Builtin with same name has different hasAutoDef");
▲ Show 20 Lines • Show All 294 Lines • Show Last 20 Lines