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.

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

[Clang] Fix linker error for function multiversioning #71706

Merged
merged 9 commits into from
Dec 5, 2023

Conversation

elizabethandrews
Copy link
Contributor

@elizabethandrews elizabethandrews commented Nov 8, 2023

Currently target_clones attribute results in a linker error when there are no multi-versioned function declarations in the calling TU.

In the calling TU, the call is generated with the ‘normal’ assembly name. This does not match any of the versions or the ifunc, since version mangling includes a .versionstring, and the ifunc includes .ifunc suffix. The linker error is not seen with GCC since the mangling for the ifunc symbol in GCC is the ‘normal’ assembly name for function i.e. no ifunc suffix.

This PR removes the .ifunc suffix to match GCC. It also adds alias with the .ifunc suffix so as to ensure backward compatibility.

The changes exclude aarch64 target because the mangling for default versions on aarch64 does not include a .default suffix and is the 'normal' assembly name, unlike other targets. It is not clear to me what the correct behavior for this target is.

Old Phabricator review - https://reviews.llvm.org/D158666

Currently target_clones attribute results in a linker error when
there are no multi-versioned function declarations in the calling TU.

In the calling TU, the call is generated with the ‘normal’ assembly name.
This does not match any of the versions or the ifunc, since version mangling
includes a .versionstring, and the ifunc includes .ifunc suffix. The linker
error is not seen with GCC since the mangling for the ifunc symbol in GCC
is the ‘normal’ assembly name for function i.e. no ifunc suffix.

This PR removes the .ifunc suffix to match GCC. It also adds alias with
the .ifunc suffix so as to ensure backward compatibility.

The changes exclude aarch64 target because the mangling for default
versions on aarch64 does not include a .default suffix and is the
'normal' assembly name, unlike other targets. It is not clear to me
what the correct behavior for this target is.
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:codegen labels Nov 8, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Nov 8, 2023

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: None (elizabethandrews)

Changes

Currently target_clones attribute results in a linker error when there are no multi-versioned function declarations in the calling TU.

In the calling TU, the call is generated with the ‘normal’ assembly name. This does not match any of the versions or the ifunc, since version mangling includes a .versionstring, and the ifunc includes .ifunc suffix. The linker error is not seen with GCC since the mangling for the ifunc symbol in GCC is the ‘normal’ assembly name for function i.e. no ifunc suffix.

This PR removes the .ifunc suffix to match GCC. It also adds alias with the .ifunc suffix so as to ensure backward compatibility.

The changes exclude aarch64 target because the mangling for default versions on aarch64 does not include a .default suffix and is the 'normal' assembly name, unlike other targets. It is not clear to me what the correct behavior for this target is.


Full diff: https://github.com/llvm/llvm-project/pull/71706.diff

4 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+1)
  • (modified) clang/lib/CodeGen/CodeGenModule.cpp (+31-4)
  • (modified) clang/test/CodeGen/attr-target-clones.c (+21-12)
  • (modified) clang/test/CodeGenCXX/attr-target-clones.cpp (+17-10)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 8bac599f88503af..9e73fa03a0355b4 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -550,6 +550,7 @@ Bug Fixes in This Version
   Fixes (`#67687 <https://github.com/llvm/llvm-project/issues/67687>`_)
 - Fix crash from constexpr evaluator evaluating uninitialized arrays as rvalue.
   Fixes (`#67317 <https://github.com/llvm/llvm-project/issues/67317>`_)
+- Fix linker error when using multiversioned function defined in a different TU.
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index 2e96fff808113ba..b54c4296a0f9dc6 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -4098,8 +4098,26 @@ void CodeGenModule::emitMultiVersionFunctions() {
     }
 
     llvm::Constant *ResolverConstant = GetOrCreateMultiVersionResolver(GD);
