Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RISCV][FMV] Support target_version #99040

Merged
merged 13 commits into from
Oct 4, 2024
Merged

[RISCV][FMV] Support target_version #99040

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
54b5d68
[RISCV][FMV] Support target_version
BeMg Mar 19, 2024
68cbc16
Use single quote for char
BeMg Sep 13, 2024
c7ad1a9
Refine comment
BeMg Sep 13, 2024
5414680
Update attr-target-version-riscv-invalid.c
BeMg Sep 18, 2024
7c6f108
Reduce some duplication
BeMg Sep 18, 2024
5e00115
Add assert for Aarch64 target
BeMg Sep 23, 2024
2fcfc9c
isPriority -> hasPriority
BeMg Sep 23, 2024
a487f19
Guard the default with arch
BeMg Sep 23, 2024
b89211c
Guard hasFeature("fmv") with getTriple().isAArch64()
BeMg Sep 23, 2024
8cd911d
Drop blank line
BeMg Sep 24, 2024
e98d84f
Add assert
BeMg Sep 24, 2024
e449161
IsDefault -> HasDefault
BeMg Sep 24, 2024
9c70204
Warning more invalid pattern
BeMg Sep 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions clang/lib/AST/ASTContext.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14270,9 +14270,16 @@ void ASTContext::getFunctionFeatureMap(llvm::StringMap<bool> &FeatureMap,
Target->initFeatureMap(FeatureMap, getDiagnostics(), TargetCPU, Features);
}
} else if (const auto *TV = FD->getAttr<TargetVersionAttr>()) {
llvm::SmallVector<StringRef, 8> Feats;
TV->getFeatures(Feats);
std::vector<std::string> Features = getFMVBackendFeaturesFor(Feats);
std::vector<std::string> Features;
if (Target->getTriple().isRISCV()) {
ParsedTargetAttr ParsedAttr = Target->parseTargetAttr(TV->getName());
Features.insert(Features.begin(), ParsedAttr.Features.begin(),
ParsedAttr.Features.end());
} else {
llvm::SmallVector<StringRef, 8> Feats;
BeMg marked this conversation as resolved.
Show resolved Hide resolved
TV->getFeatures(Feats);
Features = getFMVBackendFeaturesFor(Feats);
}
Features.insert(Features.begin(),
Target->getTargetOpts().FeaturesAsWritten.begin(),
Target->getTargetOpts().FeaturesAsWritten.end());
Expand Down
6 changes: 5 additions & 1 deletion clang/lib/CodeGen/CodeGenModule.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4268,8 +4268,12 @@ void CodeGenModule::emitMultiVersionFunctions() {
} else if (const auto *TVA = CurFD->getAttr<TargetVersionAttr>()) {
if (TVA->isDefaultVersion() && IsDefined)
ShouldEmitResolver = true;
TVA->getFeatures(Feats);
llvm::Function *Func = createFunction(CurFD);
if (getTarget().getTriple().isRISCV()) {
Feats.push_back(TVA->getName());
} else {
TVA->getFeatures(Feats);
BeMg marked this conversation as resolved.
Show resolved Hide resolved
}
Options.emplace_back(Func, /*Architecture*/ "", Feats);
} else if (const auto *TC = CurFD->getAttr<TargetClonesAttr>()) {
if (IsDefined)
Expand Down
43 changes: 31 additions & 12 deletions clang/lib/Sema/SemaDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10319,8 +10319,10 @@ Sema::ActOnFunctionDeclarator(Scope *S, Declarator &D, DeclContext *DC,
// Handle attributes.
ProcessDeclAttributes(S, NewFD, D);
const auto *NewTVA = NewFD->getAttr<TargetVersionAttr>();
if (NewTVA && !NewTVA->isDefaultVersion() &&
!Context.getTargetInfo().hasFeature("fmv")) {
if (Context.getTargetInfo().getTriple().isRISCV()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you happen to know what the "fmv" feature is? I don't have context here. Any docs, or a good starting point in the code to understand what we're leaving out here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understand is the fmv is a aarch64 feature to control the function multi-versioning. They(target_version and fmv) are both be introduced by this commit.

The ACLE spec and review comment can help to understand the context.

Additionally, it is treated as a generic option in the Clang command-line options, and it was also introduced in the same commit.

Do you think it's a good idea to add this similar feature to RISC-V, or should we just skip it?

// RISC-V doesn't support fmv feature.
} else if (NewTVA && !NewTVA->isDefaultVersion() &&
!Context.getTargetInfo().hasFeature("fmv")) {
// Don't add to scope fmv functions declarations if fmv disabled
AddToScope = false;
return NewFD;
Expand Down Expand Up @@ -11027,13 +11029,27 @@ static bool CheckMultiVersionValue(Sema &S, const FunctionDecl *FD) {
}

if (TVA) {
llvm::SmallVector<StringRef, 8> Feats;
TVA->getFeatures(Feats);
for (const auto &Feat : Feats) {
if (!TargetInfo.validateCpuSupports(Feat)) {
S.Diag(FD->getLocation(), diag::err_bad_multiversion_option)
<< Feature << Feat;
return true;
if (S.getASTContext().getTargetInfo().getTriple().isRISCV()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The need to version all of this by target is unfortunate. Is there a way we can reduce the duplication here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reduce the part of duplication, and the remain part is causing by different syntax between aarch64 and RISC-V.

To reducing the remain part of duplication, we could implement the RISC-V syntax inside clang/include/clang/Basic/Attr.td getFeatures or make Aarch64 using the AArch64TargetInfo::parseTargetAttr for target_version, or make something like parseTargetVersionAttr hook here.

By the way, the parsing of target_clones syntax is controlled by the target. (Reference:

llvm-project/clang/lib/Sema/SemaDeclAttr.cpp

Line 3121 in 9cd9377

if (TInfo.getTriple().isAArch64()) {
)

ParsedTargetAttr ParseInfo =
S.getASTContext().getTargetInfo().parseTargetAttr(TVA->getName());
for (const auto &Feat : ParseInfo.Features) {
StringRef BareFeat = StringRef{Feat}.substr(1);

if (!TargetInfo.isValidFeatureName(BareFeat)) {
S.Diag(FD->getLocation(), diag::err_bad_multiversion_option)
<< Feature << BareFeat;
return true;
}
}
} else {
llvm::SmallVector<StringRef, 8> Feats;
TVA->getFeatures(Feats);
BeMg marked this conversation as resolved.
Show resolved Hide resolved
for (const auto &Feat : Feats) {
if (!TargetInfo.validateCpuSupports(Feat)) {
S.Diag(FD->getLocation(), diag::err_bad_multiversion_option)
<< Feature << Feat;
return true;
}
}
}
}
Expand Down Expand Up @@ -11314,7 +11330,8 @@ static bool PreviousDeclsHaveMultiVersionAttribute(const FunctionDecl *FD) {
}

static void patchDefaultTargetVersion(FunctionDecl *From, FunctionDecl *To) {
if (!From->getASTContext().getTargetInfo().getTriple().isAArch64())
if (!From->getASTContext().getTargetInfo().getTriple().isAArch64() &&
!From->getASTContext().getTargetInfo().getTriple().isRISCV())
return;

MultiVersionKind MVKindFrom = From->getMultiVersionKind();
Expand Down Expand Up @@ -15501,8 +15518,10 @@ Decl *Sema::ActOnStartOfFunctionDef(Scope *FnBodyScope, Decl *D,
FD->setInvalidDecl();
}
if (const auto *Attr = FD->getAttr<TargetVersionAttr>()) {
if (!Context.getTargetInfo().hasFeature("fmv") &&
!Attr->isDefaultVersion()) {
if (Context.getTargetInfo().getTriple().isRISCV()) {
BeMg marked this conversation as resolved.
Show resolved Hide resolved
// RISC-V doesn't support fmv feature.
} else if (!Context.getTargetInfo().hasFeature("fmv") &&
!Attr->isDefaultVersion()) {
// If function multi versioning disabled skip parsing function body
// defined with non-default target_version attribute
if (SkipBody)
Expand Down
39 changes: 39 additions & 0 deletions clang/lib/Sema/SemaDeclAttr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3056,6 +3056,45 @@ bool Sema::checkTargetVersionAttr(SourceLocation LiteralLoc, Decl *D,
enum SecondParam { None };
enum ThirdParam { Target, TargetClones, TargetVersion };
llvm::SmallVector<StringRef, 8> Features;
if (Context.getTargetInfo().getTriple().isRISCV()) {

BeMg marked this conversation as resolved.
Show resolved Hide resolved
llvm::SmallVector<StringRef, 8> AttrStrs;
AttrStr.split(AttrStrs, ';');

bool IsPriority = false;
BeMg marked this conversation as resolved.
Show resolved Hide resolved
bool IsDefault = false;
BeMg marked this conversation as resolved.
Show resolved Hide resolved
for (auto &AttrStr : AttrStrs) {
// Only support arch=+ext,... syntax.
topperc marked this conversation as resolved.
Show resolved Hide resolved
if (AttrStr.starts_with("arch=+")) {
ParsedTargetAttr TargetAttr =
Context.getTargetInfo().parseTargetAttr(AttrStr);

if (TargetAttr.Features.empty() ||
llvm::any_of(TargetAttr.Features, [&](const StringRef Ext) {
return !RISCV().isValidFMVExtension(Ext);
}))
return Diag(LiteralLoc, diag::warn_unsupported_target_attribute)
<< Unsupported << None << AttrStr << TargetVersion;
} else if (AttrStr.starts_with("default")) {
IsDefault = true;
} else if (AttrStr.consume_front("priority=")) {
IsPriority = true;
int Digit;
if (AttrStr.getAsInteger(0, Digit))
return Diag(LiteralLoc, diag::warn_unsupported_target_attribute)
<< Unsupported << None << AttrStr << TargetVersion;
} else {
return Diag(LiteralLoc, diag::warn_unsupported_target_attribute)
<< Unsupported << None << AttrStr << TargetVersion;
}
}

if (IsPriority && IsDefault)
return Diag(LiteralLoc, diag::warn_unsupported_target_attribute)
<< Unsupported << None << AttrStr << TargetVersion;

return false;
}
AttrStr.split(Features, "+");
for (auto &CurFeature : Features) {
CurFeature = CurFeature.trim();
Expand Down
13 changes: 13 additions & 0 deletions clang/test/CodeGen/attr-target-version-riscv-invalid.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// RUN: not %clang_cc1 -triple riscv64 -target-feature +i -emit-llvm -o - %s 2>&1 | FileCheck %s --check-prefix=CHECK-UNSUPPORT-OS

// CHECK-UNSUPPORT-OS: error: target_clones is currently only supported on Linux
__attribute__((target_version("default"))) int foo(void) {
return 2;
}

__attribute__((target_version("arch=+c"))) int foo(void) {
return 2;
}


int bar() { return foo(); }
Loading
Loading