[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