Skip to content

[clang][FMV] Allow declaration of function versions in namespaces. #93044

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

Merged
merged 1 commit into from
May 23, 2024

Conversation

labrinea
Copy link
Collaborator

@labrinea labrinea commented May 22, 2024

Fixes the following bug:

namespace Name {
int __attribute((target_version("default"))) foo() { return 0; }
}

namespace Name {
int __attribute((target_version("sve"))) foo() { return 1; }
}

int bar() { return Name::foo(); }

error: redefinition of 'foo'
int __attribute((target_version("sve"))) foo() { return 1; }

note: previous definition is here
int __attribute((target_version("default"))) foo() { return 0; }

While fixing this I also found that in the absence of default version
declaration, the one we implicitly create has incorrect mangling if
we are in a namespace:

namespace OtherName {
int __attribute((target_version("sve"))) foo() { return 2; }
}

int baz() { return OtherName::foo(); }

In this example instead of creating a declaration for the symbol
@_ZN9OtherName3fooEv.default we are creating one for the symbol
@_Z3foov.default (the namespace mangling prefix is omitted).
This has now been fixed.

@labrinea labrinea requested a review from ilinpv May 22, 2024 15:10
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels May 22, 2024
@labrinea labrinea requested a review from jroelofs May 22, 2024 15:10
@llvmbot
Copy link
Member

llvmbot commented May 22, 2024

@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: Alexandros Lamprineas (labrinea)

Changes

Fixes a bug:

../llvm-project/clang/test/Sema/fmv-namespace.cpp:8:18: warning: attribute declaration must precede definition [-Wignored-attributes]

8 | int __attribute((target_version("sve"))) foo() { return 1; }
  |                  ^

../llvm-project/clang/test/Sema/fmv-namespace.cpp:4:46: note:
previous definition is here
4 | int __attribute((target_version("default"))) foo() { return 0; }
| ^
../llvm-project/clang/test/Sema/fmv-namespace.cpp:8:42: error:
redefinition of 'foo'
8 | int __attribute((target_version("sve"))) foo() { return 1; }
| ^
../llvm-project/clang/test/Sema/fmv-namespace.cpp:4:46: note:
previous definition is here
4 | int __attribute((target_version("default"))) foo() { return 0; }
| ^
1 warning and 1 error generated.


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

