Skip to content

[AArch64][Clang] Fix linker error for function multiversioning #74358

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

Merged
merged 1 commit into from
Jan 22, 2024
Merged

[AArch64][Clang] Fix linker error for function multiversioning #74358

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
28 changes: 10 additions & 18 deletions clang/lib/CodeGen/CodeGenModule.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1688,8 +1688,10 @@ static void AppendCPUSpecificCPUDispatchMangling(const CodeGenModule &CGM,
static void AppendTargetVersionMangling(const CodeGenModule &CGM,
const TargetVersionAttr *Attr,
raw_ostream &Out) {
if (Attr->isDefaultVersion())
if (Attr->isDefaultVersion()) {
Out << ".default";
return;
}
Out << "._";
const TargetInfo &TI = CGM.getTarget();
llvm::SmallVector<StringRef, 8> Feats;
Expand Down Expand Up @@ -1752,8 +1754,10 @@ static void AppendTargetClonesMangling(const CodeGenModule &CGM,
const TargetInfo &TI = CGM.getTarget();
if (TI.getTriple().isAArch64()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please explain why AArch64 needs to be 'specially' handled here? I can't recall where but other targets get a .default mangling as well right? Why isn't AArch64 being handled wherever it is happening for other targets?

Copy link
Member Author

Choose a reason for hiding this comment

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

As I see for other targets "default" is just copied as a feature string.
For AArch64 we want to have clones per feature or set of features instead of CPU.

__attribute__((target_clones("crc32", "aes+sha1")))
int foo(){..}

would give 3 clones; the default, crc32 and one for aes plus sha1 mangled as foo.default, foo._Mcrc32, foo._Msha1Maes respectively.
Also "aes+sha1" should be the same as "sha1+aes" after mangling.

Copy link
Contributor

Choose a reason for hiding this comment

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

@DanielKristofKiss slightly off topic: it's too bad default is implied, as it makes target_clones incompatible with target_version for constructs like this:

__attribute__((target_clones("dotprod", "aes"))) 
int callee(void) {
    return 42;
}

__attribute__((target_version("default"))) 
int callee(void) {
    return 0;
}

int caller(void) {
    return callee();
}

Copy link
Contributor

Choose a reason for hiding this comment

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

@jroelofs currently in FMV spec:

The following attributes trigger the multi version code generation: __attribute__((target_version("name"))) and __attribute__((target_clones("name",...))).
These attributes can't be mixed with each other.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, it's too bad this incompatibility is baked into the spec. Are there other reasons why this can't be supported?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was not supported in the original function multi versioning neither and excluded to that as we building top of it.
Anyway created a ticket for it ARM-software/acle#291 . I need to check what GCC and others say about it.

StringRef FeatureStr = Attr->getFeatureStr(VersionIndex);
if (FeatureStr == "default")
if (FeatureStr == "default") {
Out << ".default";
return;
}
Out << "._";
SmallVector<StringRef, 8> Features;
FeatureStr.split(Features, "+");
Expand Down Expand Up @@ -3999,6 +4003,8 @@ void CodeGenModule::EmitMultiVersionFunctionDefinition(GlobalDecl GD,
EmitGlobalFunctionDefinition(GD.getWithMultiVersionIndex(I), nullptr);
// Ensure that the resolver function is also emitted.
GetOrCreateMultiVersionResolver(GD);
} else if (FD->hasAttr<TargetVersionAttr>()) {
GetOrCreateMultiVersionResolver(GD);
} else
EmitGlobalFunctionDefinition(GD, GV);
}
Expand Down Expand Up @@ -4180,14 +4186,7 @@ void CodeGenModule::emitMultiVersionFunctions() {
llvm::Constant *ResolverConstant = GetOrCreateMultiVersionResolver(GD);
if (auto *IFunc = dyn_cast<llvm::GlobalIFunc>(ResolverConstant)) {
ResolverConstant = IFunc->getResolver();
// In Aarch64, default versions of multiversioned functions are mangled to
// their 'normal' assembly name. This deviates from other targets which
// append a '.default' string. As a result we need to continue appending
// .ifunc in Aarch64.
// FIXME: Should Aarch64 mangling for 'default' multiversion function and
// in turn ifunc function match that of other targets?
if (FD->isTargetClonesMultiVersion() &&
!getTarget().getTriple().isAArch64()) {
if (FD->isTargetClonesMultiVersion()) {
const CGFunctionInfo &FI = getTypes().arrangeGlobalDeclaration(GD);
llvm::FunctionType *DeclTy = getTypes().GetFunctionType(FI);
std::string MangledName = getMangledNameImpl(
Expand Down Expand Up @@ -4368,14 +4367,7 @@ llvm::Constant *CodeGenModule::GetOrCreateMultiVersionResolver(GlobalDecl GD) {
// a separate resolver).
std::string ResolverName = MangledName;
if (getTarget().supportsIFunc()) {
// In Aarch64, default versions of multiversioned functions are mangled to
// their 'normal' assembly name. This deviates from other targets which
// append a '.default' string. As a result we need to continue appending
// .ifunc in Aarch64.
// FIXME: Should Aarch64 mangling for 'default' multiversion function and
// in turn ifunc function match that of other targets?
if (!FD->isTargetClonesMultiVersion() ||
getTarget().getTriple().isAArch64())
if (!FD->isTargetClonesMultiVersion())
ResolverName += ".ifunc";
} else if (FD->isTargetMultiVersion()) {
ResolverName += ".resolver";
Expand Down
Loading