[Clang][FMV] Stop emitting implicit default version using target_clones. (#141808)

With the current behavior the following example yields a linker error:
"multiple definition of `foo.default'"

// Translation Unit 1
__attribute__((target_clones("dotprod, sve"))) int foo(void) { return 1; }

// Translation Unit 2
int foo(void) { return 0; }
__attribute__((target_version("dotprod"))) int foo(void);
__attribute__((target_version("sve"))) int foo(void);
int bar(void) { return foo(); }

That is because foo.default is generated twice. As a user I don't find
this particularly intuitive. If I wanted the default to be generated in
TU1 I'd rather write target_clones("dotprod, sve", "default")
explicitly.

When changing the code I noticed that the RISC-V target defers the
resolver emission when encountering a target_version definition. This
seems accidental since it only makes sense for AArch64, where we only
emit a resolver once we've processed the entire TU, and only if the
default version is present. I've changed this so that RISC-V immediately
emmits the resolver. I adjusted the codegen tests since the functions
now appear in a different order.

Implements ARM-software/acle#377

Author: labrinea

clang/lib/CodeGen/CodeGenModule.cpp

@@ -4237,19 +4237,19 @@ void CodeGenModule::EmitMultiVersionFunctionDefinition(GlobalDecl GD,
       EmitGlobalFunctionDefinition(GD.getWithMultiVersionIndex(I), nullptr);
   } else if (auto *TC = FD->getAttr<TargetClonesAttr>()) {
     for (unsigned I = 0; I < TC->featuresStrs_size(); ++I)
-      // AArch64 favors the default target version over the clone if any.
-      if ((!TC->isDefaultVersion(I) || !getTarget().getTriple().isAArch64()) &&
-          TC->isFirstOfVersion(I))
+      if (TC->isFirstOfVersion(I))
         EmitGlobalFunctionDefinition(GD.getWithMultiVersionIndex(I), nullptr);
-    // Ensure that the resolver function is also emitted.
-    GetOrCreateMultiVersionResolver(GD);
   } else
     EmitGlobalFunctionDefinition(GD, GV);
 
-  // Defer the resolver emission until we can reason whether the TU
-  // contains a default target version implementation.
-  if (FD->isTargetVersionMultiVersion())
-    AddDeferredMultiVersionResolverToEmit(GD);
+  // Ensure that the resolver function is also emitted.
+  if (FD->isTargetVersionMultiVersion() || FD->isTargetClonesMultiVersion()) {
+    // On AArch64 defer the resolver emission until the entire TU is processed.
+    if (getTarget().getTriple().isAArch64())
+      AddDeferredMultiVersionResolverToEmit(GD);
+    else
+      GetOrCreateMultiVersionResolver(GD);
+  }
 }
 
 void CodeGenModule::EmitGlobalDefinition(GlobalDecl GD, llvm::GlobalValue *GV) {
@@ -4351,7 +4351,7 @@ void CodeGenModule::emitMultiVersionFunctions() {
     };
 
     // For AArch64, a resolver is only emitted if a function marked with
-    // target_version("default")) or target_clones() is present and defined
+    // target_version("default")) or target_clones("default") is defined
     // in this TU. For other architectures it is always emitted.
     bool ShouldEmitResolver = !getTarget().getTriple().isAArch64();
     SmallVector<CodeGenFunction::FMVResolverOption, 10> Options;
@@ -4374,12 +4374,11 @@ void CodeGenModule::emitMultiVersionFunctions() {
             TVA->getFeatures(Feats, Delim);
             Options.emplace_back(Func, Feats);
           } else if (const auto *TC = CurFD->getAttr<TargetClonesAttr>()) {
-            if (IsDefined)
-              ShouldEmitResolver = true;
             for (unsigned I = 0; I < TC->featuresStrs_size(); ++I) {
               if (!TC->isFirstOfVersion(I))
                 continue;
-
+              if (TC->isDefaultVersion(I) && IsDefined)
+                ShouldEmitResolver = true;
               llvm::Function *Func = createFunction(CurFD, I);
               Feats.clear();
               if (getTarget().getTriple().isX86()) {

clang/lib/Sema/SemaDeclAttr.cpp

@@ -3495,13 +3495,7 @@ static void handleTargetClonesAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   if (HasCommas && AL.getNumArgs() > 1)
     S.Diag(AL.getLoc(), diag::warn_target_clone_mixed_values);
 
-  if (S.Context.getTargetInfo().getTriple().isAArch64() && !HasDefault) {
-    // Add default attribute if there is no one
-    HasDefault = true;
-    Strings.push_back("default");
-  }
-
-  if (!HasDefault) {
+  if (!HasDefault && !S.Context.getTargetInfo().getTriple().isAArch64()) {
     S.Diag(AL.getLoc(), diag::err_target_clone_must_have_default);
     return;
   }

clang/test/AST/attr-target-version.c

@@ -1,7 +1,7 @@
 // RUN: %clang_cc1 -triple aarch64-linux-gnu -ast-dump %s | FileCheck %s
 
 int __attribute__((target_version("sve2-bitperm + sha2"))) foov(void) { return 1; }
-int __attribute__((target_clones(" lse + fp + sha3 "))) fooc(void) { return 2; }
+int __attribute__((target_clones(" lse + fp + sha3 ", "default"))) fooc(void) { return 2; }
 // CHECK: TargetVersionAttr
 // CHECK: sve2-bitperm + sha2
 // CHECK: TargetClonesAttr

clang/test/CodeGen/AArch64/fmv-detection.c

@@ -0,0 +1,24 @@
+// RUN: %clang_cc1 -triple aarch64-linux-gnu -verify -emit-llvm-only %s -DCHECK_IMPLICIT_DEFAULT
+// RUN: %clang_cc1 -triple aarch64-linux-gnu -verify -emit-llvm-only %s -DCHECK_EXPLICIT_DEFAULT
+
+#if defined(CHECK_IMPLICIT_DEFAULT)
+
+int implicit_default_ok(void) { return 0; }
+__attribute__((target_clones("aes", "lse"))) int implicit_default_ok(void) { return 1; }
+
+int implicit_default_bad(void) { return 0; }
+// expected-error@+2 {{definition with same mangled name 'implicit_default_bad.default' as another definition}}
+// expected-note@-2 {{previous definition is here}}
+__attribute__((target_clones("aes", "lse", "default"))) int implicit_default_bad(void) { return 1; }
+
+#elif defined(CHECK_EXPLICIT_DEFAULT)
+
+__attribute__((target_version("default"))) int explicit_default_ok(void) { return 0; }
+__attribute__((target_clones("aes", "lse"))) int explicit_default_ok(void) { return 1; }
+
+__attribute__((target_version("default"))) int explicit_default_bad(void) { return 0; }
+// expected-error@+2 {{definition with same mangled name 'explicit_default_bad.default' as another definition}}
+// expected-note@-2 {{previous definition is here}}
+__attribute__((target_clones("aes", "lse", "default"))) int explicit_default_bad(void) { return 1; }
+
+#endif

clang/test/CodeGen/AArch64/fmv-duplicate-mangled-name.c

@@ -140,7 +140,7 @@ __attribute__((target_version("crc+bti+bti+bti+aes+aes+bf16"))) int fmv(void) {
 __attribute__((target_version("non_existent_extension"))) int fmv(void);
 
 // CHECK: define dso_local i32 @fmv.default() #[[default:[0-9]+]] {
-__attribute__((target_version("default"))) int fmv(void);
+__attribute__((target_version("default"))) int fmv(void) { return 0; }
 
 int caller() {
   return fmv();

clang/test/CodeGen/AArch64/fmv-features.c

@@ -68,6 +68,14 @@ inline __attribute__((target_version("default"))) void linkonce_func(void) {}
 void call_linkonce(void) { linkonce_func(); }
 
 
+// Test that an ifunc is generated when the clones attribute has a default version.
+__attribute__((target_clones("default", "aes"))) void clones_with_default(void) {}
+
+
+// Test that an ifunc is NOT generated when the clones attribute does not have a default version.
+__attribute__((target_clones("aes"))) void clones_without_default(void) {}
+
+
 //.
 // CHECK: @__aarch64_cpu_features = external dso_local global { i64 }
 // CHECK: @used_before_default_def = weak_odr ifunc void (), ptr @used_before_default_def.resolver
@@ -76,6 +84,7 @@ void call_linkonce(void) { linkonce_func(); }
 // CHECK: @indirect_use = weak_odr ifunc void (), ptr @indirect_use.resolver
 // CHECK: @internal_func = internal ifunc void (), ptr @internal_func.resolver
 // CHECK: @linkonce_func = weak_odr ifunc void (), ptr @linkonce_func.resolver
+// CHECK: @clones_with_default = weak_odr ifunc void (), ptr @clones_with_default.resolver
 //.
 // CHECK: Function Attrs: noinline nounwind optnone
 // CHECK-LABEL: define {{[^@]+}}@used_before_default_def._Maes
@@ -228,6 +237,27 @@ void call_linkonce(void) { linkonce_func(); }
 // CHECK-NEXT:    ret void
 //
 //
+// CHECK: Function Attrs: noinline nounwind optnone
+// CHECK-LABEL: define {{[^@]+}}@clones_with_default.default
+// CHECK-SAME: () #[[ATTR2]] {
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    ret void
+//
+//
+// CHECK: Function Attrs: noinline nounwind optnone
+// CHECK-LABEL: define {{[^@]+}}@clones_with_default._Maes
+// CHECK-SAME: () #[[ATTR0]] {
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    ret void
+//
+//
+// CHECK: Function Attrs: noinline nounwind optnone
+// CHECK-LABEL: define {{[^@]+}}@clones_without_default._Maes
+// CHECK-SAME: () #[[ATTR0]] {
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    ret void
+//
+//
 // CHECK-LABEL: define {{[^@]+}}@used_before_default_def.resolver() comdat {
 // CHECK-NEXT:  resolver_entry:
 // CHECK-NEXT:    call void @__init_cpu_features_resolver()
@@ -339,6 +369,20 @@ void call_linkonce(void) { linkonce_func(); }
 // CHECK:       resolver_else:
 // CHECK-NEXT:    ret ptr @linkonce_func.default
 //
+//
+// CHECK-LABEL: define {{[^@]+}}@clones_with_default.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]], 33536
+// CHECK-NEXT:    [[TMP2:%.*]] = icmp eq i64 [[TMP1]], 33536
+// 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 @clones_with_default._Maes
+// CHECK:       resolver_else:
+// CHECK-NEXT:    ret ptr @clones_with_default.default
+//
 //.
 // CHECK: [[META0:![0-9]+]] = !{i32 1, !"wchar_size", i32 4}
 // CHECK: [[META1:![0-9]+]] = !{!"{{.*}}clang version {{.*}}"}