3 Files Affected:

  • (modified) clang/lib/Sema/SemaDecl.cpp (+4-2)
  • (added) clang/test/CodeGenCXX/fmv-namespace.cpp (+56)
  • (added) clang/test/Sema/fmv-namespace.cpp (+12)
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index b5c3a27ab06e9..297a2feb66cbc 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -11868,8 +11868,10 @@ static bool CheckMultiVersionFunction(Sema &S, FunctionDecl *NewFD,
     return false;
 
   if (!OldDecl || !OldDecl->getAsFunction() ||
-      OldDecl->getDeclContext()->getRedeclContext() !=
-          NewFD->getDeclContext()->getRedeclContext()) {
+      (OldDecl->getDeclContext()->getRedeclContext() !=
+       NewFD->getDeclContext()->getRedeclContext() &&
+       OldDecl->getDeclContext()->getEnclosingNamespaceContext() !=
+       NewFD->getDeclContext()->getEnclosingNamespaceContext())) {
     // If there's no previous declaration, AND this isn't attempting to cause
     // multiversioning, this isn't an error condition.
     if (MVKind == MultiVersionKind::None)
diff --git a/clang/test/CodeGenCXX/fmv-namespace.cpp b/clang/test/CodeGenCXX/fmv-namespace.cpp
new file mode 100644
index 0000000000000..4d56e3be3e264
--- /dev/null
+++ b/clang/test/CodeGenCXX/fmv-namespace.cpp
@@ -0,0 +1,56 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --check-globals all --include-generated-funcs --version 5
+// RUN: %clang_cc1 -triple aarch64-linux-gnu -emit-llvm %s -o - | FileCheck %s
+
+namespace Name {
+int __attribute((target_version("default"))) foo() { return 0; }
+}
+
+namespace Name {
+int __attribute((target_version("sve"))) foo() { return 1; }
+}
+
+int bar() { return Name::foo(); }
+//.
+// CHECK: @__aarch64_cpu_features = external dso_local global { i64 }
+// CHECK: @_ZN4Name3fooEv.ifunc = weak_odr alias i32 (), ptr @_ZN4Name3fooEv
+// CHECK: @_ZN4Name3fooEv = weak_odr ifunc i32 (), ptr @_ZN4Name3fooEv.resolver
+//.
+// CHECK-LABEL: define dso_local noundef i32 @_ZN4Name3fooEv.default(
+// CHECK-SAME: ) #[[ATTR0:[0-9]+]] {
+// CHECK-NEXT:  [[ENTRY:.*:]]
+// CHECK-NEXT:    ret i32 0
+//
+//
+// CHECK-LABEL: define dso_local noundef i32 @_ZN4Name3fooEv._Msve(
+// CHECK-SAME: ) #[[ATTR1:[0-9]+]] {
+// CHECK-NEXT:  [[ENTRY:.*:]]
+// CHECK-NEXT:    ret i32 1
+//
+//
+// CHECK-LABEL: define dso_local noundef i32 @_Z3barv(
+// CHECK-SAME: ) #[[ATTR0]] {
+// CHECK-NEXT:  [[ENTRY:.*:]]
+// CHECK-NEXT:    [[CALL:%.*]] = call noundef i32 @_ZN4Name3fooEv()
+// CHECK-NEXT:    ret i32 [[CALL]]
+//
+//
+// CHECK-LABEL: define weak_odr ptr @_ZN4Name3fooEv.resolver() comdat {
+// CHECK-NEXT:  [[RESOLVER_ENTRY:.*:]]
+// CHECK-NEXT:    call void @__init_cpu_features_resolver()
+// CHECK-NEXT:    [[TMP0:%.*]] = load i64, ptr @__aarch64_cpu_features, align 8
+// CHECK-NEXT:    [[TMP1:%.*]] = and i64 [[TMP0]], 1073741824
+// CHECK-NEXT:    [[TMP2:%.*]] = icmp eq i64 [[TMP1]], 1073741824
+// CHECK-NEXT:    [[TMP3:%.*]] = and i1 true, [[TMP2]]
+// CHECK-NEXT:    br i1 [[TMP3]], label %[[RESOLVER_RETURN:.*]], label %[[RESOLVER_ELSE:.*]]
+// CHECK:       [[RESOLVER_RETURN]]:
+// CHECK-NEXT:    ret ptr @_ZN4Name3fooEv._Msve
+// CHECK:       [[RESOLVER_ELSE]]:
+// CHECK-NEXT:    ret ptr @_ZN4Name3fooEv.default
+//
+//.
+// CHECK: attributes #[[ATTR0]] = { mustprogress noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" }
+// CHECK: attributes #[[ATTR1]] = { mustprogress noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-features"="+fp-armv8,+fullfp16,+neon,+sve" }
+//.
+// CHECK: [[META0:![0-9]+]] = !{i32 1, !"wchar_size", i32 4}
+// CHECK: [[META1:![0-9]+]] = !{!"{{.*}}clang version {{.*}}"}
+//.
diff --git a/clang/test/Sema/fmv-namespace.cpp b/clang/test/Sema/fmv-namespace.cpp
new file mode 100644
index 0000000000000..1c12fd66cf243
--- /dev/null
+++ b/clang/test/Sema/fmv-namespace.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -triple aarch64-linux-gnu  -fsyntax-only -verify %s
+// expected-no-diagnostics
+
+namespace Name {
+int __attribute((target_version("default"))) foo() { return 0; }
+}
+
+namespace Name {
+int __attribute((target_version("sve"))) foo() { return 1; }
+}
+
+int bar() { return Name::foo(); }

Copy link

github-actions bot commented May 22, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@labrinea labrinea force-pushed the fmv-redeclare-in-namespace branch from e03d8cf to e1de85d Compare May 22, 2024 15:23
@labrinea labrinea force-pushed the fmv-redeclare-in-namespace branch from e1de85d to 1eb344d Compare May 22, 2024 18:05
@llvmbot llvmbot added the clang:codegen IR generation bugs: mangling, exceptions, etc. label May 22, 2024
@labrinea labrinea requested a review from jroelofs May 22, 2024 18:07
@labrinea
Copy link
Collaborator Author

labrinea commented May 22, 2024

I found another bug for implicitly declared default versions not having the namespace mangling. Fixed now, please take another look.

@labrinea labrinea force-pushed the fmv-redeclare-in-namespace branch from 1eb344d to 730c8c5 Compare May 23, 2024 08:51
Fixes the following bug:

namespace Name {
int __attribute((target_version("default"))) foo() { return 0; }
}

namespace Name {
int __attribute((target_version("sve"))) foo() { return 1; }
}

int bar() { return Name::foo(); }

error: redefinition of 'foo'
  int __attribute((target_version("sve"))) foo() { return 1; }

note: previous definition is here
  int __attribute((target_version("default"))) foo() { return 0; }

While fixing this I also found that in the absence of default version
declaration, the one we implicitly create has incorrect mangling if
we are in a namespace:

namespace OtherName {
int __attribute((target_version("sve"))) foo() { return 2; }
}

int baz() { return OtherName::foo(); }

In this example instead of creating a declaration for the symbol
@_ZN9OtherName3fooEv.default we are creating one for the symbol
@_Z3foov.default (the namespace mangling prefix is omitted).
This has now been fixed.
@labrinea labrinea force-pushed the fmv-redeclare-in-namespace branch from 730c8c5 to ba2b8b5 Compare May 23, 2024 08:58
@labrinea labrinea merged commit 8930ba9 into llvm:main May 23, 2024
3 checks passed
@labrinea labrinea deleted the fmv-redeclare-in-namespace branch May 23, 2024 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:codegen IR generation bugs: mangling, exceptions, etc. 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.

4 participants