[NFC][Clang] Address reviews about overrideFunctionFeaturesWithTargetFeatures

[jmmartinez]

Addressing remarks after merge of D159257

• Add comment
• Remove irrelevant CHECKs from test
• Simplify function
• Use llvm::sort before setting target-features as it is done in CodeGenModeule

[llvmbot]

@llvm/pr-subscribers-clang

Changes

Addressing remarks after merge of D159257

• Add comment
• Remove irrelevant CHECKs from test
• Simplify function
• Use llvm::sort before setting target-features as it is done in CodeGenModeule
  --
  Full diff: https://github.com/llvm/llvm-project/pull/65938.diff

2 Files Affected:

• (modified) clang/lib/CodeGen/CGCall.cpp (+18-29)
• (modified) clang/test/CodeGen/link-builtin-bitcode.c (+4-4)

diff --git a/clang/lib/CodeGen/CGCall.cpp b/clang/lib/CodeGen/CGCall.cpp
index e15a4634b1d041b..3fe8d0a7cf49e16 100644
--- a/clang/lib/CodeGen/CGCall.cpp
+++ b/clang/lib/CodeGen/CGCall.cpp
@@ -2001,52 +2001,41 @@ static void getTrivialDefaultFunctionAttributes(
   }
 }
 
+/// Merges `target-features` from \TargetOpts and \F, and sets the result in
+/// \FuncAttr
+/// * features from \F are always kept
+/// * a feature from \TargetOpts is kept if itself and its opposite are absent
+/// from \F
 static void
 overrideFunctionFeaturesWithTargetFeatures(llvm::AttrBuilder &FuncAttr,
                                            const llvm::Function &F,
                                            const TargetOptions &TargetOpts) {
   auto FFeatures = F.getFnAttribute("target-features");
 
-  llvm::StringSet<> IncompatibleFeatureNames;
+  llvm::StringSet<> MergedNames;
   SmallVector MergedFeatures;
   MergedFeatures.reserve(TargetOpts.Features.size());
 
-  if (FFeatures.isValid()) {
-    const auto &TFeatures = TargetOpts.FeatureMap;
-    for (StringRef Feature : llvm::split(FFeatures.getValueAsString(), ',')) {
+  auto AddUnmergedFeatures = [&](auto &&FeatureRange) {
+    for (StringRef Feature : FeatureRange) {
       if (Feature.empty())
         continue;
-
-      bool EnabledForFunc = Feature.starts_with("+");
-      assert(EnabledForFunc || Feature.starts_with("-"));
-
+      assert(Feature[0] == '+' || Feature[0] == '-');
       StringRef Name = Feature.drop_front(1);
-      auto TEntry = TFeatures.find(Name);
-
-      // Preserves features that are incompatible (either set to something
-      // different or missing) from the target features
-      bool MissingFromTarget = TEntry == TFeatures.end();
-      bool EnabledForTarget = !MissingFromTarget && TEntry->second;
-      bool Incompatible = EnabledForTarget != EnabledForFunc;
-      if (MissingFromTarget || Incompatible) {
+      bool Merged = !MergedNames.insert(Name).second;
+      if (!Merged)
         MergedFeatures.push_back(Feature);
-        if (Incompatible)
-          IncompatibleFeatureNames.insert(Name);
-      }
     }
-  }
+  };
 
-  for (StringRef Feature : TargetOpts.Features) {
-    if (Feature.empty())
-      continue;
-    StringRef Name = Feature.drop_front(1);
-    if (IncompatibleFeatureNames.contains(Name))
-      continue;
-    MergedFeatures.push_back(Feature);
-  }
+  if (FFeatures.isValid())
+    AddUnmergedFeatures(llvm::split(FFeatures.getValueAsString(), ','));
+  AddUnmergedFeatures(TargetOpts.Features);
 
-  if (!MergedFeatures.empty())
+  if (!MergedFeatures.empty()) {
+    llvm::sort(MergedFeatures);
     FuncAttr.addAttribute("target-features", llvm::join(MergedFeatures, ","));
+  }
 }
 
 void CodeGen::mergeDefaultFunctionDefinitionAttributes(
diff --git a/clang/test/CodeGen/link-builtin-bitcode.c b/clang/test/CodeGen/link-builtin-bitcode.c
index fe60a9746f1c85f..d4818cc28717312 100644
--- a/clang/test/CodeGen/link-builtin-bitcode.c
+++ b/clang/test/CodeGen/link-builtin-bitcode.c
@@ -43,7 +43,7 @@ int bar() { return no_attr() + attr_in_target() + attr_not_in_target() + attr_in
 // CHECK-LABEL: @attr_incompatible
 // CHECK-SAME: () #[[ATTR_INCOMPATIBLE:[0-9]+]] {
 
-// CHECK: attributes #[[ATTR_BAR]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="gfx90a" "target-features"="+16-bit-insts,+atomic-buffer-global-pk-add-f16-insts,+atomic-fadd-rtn-insts,+ci-insts,+dl-insts,+dot1-insts,+dot10-insts,+dot2-insts,+dot3-insts,+dot4-insts,+dot5-insts,+dot6-insts,+dot7-insts,+dpp,+gfx8-insts,+gfx9-insts,+gfx90a-insts,+mai-insts,+s-memrealtime,+s-memtime-inst,+wavefrontsize64" }
-// CHECK: attributes #[[ATTR_COMPATIBLE]] = { convergent noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="gfx90a" "target-features"="+16-bit-insts,+atomic-buffer-global-pk-add-f16-insts,+atomic-fadd-rtn-insts,+ci-insts,+dl-insts,+dot1-insts,+dot10-insts,+dot2-insts,+dot3-insts,+dot4-insts,+dot5-insts,+dot6-insts,+dot7-insts,+dpp,+gfx8-insts,+gfx9-insts,+gfx90a-insts,+gws,+image-insts,+mai-insts,+s-memrealtime,+s-memtime-inst,+wavefrontsize64" }
-// CHECK: attributes #[[ATTR_EXTEND]] = { convergent noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="gfx90a" "target-features"="+extended-image-insts,+16-bit-insts,+atomic-buffer-global-pk-add-f16-insts,+atomic-fadd-rtn-insts,+ci-insts,+dl-insts,+dot1-insts,+dot10-insts,+dot2-insts,+dot3-insts,+dot4-insts,+dot5-insts,+dot6-insts,+dot7-insts,+dpp,+gfx8-insts,+gfx9-insts,+gfx90a-insts,+gws,+image-insts,+mai-insts,+s-memrealtime,+s-memtime-inst,+wavefrontsize64" }
-// CHECK: attributes #[[ATTR_INCOMPATIBLE]] = { convergent noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="gfx90a" "target-features"="-gfx9-insts,+16-bit-insts,+atomic-buffer-global-pk-add-f16-insts,+atomic-fadd-rtn-insts,+ci-insts,+dl-insts,+dot1-insts,+dot10-insts,+dot2-insts,+dot3-insts,+dot4-insts,+dot5-insts,+dot6-insts,+dot7-insts,+dpp,+gfx8-insts,+gfx90a-insts,+gws,+image-insts,+mai-insts,+s-memrealtime,+s-memtime-inst,+wavefrontsize64" }
+// CHECK: attributes #[[ATTR_BAR]] = { {{.*}} }
+// CHECK: attributes #[[ATTR_COMPATIBLE]] = { {{.*}} }
+// CHECK: attributes #[[ATTR_EXTEND]] = { convergent noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="gfx90a" "target-features"="+16-bit-insts,+atomic-buffer-global-pk-add-f16-insts,+atomic-fadd-rtn-insts,+ci-insts,+dl-insts,+dot1-insts,+dot10-insts,+dot2-insts,+dot3-insts,+dot4-insts,+dot5-insts,+dot6-insts,+dot7-insts,+dpp,+extended-image-insts,+gfx8-insts,+gfx9-insts,+gfx90a-insts,+gws,+image-insts,+mai-insts,+s-memrealtime,+s-memtime-inst,+wavefrontsize64" }
+// CHECK: attributes #[[ATTR_INCOMPATIBLE]] = { convergent noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="gfx90a" "target-features"="+16-bit-insts,+atomic-buffer-global-pk-add-f16-insts,+atomic-fadd-rtn-insts,+ci-insts,+dl-insts,+dot1-insts,+dot10-insts,+dot2-insts,+dot3-insts,+dot4-insts,+dot5-insts,+dot6-insts,+dot7-insts,+dpp,+gfx8-insts,+gfx90a-insts,+gws,+image-insts,+mai-insts,+s-memrealtime,+s-memtime-inst,+wavefrontsize64,-gfx9-insts" }

[jmmartinez]

@yxsamliu Is it ok if I merge this patch?

[yxsamliu]

LGTM. Sorry for the delay