-    if (auto *IFunc = dyn_cast<llvm::GlobalIFunc>(ResolverConstant))
+    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()) {
+        const CGFunctionInfo &FI = getTypes().arrangeGlobalDeclaration(GD);
+        llvm::FunctionType *DeclTy = getTypes().GetFunctionType(FI);
+        std::string MangledName = getMangledNameImpl(
+            *this, GD, FD, /*OmitMultiVersionMangling=*/true);
+        auto *Alias = llvm::GlobalAlias::create(
+            DeclTy, 0, llvm::Function::WeakODRLinkage, MangledName + ".ifunc",
+            IFunc, &getModule());
+        SetCommonAttributes(FD, Alias);
+      }
+    }
     llvm::Function *ResolverFunc = cast<llvm::Function>(ResolverConstant);
 
     ResolverFunc->setLinkage(getMultiversionLinkage(*this, GD));
@@ -4266,10 +4284,19 @@ llvm::Constant *CodeGenModule::GetOrCreateMultiVersionResolver(GlobalDecl GD) {
   // Holds the name of the resolver, in ifunc mode this is the ifunc (which has
   // a separate resolver).
   std::string ResolverName = MangledName;
-  if (getTarget().supportsIFunc())
-    ResolverName += ".ifunc";
-  else if (FD->isTargetMultiVersion())
+  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())
+      ResolverName += ".ifunc";
+  } else if (FD->isTargetMultiVersion()) {
     ResolverName += ".resolver";
+  }
 
   // If the resolver has already been created, just return it.
   if (llvm::GlobalValue *ResolverGV = GetGlobalValue(ResolverName))
diff --git a/clang/test/CodeGen/attr-target-clones.c b/clang/test/CodeGen/attr-target-clones.c
index 98ffea40f56d887..0b7c44f26acc6b8 100644
--- a/clang/test/CodeGen/attr-target-clones.c
+++ b/clang/test/CodeGen/attr-target-clones.c
@@ -16,13 +16,22 @@
 // LINUX: @__cpu_model = external dso_local global { i32, i32, i32, [1 x i32] }
 // LINUX: @__cpu_features2 = external dso_local global [3 x i32]
 
-// LINUX: @internal.ifunc = internal ifunc i32 (), ptr @internal.resolver
-// LINUX: @foo.ifunc = weak_odr ifunc i32 (), ptr @foo.resolver
-// LINUX: @foo_dupes.ifunc = weak_odr ifunc void (), ptr @foo_dupes.resolver
-// LINUX: @unused.ifunc = weak_odr ifunc void (), ptr @unused.resolver
-// LINUX: @foo_inline.ifunc = weak_odr ifunc i32 (), ptr @foo_inline.resolver
-// LINUX: @foo_inline2.ifunc = weak_odr ifunc i32 (), ptr @foo_inline2.resolver
-// LINUX: @foo_used_no_defn.ifunc = weak_odr ifunc i32 (), ptr @foo_used_no_defn.resolver
+// LINUX: @internal.ifunc = weak_odr alias i32 (), ptr @internal
+// LINUX: @foo.ifunc = weak_odr alias i32 (), ptr @foo
+// LINUX: @foo_dupes.ifunc = weak_odr alias void (), ptr @foo_dupes
+// LINUX: @unused.ifunc = weak_odr alias void (), ptr @unused
+// LINUX: @foo_inline.ifunc = weak_odr alias i32 (), ptr @foo_inline
+// LINUX: @foo_inline2.ifunc = weak_odr alias i32 (), ptr @foo_inline2
+// LINUX: @foo_used_no_defn.ifunc = weak_odr alias i32 (), ptr @foo_used_no_defn
+// LINUX: @isa_level.ifunc = weak_odr alias i32 (i32), ptr @isa_level
+
+// LINUX: @internal = internal ifunc i32 (), ptr @internal.resolver
+// LINUX: @foo = weak_odr ifunc i32 (), ptr @foo.resolver
+// LINUX: @foo_dupes = weak_odr ifunc void (), ptr @foo_dupes.resolver
+// LINUX: @unused = weak_odr ifunc void (), ptr @unused.resolver
+// LINUX: @foo_inline = weak_odr ifunc i32 (), ptr @foo_inline.resolver
+// LINUX: @foo_inline2 = weak_odr ifunc i32 (), ptr @foo_inline2.resolver
+// LINUX: @foo_used_no_defn = weak_odr ifunc i32 (), ptr @foo_used_no_defn.resolver
 
 static int __attribute__((target_clones("sse4.2, default"))) internal(void) { return 0; }
 int use(void) { return internal(); }
