llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Adam Smith (adams381) <details> <summary>Changes</summary> A function declaration carried no CPU or feature attributes. CIRGen set them from `setNonAliasAttributes`, which runs only for a definition, where classic CodeGen sets them from `ConstructAttributeList` for a declaration too. We now set them properly in `constructAttributeList` alongside the other non-call-site attributes. Recording them on a declaration exposes a second bug. When a function is declared first and defined later with a `target` attribute, `setNonAliasAttributes` wrote the definition's values over the declaration's rather than replacing them, so a `tune-cpu` that the `target` attribute suppresses survived. It now clears the three attributes before writing, which is safe because `getCPUAndFeaturesAttributes` resolves the most recent declaration, so its result supersedes anything an earlier one wrote. Assisted-by: Cursor / claude-opus-5 --- Full diff: https://github.com/llvm/llvm-project/pull/214986.diff 6 Files Affected: - (modified) clang/lib/CIR/CodeGen/CIRGenCall.cpp (+8-1) - (modified) clang/lib/CIR/CodeGen/CIRGenModule.cpp (+19-6) - (modified) clang/test/CIR/CodeGen/alloc-size.c (+2-2) - (modified) clang/test/CIR/CodeGen/asm-label-redirect.c (+2-2) - (modified) clang/test/CIR/CodeGen/attr-target-x86.c (+29) - (modified) clang/test/CIR/CodeGen/global-init.cpp (+3-1) ``````````diff diff --git a/clang/lib/CIR/CodeGen/CIRGenCall.cpp b/clang/lib/CIR/CodeGen/CIRGenCall.cpp index 28670cf31694a..0fd1fc4deca51 100644 --- a/clang/lib/CIR/CodeGen/CIRGenCall.cpp +++ b/clang/lib/CIR/CodeGen/CIRGenCall.cpp @@ -492,11 +492,18 @@ void CIRGenModule::constructAttributeList( "sigsetjmp", "__sigsetjmp", "savectx", "getcontext"}; if (returnsTwiceFn.contains(name)) addUnitAttr(cir::CIRDialect::getReturnsTwiceAttrName()); + + llvm::StringMap<std::string> cpuAndFeatures; + if (getCPUAndFeaturesAttributes(calleeInfo.getCalleeDecl(), + cpuAndFeatures)) { + for (const auto &[key, val] : cpuAndFeatures) + attrs.set(key, builder.getStringAttr(val)); + } } // TODO(cir): A bunch of non-call-site function IR attributes from // declaration-specific information, including tail calls, - // cmse_nonsecure_entry, CPU-features/overrides, and hotpatch support. + // cmse_nonsecure_entry, and hotpatch support. // TODO(cir): Add loader-replaceable attribute here. diff --git a/clang/lib/CIR/CodeGen/CIRGenModule.cpp b/clang/lib/CIR/CodeGen/CIRGenModule.cpp index ae9cad0b7c30f..1b5ed527497fd 100644 --- a/clang/lib/CIR/CodeGen/CIRGenModule.cpp +++ b/clang/lib/CIR/CodeGen/CIRGenModule.cpp @@ -806,6 +806,13 @@ getFeatureDeltaFromDefault(const CIRGenModule &cgm, llvm::StringRef targetCPU, return delta; } +/// The names getCPUAndFeaturesAttributes produces. setNonAliasAttributes +/// clears them before writing a definition's values, so the two have to agree. +static constexpr llvm::StringLiteral targetCPUAttrName = "cir.target-cpu"; +static constexpr llvm::StringLiteral tuneCPUAttrName = "cir.tune-cpu"; +static constexpr llvm::StringLiteral targetFeaturesAttrName = + "cir.target-features"; + bool CIRGenModule::getCPUAndFeaturesAttributes( GlobalDecl gd, llvm::StringMap<std::string> &attrs, bool setTargetFeatures) { @@ -885,11 +892,11 @@ bool CIRGenModule::getCPUAndFeaturesAttributes( } if (!targetCPU.empty()) { - attrs["cir.target-cpu"] = targetCPU.str(); + attrs[targetCPUAttrName] = targetCPU.str(); addedAttr = true; } if (!tuneCPU.empty()) { - attrs["cir.tune-cpu"] = tuneCPU.str(); + attrs[tuneCPUAttrName] = tuneCPU.str(); addedAttr = true; } if (!features.empty() && setTargetFeatures) { @@ -899,7 +906,7 @@ bool CIRGenModule::getCPUAndFeaturesAttributes( return getTarget().isReadOnlyFeature(f.substr(1)); }); llvm::sort(features); - attrs["cir.target-features"] = llvm::join(features, ","); + attrs[targetFeaturesAttrName] = llvm::join(features, ","); addedAttr = true; } // TODO(cir): add metadata for AArch64 Function Multi Versioning. @@ -921,9 +928,15 @@ void CIRGenModule::setNonAliasAttributes(GlobalDecl gd, mlir::Operation *op) { if (auto func = dyn_cast<cir::FuncOp>(op)) { llvm::StringMap<std::string> attrs; if (getCPUAndFeaturesAttributes(gd, attrs)) { - // TODO(cir): Classic codegen removes the existing target-cpu, - // target-features, tune-cpu and fmv-features attributes here - // before adding the new ones. + // TODO(cir): Classic codegen also removes fmv-features here, which + // CIR does not emit yet. + // + // getCPUAndFeaturesAttributes reads the most recent declaration, so + // its result supersedes anything an earlier one wrote. Clear first: + // setAttr alone would leave a name this call no longer produces. + for (llvm::StringRef name : + {targetCPUAttrName, tuneCPUAttrName, targetFeaturesAttrName}) + func->removeAttr(name); for (const auto &[key, val] : attrs) func->setAttr(key, builder.getStringAttr(val)); } diff --git a/clang/test/CIR/CodeGen/alloc-size.c b/clang/test/CIR/CodeGen/alloc-size.c index e3ff12da1a083..c3c28cafb92db 100644 --- a/clang/test/CIR/CodeGen/alloc-size.c +++ b/clang/test/CIR/CodeGen/alloc-size.c @@ -9,9 +9,9 @@ typedef unsigned long size_t; -// CIR: cir.func{{.*}}@my_malloc(!s32i {llvm.noundef}){{.*}} attributes {allocsize = array<i32: 0>} +// CIR: cir.func{{.*}}@my_malloc(!s32i {llvm.noundef}){{.*}} attributes {allocsize = array<i32: 0>{{[,}]}} extern void *my_malloc(int) __attribute__((alloc_size(1))); -// CIR: cir.func{{.*}}@my_calloc(!s32i {llvm.noundef}, !s32i {llvm.noundef}){{.*}} attributes {allocsize = array<i32: 0, 1>} +// CIR: cir.func{{.*}}@my_calloc(!s32i {llvm.noundef}, !s32i {llvm.noundef}){{.*}} attributes {allocsize = array<i32: 0, 1>{{[,}]}} extern void *my_calloc(int, int) __attribute__((alloc_size(1, 2))); // CIR-LABEL: @call_direct diff --git a/clang/test/CIR/CodeGen/asm-label-redirect.c b/clang/test/CIR/CodeGen/asm-label-redirect.c index 3e40976df04a9..0dc01c912da2d 100644 --- a/clang/test/CIR/CodeGen/asm-label-redirect.c +++ b/clang/test/CIR/CodeGen/asm-label-redirect.c @@ -26,8 +26,8 @@ int test(const char *p) { // it sees first - here, the my_stat declaration. // // CIR-LABEL: cir.func private @real_impl( -// CIR-SAME: !cir.ptr<!s8i> {{.*}}, -// CIR-SAME: !cir.ptr<!rec_my_stat> {{.*}}) -> !s32i +// CIR-SAME: !cir.ptr<!s8i> {{[^,]*}}, +// CIR-SAME: !cir.ptr<!rec_my_stat> {{[^,]*}}) -> !s32i // CIR-LABEL: cir.func {{.*}} @test( // diff --git a/clang/test/CIR/CodeGen/attr-target-x86.c b/clang/test/CIR/CodeGen/attr-target-x86.c index c99484c42a1db..d46749c58ac34 100644 --- a/clang/test/CIR/CodeGen/attr-target-x86.c +++ b/clang/test/CIR/CodeGen/attr-target-x86.c @@ -26,6 +26,8 @@ // LLVM: define {{.*}}@f_avx10_1{{.*}} [[f_avx10_1:#[0-9]+]] // LLVM: define {{.*}}@f_prefer_256_bit({{.*}} [[f_prefer_256_bit:#[0-9]+]] // LLVM: define {{.*}}@f_no_prefer_256_bit({{.*}} [[f_no_prefer_256_bit:#[0-9]+]] +// LLVM: declare {{.*}}@f_decl_only() [[f_decl_only:#[0-9]+]] +// LLVM: declare {{.*}}@f_decl_default() [[f_decl_default:#[0-9]+]] // CIR: cir.func{{.*}} @f_default() // CIR-SAME: "cir.target-cpu" = "i686" @@ -124,6 +126,11 @@ void usage(void){ // f_use_before_def: same attributes as f_lakemont_mmx (checked above) - the // definition's attribute should be propagated to the earlier declaration. +// The dictionary closes after the features, so the tune-cpu the declaration +// recorded before the definition was seen does not survive. +// CIR: cir.func{{.*}} @f_use_before_def() +// CIR-SAME: "cir.target-cpu" = "lakemont" +// CIR-SAME: "cir.target-features" = "+cx8,+mmx", nothrow} __attribute__((target("arch=lakemont,mmx"))) void f_use_before_def(void) {} @@ -184,3 +191,25 @@ void f_prefer_256_bit(void) {} // LLVM: [[f_no_prefer_256_bit]] = {{.*}}"target-features"="{{.*}}-prefer-256-bit __attribute__((target("no-prefer-256-bit"))) void f_no_prefer_256_bit(void) {} + +// A declaration gets the attributes too. target(arch=) suppresses tune-cpu, +// so the dictionary closes after the features. +// CIR: cir.func private @f_decl_only() +// CIR-SAME: "cir.target-cpu" = "lakemont" +// CIR-SAME: "cir.target-features" = "+cx8,+mmx"} + +// LLVM: [[f_decl_only]] = {{.*}}"target-cpu"="lakemont" "target-features"="+cx8,+mmx" } +__attribute__((target("arch=lakemont,mmx"))) +void f_decl_only(void); +void use_decl_only(void) { f_decl_only(); } + +// A declaration with no target attribute takes all three from the command +// line, tune-cpu included. +// CIR: cir.func private @f_decl_default() +// CIR-SAME: "cir.target-cpu" = "i686" +// CIR-SAME: "cir.target-features" = "+cmov,+cx8,+x87" +// CIR-SAME: "cir.tune-cpu" = "i686" + +// LLVM: [[f_decl_default]] = {{.*}}"target-cpu"="i686" "target-features"="+cmov,+cx8,+x87" "tune-cpu"="i686" +void f_decl_default(void); +void use_decl_default(void) { f_decl_default(); } diff --git a/clang/test/CIR/CodeGen/global-init.cpp b/clang/test/CIR/CodeGen/global-init.cpp index 7abb70e42d608..c5bdded22af98 100644 --- a/clang/test/CIR/CodeGen/global-init.cpp +++ b/clang/test/CIR/CodeGen/global-init.cpp @@ -230,7 +230,7 @@ ArrayDtor arrDtor[16]; // LLVM: %[[CUR:.*]] = load ptr, ptr %[[CUR_ADDR]] // LLVM: %[[PREV:.*]] = getelementptr %struct.ArrayDtor, ptr %[[CUR]], i64 -1 // LLVM: store ptr %[[PREV]], ptr %[[CUR_ADDR]] -// LLVM: call void @_ZN9ArrayDtorD1Ev(ptr noundef nonnull align 1 dereferenceable(1) %[[PREV]]) #0 +// LLVM: call void @_ZN9ArrayDtorD1Ev(ptr noundef nonnull align 1 dereferenceable(1) %[[PREV]]) [[NOUNWIND:#[0-9]+]] // LLVM: br label %[[LOOP_COND]] // LLVM: [[LOOP_END]]: // LLVM: ret void @@ -279,6 +279,8 @@ ArrayDtor arrDtor[16]; // LLVM: call void @__cxx_global_var_init.4() // LLVM: call void @__cxx_global_var_init.5() +// LLVM: attributes [[NOUNWIND]] = { nounwind } + // OGCG: define internal void @_GLOBAL__sub_I_[[FILENAME]]() {{.*}} section ".text.startup" { // OGCG: call void @__cxx_global_var_init() // OGCG: call void @__cxx_global_var_init.1() `````````` </details> https://github.com/llvm/llvm-project/pull/214986 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
