llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clangir Author: Erich Keane (erichkeane) <details> <summary>Changes</summary> Itanium emits the complete-object ctor/dtor as an alias to the base object version when they are able to be put into COMDAT. However, this is not currently implemented in CIR, where we are missing the 'comdat with a name' functionality. This patch limits that to ONLY FuncOp, and only does the named version (instead of referencing its own name) for these ctor/dtors. Doing this for the comdat attribute and for GlobalOp is left to a future patch. This showed up in the 'root' project, where the wrong dtor was called as a result! Note: Claude helped me with the diagnosis, and wrote the reproducer. --- Full diff: https://github.com/llvm/llvm-project/pull/225929.diff 6 Files Affected: - (modified) clang/include/clang/CIR/Dialect/IR/CIROps.td (+12-1) - (modified) clang/include/clang/CIR/Interfaces/CIROpInterfaces.td (+1-1) - (modified) clang/lib/CIR/CodeGen/CIRGenItaniumCXXABI.cpp (+17-1) - (modified) clang/lib/CIR/Dialect/IR/CIRDialect.cpp (+15-3) - (modified) clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp (+4-2) - (added) clang/test/CIR/CodeGen/dtor-comdat-key.cpp (+19) ``````````diff diff --git a/clang/include/clang/CIR/Dialect/IR/CIROps.td b/clang/include/clang/CIR/Dialect/IR/CIROps.td index f6b524176a655..2b772c34d1cb7 100644 --- a/clang/include/clang/CIR/Dialect/IR/CIROps.td +++ b/clang/include/clang/CIR/Dialect/IR/CIROps.td @@ -3439,6 +3439,9 @@ def CIR_GlobalOp : CIR_RegionBranchOpBase<"global", [ OptionalAttr<CIR_TLSModelAttr>:$tls_model, OptionalAttr<CIR_ThreadLocalGlobalWrapperInitAttr>:$tls_refs, OptionalAttr<AnyAttr>:$initial_value, + // FIXME(cir): This eventually needs to become a + // stringattr like FuncOp to allow customization of the + // group name. UnitProp:$comdat, UnitProp:$constant, UnitProp:$dso_local, @@ -4277,6 +4280,10 @@ def CIR_FuncOp : CIR_Op<"func", [ exception handling. This is a symbol reference to the personality function (e.g., `@__gxx_personality_v0` for C++ exceptions). + The 'comdat' attribute marks the function as belonging to a COMDAT group. + This attribute includes an optional 'group' name, which if not provided, + results in the function's symbol name being used. + Example: ``` @@ -4329,7 +4336,7 @@ def CIR_FuncOp : CIR_Op<"func", [ "cir::CallingConv::C" >:$calling_conv, OptionalAttr<StrAttr>:$sym_visibility, - UnitAttr:$comdat, + OptionalAttr<StrAttr>:$comdat, OptionalAttr<I64Attr>:$alignment, OptionalAttr<I64Attr>:$preferred_alignment, OptionalAttr<DictArrayAttr>:$arg_attrs, @@ -4376,6 +4383,10 @@ def CIR_FuncOp : CIR_Op<"func", [ return getFunctionType().getReturnTypes(); } + void setComdat(bool) { + setComdat(llvm::StringRef("")); + } + //===------------------------------------------------------------------===// // SymbolOpInterface Methods //===------------------------------------------------------------------===// diff --git a/clang/include/clang/CIR/Interfaces/CIROpInterfaces.td b/clang/include/clang/CIR/Interfaces/CIROpInterfaces.td index 1cd45dc5326a0..7ebce64d3e87b 100644 --- a/clang/include/clang/CIR/Interfaces/CIROpInterfaces.td +++ b/clang/include/clang/CIR/Interfaces/CIROpInterfaces.td @@ -169,7 +169,7 @@ let cppNamespace = "::cir" in { InterfaceMethod<"", "bool", "hasComdat", (ins), [{}], /*defaultImplementation=*/[{ - return $_op.getComdat(); + return static_cast<bool>($_op.getComdat()); }] >, InterfaceMethod<"", diff --git a/clang/lib/CIR/CodeGen/CIRGenItaniumCXXABI.cpp b/clang/lib/CIR/CodeGen/CIRGenItaniumCXXABI.cpp index 59461a6c6de4a..827f000ae573d 100644 --- a/clang/lib/CIR/CodeGen/CIRGenItaniumCXXABI.cpp +++ b/clang/lib/CIR/CodeGen/CIRGenItaniumCXXABI.cpp @@ -22,11 +22,14 @@ #include "clang/AST/ExprCXX.h" #include "clang/AST/GlobalDecl.h" +#include "clang/AST/Mangle.h" #include "clang/AST/TypeBase.h" #include "clang/AST/VTableBuilder.h" #include "clang/CIR/MissingFeatures.h" #include "clang/CodeGenUtils/ItaniumCXXABIUtils.h" +#include "llvm/ADT/SmallString.h" #include "llvm/Support/ErrorHandling.h" +#include "llvm/Support/raw_ostream.h" using namespace clang; using namespace clang::CIRGen; @@ -358,6 +361,7 @@ void CIRGenItaniumCXXABI::emitCXXStructor(GlobalDecl gd) { auto *md = cast<CXXMethodDecl>(gd.getDecl()); StructorCIRGen cirGenType = getCIRGenToUse(cgm, md); const auto *cd = dyn_cast<CXXConstructorDecl>(md); + const CXXDestructorDecl *dd = cd ? nullptr : cast<CXXDestructorDecl>(md); if (cd ? gd.getCtorType() == Ctor_Complete : gd.getDtorType() == Dtor_Complete) { @@ -381,7 +385,19 @@ void CIRGenItaniumCXXABI::emitCXXStructor(GlobalDecl gd) { auto fn = cgm.codegenCXXStructor(gd); - cgm.maybeSetTrivialComdat(*md, fn); + if (cirGenType == StructorCIRGen::COMDAT) { + llvm::SmallString<256> comdatKey; + llvm::raw_svector_ostream out(comdatKey); + ItaniumMangleContext &mangler = + cast<ItaniumMangleContext>(cgm.getCXXABI().getMangleContext()); + if (dd) + mangler.mangleCXXDtorComdat(dd, out); + else + mangler.mangleCXXCtorComdat(cd, out); + fn.setComdat(llvm::StringRef(comdatKey)); + } else { + cgm.maybeSetTrivialComdat(*md, fn); + } } void CIRGenItaniumCXXABI::addImplicitStructorParams(CIRGenFunction &cgf, diff --git a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp index b994d19ea1477..ce406707f5942 100644 --- a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp +++ b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp @@ -2708,8 +2708,17 @@ ParseResult cir::FuncOp::parse(OpAsmParser &parser, OperationState &state) { if (parser.parseOptionalKeyword(noProtoNameAttr).succeeded()) state.addAttribute(noProtoNameAttr, parser.getBuilder().getUnitAttr()); - if (parser.parseOptionalKeyword(comdatNameAttr).succeeded()) - state.addAttribute(comdatNameAttr, parser.getBuilder().getUnitAttr()); + if (parser.parseOptionalKeyword(comdatNameAttr).succeeded()) { + std::string comdatKey; + if (mlir::succeeded(parser.parseOptionalLParen())) { + if (parser.parseString(&comdatKey).failed()) + return failure(); + if (parser.parseRParen().failed()) + return failure(); + } + state.addAttribute(comdatNameAttr, + parser.getBuilder().getStringAttr(comdatKey)); + } auto parseAlignmentBody = [&](int64_t &value) { if (parser.parseLParen().failed() || parser.parseInteger(value).failed() || @@ -3043,8 +3052,11 @@ void cir::FuncOp::print(OpAsmPrinter &p) { if (getNoProto()) p << " no_proto"; - if (getComdat()) + if (std::optional<StringRef> comdatKey = getComdat()) { p << " comdat"; + if (!comdatKey->empty()) + p << "(\"" << *comdatKey << "\")"; + } if (getAlignment()) p << " alignment(" << *getAlignment() << ')'; diff --git a/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp b/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp index 1b1423862ba7b..bd529f71b38ed 100644 --- a/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp +++ b/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp @@ -3215,10 +3215,12 @@ CIRToLLVMGlobalOpLowering::getComdatAttr(cir::GlobalOp &op, mlir::SymbolRefAttr CIRToLLVMFuncOpLowering::getComdatAttr(cir::FuncOp &op, mlir::OpBuilder &builder) const { - if (!op.getComdat()) + std::optional<llvm::StringRef> comdat = op.getComdat(); + if (!comdat) return mlir::SymbolRefAttr{}; + llvm::StringRef comdatKey = comdat->empty() ? op.getSymName() : *comdat; return getComdatAttrHelper(op->getParentOfType<mlir::ModuleOp>(), builder, - op.getSymName(), comdatOp); + comdatKey, comdatOp); } mlir::LogicalResult CIRToLLVMSwitchFlatOpLowering::matchAndRewrite( diff --git a/clang/test/CIR/CodeGen/dtor-comdat-key.cpp b/clang/test/CIR/CodeGen/dtor-comdat-key.cpp new file mode 100644 index 0000000000000..69121adb77669 --- /dev/null +++ b/clang/test/CIR/CodeGen/dtor-comdat-key.cpp @@ -0,0 +1,19 @@ +// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++17 -mconstructor-aliases -fclangir -emit-cir %s -o %t.cir +// RUN: FileCheck --input-file=%t.cir %s --check-prefix=CIR +// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++17 -mconstructor-aliases -fclangir -emit-llvm %s -o %t-cir.ll +// RUN: FileCheck --input-file=%t-cir.ll %s --check-prefix=LLVM +// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++17 -mconstructor-aliases -emit-llvm %s -o %t.ll +// RUN: FileCheck --input-file=%t.ll %s --check-prefix=LLVM + +template <class T> struct K { + T v; + virtual ~K() { v = T(); } +}; +template class K<double>; + +// CIR: cir.func {{.*}} comdat("_ZN1KIdED5Ev") {{.*}} weak_odr @_ZN1KIdED2Ev( +// CIR: cir.func weak_odr private @_ZN1KIdED1Ev(!cir.ptr<!rec_K3Cdouble3E>) alias(@_ZN1KIdED2Ev) + +// LLVM: $_ZN1KIdED5Ev = comdat any +// LLVM: @_ZN1KIdED1Ev = weak_odr {{.*}}alias void (ptr), ptr @_ZN1KIdED2Ev +// LLVM: define weak_odr void @_ZN1KIdED2Ev({{.*}}) {{.*}} comdat($_ZN1KIdED5Ev) `````````` </details> https://github.com/llvm/llvm-project/pull/225929 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