@@ -60,7 +69,7 @@ void bar2(void) {
   // LINUX: define {{.*}}void @bar2()
   // WINDOWS: define dso_local void @bar2()
   foo_dupes();
-  // LINUX: call void @foo_dupes.ifunc()
+  // LINUX: call void @foo_dupes()
   // WINDOWS: call void @foo_dupes()
 }
 
@@ -68,7 +77,7 @@ int bar(void) {
   // LINUX: define {{.*}}i32 @bar() #[[DEF:[0-9]+]]
   // WINDOWS: define dso_local i32 @bar() #[[DEF:[0-9]+]]
   return foo();
-  // LINUX: call i32 @foo.ifunc()
+  // LINUX: call i32 @foo()
   // WINDOWS: call i32 @foo()
 }
 
@@ -95,8 +104,8 @@ int bar3(void) {
   // LINUX: define {{.*}}i32 @bar3()
   // WINDOWS: define dso_local i32 @bar3()
   return foo_inline() + foo_inline2();
-  // LINUX: call i32 @foo_inline.ifunc()
-  // LINUX: call i32 @foo_inline2.ifunc()
+  // LINUX: call i32 @foo_inline()
+  // LINUX: call i32 @foo_inline2()
   // WINDOWS: call i32 @foo_inline()
   // WINDOWS: call i32 @foo_inline2()
 }
@@ -134,7 +143,7 @@ int test_foo_used_no_defn(void) {
   // LINUX: define {{.*}}i32 @test_foo_used_no_defn()
   // WINDOWS: define dso_local i32 @test_foo_used_no_defn()
   return foo_used_no_defn();
-  // LINUX: call i32 @foo_used_no_defn.ifunc()
+  // LINUX: call i32 @foo_used_no_defn()
   // WINDOWS: call i32 @foo_used_no_defn()
 }
 
diff --git a/clang/test/CodeGenCXX/attr-target-clones.cpp b/clang/test/CodeGenCXX/attr-target-clones.cpp
index 86293b98dbbd35f..fd2d38062a71e51 100644
--- a/clang/test/CodeGenCXX/attr-target-clones.cpp
+++ b/clang/test/CodeGenCXX/attr-target-clones.cpp
@@ -1,13 +1,20 @@
 // RUN: %clang_cc1 -std=c++11 -triple x86_64-linux-gnu -emit-llvm %s -o - | FileCheck %s --check-prefix=LINUX
 // RUN: %clang_cc1 -std=c++11 -triple x86_64-windows-pc -emit-llvm %s -o - | FileCheck %s --check-prefix=WINDOWS
 
+// Aliases for ifuncs
+// LINUX: @_Z10overloadedi.ifunc = weak_odr alias i32 (i32), ptr @_Z10overloadedi
+// LINUX: @_Z10overloadedPKc.ifunc = weak_odr alias i32 (ptr), ptr @_Z10overloadedPKc
+// LINUX: @_ZN1CIssE3fooEv.ifunc = weak_odr alias i32 (ptr), ptr @_ZN1CIssE3fooEv
+// LINUX: @_ZN1CIisE3fooEv.ifunc = weak_odr alias i32 (ptr), ptr @_ZN1CIisE3fooEv
+// LINUX: @_ZN1CIdfE3fooEv.ifunc = weak_odr alias i32 (ptr), ptr @_ZN1CIdfE3fooEv
+
 // Overloaded ifuncs
-// LINUX: @_Z10overloadedi.ifunc = weak_odr ifunc i32 (i32), ptr @_Z10overloadedi.resolver
-// LINUX: @_Z10overloadedPKc.ifunc = weak_odr ifunc i32 (ptr), ptr @_Z10overloadedPKc.resolver
+// LINUX: @_Z10overloadedi = weak_odr ifunc i32 (i32), ptr @_Z10overloadedi.resolver
+// LINUX: @_Z10overloadedPKc = weak_odr ifunc i32 (ptr), ptr @_Z10overloadedPKc.resolver
 // struct 'C' ifuncs, note the 'float, U' one doesn't get one.
-// LINUX: @_ZN1CIssE3fooEv.ifunc = weak_odr ifunc i32 (ptr), ptr @_ZN1CIssE3fooEv.resolver
-// LINUX: @_ZN1CIisE3fooEv.ifunc = weak_odr ifunc i32 (ptr), ptr @_ZN1CIisE3fooEv.resolver
-// LINUX: @_ZN1CIdfE3fooEv.ifunc = weak_odr ifunc i32 (ptr), ptr @_ZN1CIdfE3fooEv.resolver
+// LINUX: @_ZN1CIssE3fooEv = weak_odr ifunc i32 (ptr), ptr @_ZN1CIssE3fooEv.resolver
+// LINUX: @_ZN1CIisE3fooEv = weak_odr ifunc i32 (ptr), ptr @_ZN1CIisE3fooEv.resolver
+// LINUX: @_ZN1CIdfE3fooEv = weak_odr ifunc i32 (ptr), ptr @_ZN1CIdfE3fooEv.resolver
 
 int __attribute__((target_clones("sse4.2", "default"))) overloaded(int) { return 1; }
 // LINUX: define {{.*}}i32 @_Z10overloadedi.sse4.2.0(i32{{.+}})
@@ -37,10 +44,10 @@ int __attribute__((target_clones("arch=ivybridge", "default"))) overloaded(const
 
 void use_overloaded() {
   overloaded(1);
-  // LINUX: call noundef i32 @_Z10overloadedi.ifunc
+  // LINUX: call noundef i32 @_Z10overloadedi
   // WINDOWS: call noundef i32 @"?overloaded@@YAHH@Z"
   overloaded(nullptr);
-  // LINUX: call noundef i32 @_Z10overloadedPKc.ifunc 
+  // LINUX: call noundef i32 @_Z10overloadedPKc 
   // WINDOWS: call noundef i32 @"?overloaded@@YAHPEBD@Z"
 }
 
@@ -64,11 +71,11 @@ int __attribute__((target_clones("sse4.2", "default"))) foo(){ return 3;}
 void uses_specialized() {
   C<short, short> c;
   c.foo();
-  // LINUX: call noundef i32 @_ZN1CIssE3fooEv.ifunc(ptr
+  // LINUX: call noundef i32 @_ZN1CIssE3fooEv(ptr
   // WINDOWS: call noundef i32 @"?foo@?$C@FF@@QEAAHXZ"(ptr
   C<int, short> c2;
   c2.foo();
-  // LINUX: call noundef i32 @_ZN1CIisE3fooEv.ifunc(ptr
+  // LINUX: call noundef i32 @_ZN1CIisE3fooEv(ptr
   // WINDOWS: call noundef i32 @"?foo@?$C@HF@@QEAAHXZ"(ptr
   C<float, short> c3;
   c3.foo();
@@ -77,7 +84,7 @@ void uses_specialized() {
   // WINDOWS: call noundef i32 @"?foo@?$C@MF@@QEAAHXZ"(ptr
   C<double, float> c4;
   c4.foo();
-  // LINUX: call noundef i32 @_ZN1CIdfE3fooEv.ifunc(ptr
+  // LINUX: call noundef i32 @_ZN1CIdfE3fooEv(ptr
   // WINDOWS: call noundef i32 @"?foo@?$C@NM@@QEAAHXZ"(ptr
 }
 

Copy link
Collaborator

@efriedma-quic efriedma-quic left a comment

Choose a reason for hiding this comment

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

Added reviewers from https://reviews.llvm.org/D127812, for the AArch64 side.

clang/lib/CodeGen/CodeGenModule.cpp Outdated Show resolved Hide resolved
@elizabethandrews
Copy link
Contributor Author

I ran some tests compiling TU1 and TU2 in various combinations of clang17, trunk and g++.

TU1.cpp

int foo();
int main()
{   return foo(); }

TU2.cpp

#include<iostream>

__attribute__((target_clones("default", "arch=sapphirerapids")))
int foo() {
  std::cout<<"Hello World!";
  return 0;
}

TU1 - Clang Trunk ; TU2 - Clang 17 : Continue to have link error as expected
TU1 - Clang 17 ; TU2 - Clang Trunk : Passes
TU1 – g++, TU2 – Clang Trunk : Passes
TU1 – clang Trunk TU2 – G++ : Passes with -fPIE

@elizabethandrews
Copy link
Contributor Author

ping

Copy link
Collaborator

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

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

This seems fine to me, but I think @efriedma-quic /the weak ODR discussion needs to be finalized before this can be accepted.

@DanielKristofKiss
Copy link
Member

I proposed a change to the aarch64 spec to help the harmonisation.
ARM-software/acle#277

I'd drop the aarch64 specifics. I have a few more lines to make it work on aarch64 (e.g. AppendTargetVersionMangling and AppendTargetClonesMangling needs do the the .default mangling )

@efriedma-quic
Copy link
Collaborator

efriedma-quic commented Nov 21, 2023

@DanielKristofKiss From your response, I gather you want AArch64 to align with the other targets, but additional changes are required to make that work?

In the interest of keeping things moving, how do you want to stage this? Would it make sense to land this as-is? Then you can remove the aarch64-specific codepath along with whatever other changes are necessary.

@DanielKristofKiss
Copy link
Member

@efriedma-quic Sounds good to me, I'll land those change top of this PR and drop these todos.

Copy link
Contributor

@tahonermann tahonermann left a comment

Choose a reason for hiding this comment

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

I requested some minor changes.

Can we document the .ifunc symbols as a deprecated feature? With this change, they will never be referenced except by code compiled by older compiler versions. Maybe plan to deprecate them a year from now?

It looks like there is a related issue in which multiple ifunc symbols are emitted for the cpu_dispatch attribute. See https://godbolt.org/z/71vr8ceza. The relevant symbols emitted are listed below. Note that both _Z12cpu_specificv and _Z12cpu_specificv.ifunc are "i" symbols with the same address. The caller in this case calls the .ifunc symbol (just as for target_clones prior to this change). It would be nice if we can fix this issue at the same time and likewise deprecate the .ifunc symbol for cpu_dispatch/cpu_specific.

00000000000024c0 i _Z12cpu_specificv
0000000000002480 T _Z12cpu_specificv.A
0000000000002490 T _Z12cpu_specificv.M
00000000000024c0 i _Z12cpu_specificv.ifunc
00000000000024c0 W _Z12cpu_specificv.resolver

I think the only time a symbol with a .ifunc suffix is actually needed is when the target attribute is used in an overloading context (since in that situation, the target(default) definition gets the non-suffixed name.

clang/docs/ReleaseNotes.rst Outdated Show resolved Hide resolved
clang/lib/CodeGen/CodeGenModule.cpp Outdated Show resolved Hide resolved
Comment on lines +4119 to +4124
// 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?
Copy link
Contributor

Choose a reason for hiding this comment

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

@DanielKristofKiss, please ensure this comment and FIXME is addressed/removed by your follow up patch.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have been meaning to file an issue about this. Not using a different mangling means you always get the default version when referencing a multi-versioned function outside of the TU.

Consider e.g:

PublicHeader.h:
 
int versioned(void);
 
void *inside_TU();
 
void *outside_TU();
 
---
 
TUA.c:
 
#include "PublicHeader.h"
 
__attribute__((target_version("simd")))
int versioned(void) { return 1; }
 
__attribute__((target_version("default")))
int versioned(void) { return 2; }
 
void *inside_TU(void) { return versioned; }
 
---
 
TUB.c:
 
#include "PublicHeader.h"
 
void *outside_TU(void) { return versioned; }
 
---
 
Check.c:
 
#include "PublicHeader.h"
 
int main() { return inside_TU() == outside_TU(); }

@rsandifo-arm brought up a similar case on x86:

__attribute__((target("avx2")))
int versioned(void) { return 1; }
 
__attribute__((target("default")))
int versioned(void) { return 2; }
 
int (*inside_TU(void))(void) { return versioned; }

If we fix this, we should definitely make sure both the ACLE folks, and GCC are on board, and that the fix makes sense in the context of other targets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://gcc.gnu.org/wiki/FunctionMultiVersioning says "The only exception to this is the default version tagged with target attribute string "default". The default version retains the original assembler name and is not changed" Looks like this was intentional for some reason.

It is interesting that GCC changed mangling rules for target_clones attribute though. The default versions do have a .default suffix

Copy link
Contributor

Choose a reason for hiding this comment

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

Since target_version is new, I think it is reasonable to change its behavior (via a different github PR).

I suspect the target behavior is an artifact of how the multiversion function features evolved. My best guess is that the original implementation didn't support an indirect function facility that enabled dynamic resolution from outside a defining TU. Perhaps that was part of the motivation for adding target_clones.

I just discovered that gcc doesn't support the target attribute when compiling for C. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108964. I had been under the impression that it just didn't support overloading on the target attribute (but that the attribute still affected code generation). Interesting that no warning is given for an ignored attribute. Here is the example from the doc Elizabeth linked compiling as C. https://godbolt.org/z/asfrhG51G.

Comment on lines +4304 to +4309
// 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?
Copy link
Contributor

Choose a reason for hiding this comment

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

@DanielKristofKiss, please ensure this comment and FIXME is addressed/removed by your follow up patch.

clang/test/CodeGen/attr-target-clones.c Show resolved Hide resolved
@elizabethandrews
Copy link
Contributor Author

elizabethandrews commented Nov 28, 2023

Thank you for the reviews! I apologize for the delay in my response. I was OOO last week. I will apply review comments and push an update ASAP.

@tahonermann
Copy link
Contributor

@elizabethandrews, PR #73688 seeks to enable ifunc support for Mach-o targets like Darwin. Collaboration might be required to avoid merge conflicts and/or build bot breakage depending on when changes land.

@elizabethandrews
Copy link
Contributor Author

elizabethandrews commented Nov 29, 2023

I requested some minor changes.

Can we document the .ifunc symbols as a deprecated feature? With this change, they will never be referenced except by code compiled by older compiler versions. Maybe plan to deprecate them a year from now?

How/where do I document this?

It looks like there is a related issue in which multiple ifunc symbols are emitted for the cpu_dispatch attribute. See https://godbolt.org/z/71vr8ceza. The relevant symbols emitted are listed below. Note that both _Z12cpu_specificv and _Z12cpu_specificv.ifunc are "i" symbols with the same address. The caller in this case calls the .ifunc symbol (just as for target_clones prior to this change). It would be nice if we can fix this issue at the same time and likewise deprecate the .ifunc symbol for cpu_dispatch/cpu_specific.

00000000000024c0 i _Z12cpu_specificv
0000000000002480 T _Z12cpu_specificv.A
0000000000002490 T _Z12cpu_specificv.M
00000000000024c0 i _Z12cpu_specificv.ifunc
00000000000024c0 W _Z12cpu_specificv.resolver

I think the only time a symbol with a .ifunc suffix is actually needed is when the target attribute is used in an overloading context (since in that situation, the target(default) definition gets the non-suffixed name.

The symbols correspond to the ifunc and it's alias. Mangling rules seem to vary between different MV attributes. For example - for target attribute, the default versions do not include a .default suffix. I do not know what the mangling requirements for cpu_dispatch are off the top of my head. I do not think we should change it to match target_clones attribute without confirming this is the correct behavior for this attribute.

@llvmbot llvmbot added the clang:frontend Language frontend issues, e.g. anything involving "Sema" label Dec 1, 2023
@elizabethandrews
Copy link
Contributor Author

I requested some minor changes.
Can we document the .ifunc symbols as a deprecated feature? With this change, they will never be referenced except by code compiled by older compiler versions. Maybe plan to deprecate them a year from now?

How/where do I document this?

It looks like there is a related issue in which multiple ifunc symbols are emitted for the cpu_dispatch attribute. See https://godbolt.org/z/71vr8ceza. The relevant symbols emitted are listed below. Note that both _Z12cpu_specificv and _Z12cpu_specificv.ifunc are "i" symbols with the same address. The caller in this case calls the .ifunc symbol (just as for target_clones prior to this change). It would be nice if we can fix this issue at the same time and likewise deprecate the .ifunc symbol for cpu_dispatch/cpu_specific.

00000000000024c0 i _Z12cpu_specificv
0000000000002480 T _Z12cpu_specificv.A
0000000000002490 T _Z12cpu_specificv.M
00000000000024c0 i _Z12cpu_specificv.ifunc
00000000000024c0 W _Z12cpu_specificv.resolver

I think the only time a symbol with a .ifunc suffix is actually needed is when the target attribute is used in an overloading context (since in that situation, the target(default) definition gets the non-suffixed name.

The symbols correspond to the ifunc and it's alias. Mangling rules seem to vary between different MV attributes. For example - for target attribute, the default versions do not include a .default suffix. I do not know what the mangling requirements for cpu_dispatch are off the top of my head. I do not think we should change it to match target_clones attribute without confirming this is the correct behavior for this attribute.

@tahonermann as discussed offline, I documented the deprecation in attribute documentation. I wasn't 100% certain of what to say, so please feel free to wordsmith! As also discussed, I will make changes to cpu_dispatch in a follow-up PR.

@elizabethandrews
Copy link
Contributor Author

Hmm.... the build fails with:

⚠️ Warning: Checkout failed! checking out commit "af600cbf98ce1bf55c51ef88ddf94cd9114181c2": exit status 128 (Attempt 3/3)
🚨 Error: checking out commit "af600cbf98ce1bf55c51ef88ddf94cd9114181c2": exit status 128

Would anyone happen to know what is happening? All I did was git fetch upstream, where upstream is https://github.com/llvm/llvm-project.git and merged upstream/main to this PR branch

@DanielKristofKiss
Copy link
Member

Hmm.... the build fails with:

⚠️ Warning: Checkout failed! checking out commit "af600cbf98ce1bf55c51ef88ddf94cd9114181c2": exit status 128 (Attempt 3/3) 🚨 Error: checking out commit "af600cbf98ce1bf55c51ef88ddf94cd9114181c2": exit status 128

Would anyone happen to know what is happening? All I did was git fetch upstream, where upstream is https://github.com/llvm/llvm-project.git and merged upstream/main to this PR branch

Seems just some glitch, #74358 which contains these patches everything worked.

@elizabethandrews
Copy link
Contributor Author

Hmm.... the build fails with:
⚠️ Warning: Checkout failed! checking out commit "af600cbf98ce1bf55c51ef88ddf94cd9114181c2": exit status 128 (Attempt 3/3) 🚨 Error: checking out commit "af600cbf98ce1bf55c51ef88ddf94cd9114181c2": exit status 128
Would anyone happen to know what is happening? All I did was git fetch upstream, where upstream is https://github.com/llvm/llvm-project.git and merged upstream/main to this PR branch

Seems just some glitch, #74358 which contains these patches everything worked.

Ok Thank you for letting me know! I will make the comment change you mentioned and update PR.

Copy link
Contributor

@tahonermann tahonermann left a comment

Choose a reason for hiding this comment

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

This looks good to me. I suggested some edits to the doc; take or leave as you see fit!

clang/include/clang/Basic/AttrDocs.td Outdated Show resolved Hide resolved
Co-authored-by: Tom Honermann <tom@honermann.net>
@elizabethandrews
Copy link
Contributor Author

Thanks for reviews everyone! I think I have addressed all pending comments. I will merge the PR in a couple of hours unless someone has remaining concerns.

@elizabethandrews elizabethandrews merged commit cee5b87 into llvm:main Dec 5, 2023
5 checks passed
DanielKristofKiss added a commit to DanielKristofKiss/llvm-project that referenced this pull request Dec 6, 2023
AArch64 part of llvm#71706.

Default version is now mangled with .default.
Resolver for the TargetVersion need to be emitted from the
CodeGenModule::EmitMultiVersionFunctionDefinition.
jroelofs pushed a commit to apple/llvm-project that referenced this pull request Jan 10, 2024
Currently target_clones attribute results in a linker error when there
are no multi-versioned function declarations in the calling TU.

In the calling TU, the call is generated with the ‘normal’ assembly
name. This does not match any of the versions or the ifunc, since
version mangling includes a .versionstring, and the ifunc includes
.ifunc suffix. The linker error is not seen with GCC since the mangling
for the ifunc symbol in GCC is the ‘normal’ assembly name for function
i.e. no ifunc suffix.

This PR removes the .ifunc suffix to match GCC. It also adds alias with
the .ifunc suffix so as to ensure backward compatibility.

The changes exclude aarch64 target because the mangling for default
versions on aarch64 does not include a .default suffix and is the
'normal' assembly name, unlike other targets. It is not clear to me what
the correct behavior for this target is.

Old Phabricator review - https://reviews.llvm.org/D158666

---------

Co-authored-by: Tom Honermann <tom@honermann.net>
jroelofs pushed a commit to apple/llvm-project that referenced this pull request Jan 10, 2024
Currently target_clones attribute results in a linker error when there
are no multi-versioned function declarations in the calling TU.

In the calling TU, the call is generated with the ‘normal’ assembly
name. This does not match any of the versions or the ifunc, since
version mangling includes a .versionstring, and the ifunc includes
.ifunc suffix. The linker error is not seen with GCC since the mangling
for the ifunc symbol in GCC is the ‘normal’ assembly name for function
i.e. no ifunc suffix.

This PR removes the .ifunc suffix to match GCC. It also adds alias with
the .ifunc suffix so as to ensure backward compatibility.

The changes exclude aarch64 target because the mangling for default
versions on aarch64 does not include a .default suffix and is the
'normal' assembly name, unlike other targets. It is not clear to me what
the correct behavior for this target is.

Old Phabricator review - https://reviews.llvm.org/D158666

---------

Co-authored-by: Tom Honermann <tom@honermann.net>
DanielKristofKiss added a commit that referenced this pull request Jan 22, 2024
AArch64 part of #71706.

Default version is now mangled with .default.
Resolver for the TargetVersion need to be emitted from the
CodeGenModule::EmitMultiVersionFunctionDefinition.
jroelofs pushed a commit to apple/llvm-project that referenced this pull request Jan 22, 2024
…74358)

AArch64 part of llvm#71706.

Default version is now mangled with .default.
Resolver for the TargetVersion need to be emitted from the
CodeGenModule::EmitMultiVersionFunctionDefinition.
jroelofs pushed a commit to apple/llvm-project that referenced this pull request Jan 23, 2024
…74358)

AArch64 part of llvm#71706.

Default version is now mangled with .default.
Resolver for the TargetVersion need to be emitted from the
CodeGenModule::EmitMultiVersionFunctionDefinition.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:codegen clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants