[CIR] Add support for function linkage and visibility #145600
Merged
+845
−542
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change adds support for function linkage and visibility and related attributes. Most of the test changes are generalizations to allow 'dso_local' to be accepted where we aren't specifically testing for it. Some tests based on CIR inputs have been updated to add 'private' to function declarations where required by newly supported interfaces. The dso-local.c test has been updated to add specific tests for dso_local being set correctly, and a new test, func-linkage.cpp tests other linkage settings.
andykaylor
requested review from
xlauko,
bcardosolopes,
mmha,
erichkeane,
lanza and
Lancern
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clangir Author: Andy Kaylor (andykaylor) ChangesThis change adds support for function linkage and visibility and related attributes. Most of the test changes are generalizations to allow 'dso_local' to be accepted where we aren't specifically testing for it. Some tests based on CIR inputs have been updated to add 'private' to function declarations where required by newly supported interfaces. The dso-local.c test has been updated to add specific tests for dso_local being set correctly, and a new test, func-linkage.cpp tests other linkage settings. This change sets Patch is 217.10 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/145600.diff 86 Files Affected:
diff --git a/clang/include/clang/CIR/Dialect/IR/CIROps.td b/clang/include/clang/CIR/Dialect/IR/CIROps.td
index decba83251df2..368bcae259246 100644
--- a/clang/include/clang/CIR/Dialect/IR/CIROps.td
+++ b/clang/include/clang/CIR/Dialect/IR/CIROps.td
@@ -1737,16 +1737,40 @@ def GetMemberOp : CIR_Op<"get_member"> {
def FuncOp : CIR_Op<"func", [
AutomaticAllocationScope, CallableOpInterface, FunctionOpInterface,
+ DeclareOpInterfaceMethods<CIRGlobalValueInterface>,
IsolatedFromAbove
]> {
let summary = "Declare or define a function";
let description = [{
The `cir.func` operation defines a function, similar to the `mlir::FuncOp`
built-in.
+
+ The function linkage information is specified by `linkage`, as defined by
+ `GlobalLinkageKind` attribute.
+
+ Example:
+
+ ```mlir
+ // External function definitions.
+ cir.func @abort()
+
+ // A function with internal linkage.
+ cir.func internal @count(%x: i64) -> (i64)
+ return %x : i64
+
+ // Linkage information
+ cir.func linkonce_odr @some_method(...)
+ ```
}];
let arguments = (ins SymbolNameAttr:$sym_name,
+ CIR_VisibilityAttr:$global_visibility,
TypeAttrOf<CIR_FuncType>:$function_type,
+ UnitAttr:$dso_local,
+ DefaultValuedAttr<CIR_GlobalLinkageKind,
+ "cir::GlobalLinkageKind::ExternalLinkage">:$linkage,
+ OptionalAttr<StrAttr>:$sym_visibility,
+ UnitAttr:$comdat,
OptionalAttr<DictArrayAttr>:$arg_attrs,
OptionalAttr<DictArrayAttr>:$res_attrs);
@@ -1754,8 +1778,10 @@ def FuncOp : CIR_Op<"func", [
let skipDefaultBuilders = 1;
- let builders = [OpBuilder<(ins "llvm::StringRef":$sym_name,
- "FuncType":$type)>];
+ let builders = [OpBuilder<(ins
+ "llvm::StringRef":$sym_name, "FuncType":$type,
+ CArg<"cir::GlobalLinkageKind", "cir::GlobalLinkageKind::ExternalLinkage">:$linkage)
+ >];
let extraClassDeclaration = [{
/// Returns the region on the current operation that is callable. This may
diff --git a/clang/include/clang/CIR/MissingFeatures.h b/clang/include/clang/CIR/MissingFeatures.h
index fb5014a877151..b6c05e42c59c3 100644
--- a/clang/include/clang/CIR/MissingFeatures.h
+++ b/clang/include/clang/CIR/MissingFeatures.h
@@ -72,16 +72,18 @@ struct MissingFeatures {
// FuncOp handling
static bool opFuncOpenCLKernelMetadata() { return false; }
+ static bool opFuncAstDeclAttr() { return false; }
static bool opFuncCallingConv() { return false; }
static bool opFuncExtraAttrs() { return false; }
- static bool opFuncDsoLocal() { return false; }
- static bool opFuncLinkage() { return false; }
- static bool opFuncVisibility() { return false; }
static bool opFuncNoProto() { return false; }
static bool opFuncCPUAndFeaturesAttributes() { return false; }
static bool opFuncSection() { return false; }
- static bool opFuncSetComdat() { return false; }
+ static bool opFuncMultipleReturnVals() { return false; }
static bool opFuncAttributesForDefinition() { return false; }
+ static bool opFuncMaybeHandleStaticInExternC() { return false; }
+ static bool opFuncGlobalAliases() { return false; }
+ static bool setLLVMFunctionFEnvAttributes() { return false; }
+ static bool setFunctionAttributes() { return false; }
// CallOp handling
static bool opCallPseudoDtor() { return false; }
diff --git a/clang/lib/CIR/CodeGen/CIRGenCXX.cpp b/clang/lib/CIR/CodeGen/CIRGenCXX.cpp
index 51751483d34e9..da507d6f28335 100644
--- a/clang/lib/CIR/CodeGen/CIRGenCXX.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenCXX.cpp
@@ -25,7 +25,7 @@ cir::FuncOp CIRGenModule::codegenCXXStructor(GlobalDecl gd) {
cir::FuncType funcType = getTypes().getFunctionType(fnInfo);
cir::FuncOp fn = getAddrOfCXXStructor(gd, &fnInfo, /*FnType=*/nullptr,
/*DontDefer=*/true, ForDefinition);
- assert(!cir::MissingFeatures::opFuncLinkage());
+ setFunctionLinkage(gd, fn);
CIRGenFunction cgf{*this, builder};
curCGF = &cgf;
{
diff --git a/clang/lib/CIR/CodeGen/CIRGenModule.cpp b/clang/lib/CIR/CodeGen/CIRGenModule.cpp
index 68ab81ed53af9..06b47d700a433 100644
--- a/clang/lib/CIR/CodeGen/CIRGenModule.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenModule.cpp
@@ -406,6 +406,16 @@ void CIRGenModule::emitGlobalFunctionDefinition(clang::GlobalDecl gd,
/*DontDefer=*/true, ForDefinition);
}
+ // Already emitted.
+ if (!funcOp.isDeclaration())
+ return;
+
+ setFunctionLinkage(gd, funcOp);
+ setGVProperties(funcOp, funcDecl);
+ assert(!cir::MissingFeatures::opFuncMaybeHandleStaticInExternC());
+ maybeSetTrivialComdat(*funcDecl, funcOp);
+ assert(!cir::MissingFeatures::setLLVMFunctionFEnvAttributes());
+
CIRGenFunction cgf(*this, builder);
curCGF = &cgf;
{
@@ -413,7 +423,17 @@ void CIRGenModule::emitGlobalFunctionDefinition(clang::GlobalDecl gd,
cgf.generateCode(gd, funcOp, funcType);
}
curCGF = nullptr;
+
+ setNonAliasAttributes(gd, funcOp);
assert(!cir::MissingFeatures::opFuncAttributesForDefinition());
+
+ if (const ConstructorAttr *ca = funcDecl->getAttr<ConstructorAttr>())
+ errorNYI(funcDecl->getSourceRange(), "constructor attribute");
+ if (const DestructorAttr *da = funcDecl->getAttr<DestructorAttr>())
+ errorNYI(funcDecl->getSourceRange(), "destructor attribute");
+
+ if (funcDecl->getAttr<AnnotateAttr>())
+ errorNYI(funcDecl->getSourceRange(), "deferredAnnotations");
}
mlir::Operation *CIRGenModule::getGlobalValue(StringRef name) {
@@ -855,10 +875,12 @@ static bool shouldBeInCOMDAT(CIRGenModule &cgm, const Decl &d) {
void CIRGenModule::maybeSetTrivialComdat(const Decl &d, mlir::Operation *op) {
if (!shouldBeInCOMDAT(*this, d))
return;
- if (auto globalOp = dyn_cast_or_null<cir::GlobalOp>(op))
+ if (auto globalOp = dyn_cast_or_null<cir::GlobalOp>(op)) {
globalOp.setComdat(true);
-
- assert(!cir::MissingFeatures::opFuncSetComdat());
+ } else {
+ auto funcOp = cast<cir::FuncOp>(op);
+ funcOp.setComdat(true);
+ }
}
void CIRGenModule::updateCompletedType(const TagDecl *td) {
@@ -1028,6 +1050,17 @@ CIRGenModule::getCIRLinkageVarDefinition(const VarDecl *vd, bool isConstant) {
return getCIRLinkageForDeclarator(vd, linkage, isConstant);
}
+cir::GlobalLinkageKind CIRGenModule::getFunctionLinkage(GlobalDecl gd) {
+ const auto *fd = cast<FunctionDecl>(gd.getDecl());
+
+ GVALinkage linkage = astContext.GetGVALinkageForFunction(fd);
+
+ if (const auto *dtor = dyn_cast<CXXDestructorDecl>(fd))
+ errorNYI(fd->getSourceRange(), "getFunctionLinkage: CXXDestructorDecl");
+
+ return getCIRLinkageForDeclarator(fd, linkage, /*IsConstantVariable=*/false);
+}
+
static cir::GlobalOp
generateStringLiteral(mlir::Location loc, mlir::TypedAttr c,
cir::GlobalLinkageKind lt, CIRGenModule &cgm,
@@ -1534,6 +1567,27 @@ void CIRGenModule::setGVPropertiesAux(mlir::Operation *op,
assert(!cir::MissingFeatures::opGlobalPartition());
}
+void CIRGenModule::setFunctionAttributes(GlobalDecl globalDecl,
+ cir::FuncOp func,
+ bool isIncompleteFunction,
+ bool isThunk) {
+ // NOTE(cir): Original CodeGen checks if this is an intrinsic. In CIR we
+ // represent them in dedicated ops. The correct attributes are ensured during
+ // translation to LLVM. Thus, we don't need to check for them here.
+
+ assert(!cir::MissingFeatures::setFunctionAttributes());
+ assert(!cir::MissingFeatures::setTargetAttributes());
+
+ // TODO(cir): This needs a lot of work to better match CodeGen. That
+ // ultimately ends up in setGlobalVisibility, which already has the linkage of
+ // the LLVM GV (corresponding to our FuncOp) computed, so it doesn't have to
+ // recompute it here. This is a minimal fix for now.
+ if (!isLocalLinkage(getFunctionLinkage(globalDecl))) {
+ const auto *decl = globalDecl.getDecl();
+ func.setGlobalVisibilityAttr(getGlobalVisibilityAttrFromDecl(decl));
+ }
+}
+
cir::FuncOp CIRGenModule::getOrCreateCIRFunction(
StringRef mangledName, mlir::Type funcType, GlobalDecl gd, bool forVTable,
bool dontDefer, bool isThunk, ForDefinition_t isForDefinition,
@@ -1576,8 +1630,9 @@ cir::FuncOp CIRGenModule::getOrCreateCIRFunction(
// If there are two attempts to define the same mangled name, issue an
// error.
auto fn = cast<cir::FuncOp>(entry);
- assert((!isForDefinition || !fn || !fn.isDeclaration()) &&
- "Duplicate function definition");
+ if (isForDefinition && fn && !fn.isDeclaration()) {
+ errorNYI(d->getSourceRange(), "Duplicate function definition");
+ }
if (fn && fn.getFunctionType() == funcType) {
return fn;
}
@@ -1598,6 +1653,9 @@ cir::FuncOp CIRGenModule::getOrCreateCIRFunction(
invalidLoc ? theModule->getLoc() : getLoc(funcDecl->getSourceRange()),
mangledName, mlir::cast<cir::FuncType>(funcType), funcDecl);
+ if (d)
+ setFunctionAttributes(gd, funcOp, /*isIncompleteFunction=*/false, isThunk);
+
// 'dontDefer' actually means don't move this to the deferredDeclsToEmit list.
if (dontDefer) {
// TODO(cir): This assertion will need an additional condition when we
@@ -1668,6 +1726,20 @@ CIRGenModule::createCIRFunction(mlir::Location loc, StringRef name,
func = builder.create<cir::FuncOp>(loc, name, funcType);
+ assert(!cir::MissingFeatures::opFuncAstDeclAttr());
+ assert(!cir::MissingFeatures::opFuncNoProto());
+
+ assert(func.isDeclaration() && "expected empty body");
+
+ // A declaration gets private visibility by default, but external linkage
+ // as the default linkage.
+ func.setLinkageAttr(cir::GlobalLinkageKindAttr::get(
+ &getMLIRContext(), cir::GlobalLinkageKind::ExternalLinkage));
+ mlir::SymbolTable::setSymbolVisibility(
+ func, mlir::SymbolTable::Visibility::Private);
+
+ assert(!cir::MissingFeatures::opFuncExtraAttrs());
+
if (!cgf)
theModule.push_back(func);
}
diff --git a/clang/lib/CIR/CodeGen/CIRGenModule.h b/clang/lib/CIR/CodeGen/CIRGenModule.h
index 71806e3c5de21..9f6a57c31d291 100644
--- a/clang/lib/CIR/CodeGen/CIRGenModule.h
+++ b/clang/lib/CIR/CodeGen/CIRGenModule.h
@@ -268,6 +268,10 @@ class CIRGenModule : public CIRGenTypeCache {
void setGVProperties(mlir::Operation *op, const NamedDecl *d) const;
void setGVPropertiesAux(mlir::Operation *op, const NamedDecl *d) const;
+ /// Set function attributes for a function declaration.
+ void setFunctionAttributes(GlobalDecl gd, cir::FuncOp f,
+ bool isIncompleteFunction, bool isThunk);
+
void emitGlobalDefinition(clang::GlobalDecl gd,
mlir::Operation *op = nullptr);
void emitGlobalFunctionDefinition(clang::GlobalDecl gd, mlir::Operation *op);
@@ -340,10 +344,16 @@ class CIRGenModule : public CIRGenTypeCache {
clang::VisibilityAttr::VisibilityType visibility);
cir::VisibilityAttr getGlobalVisibilityAttrFromDecl(const Decl *decl);
static mlir::SymbolTable::Visibility getMLIRVisibility(cir::GlobalOp op);
-
+ cir::GlobalLinkageKind getFunctionLinkage(GlobalDecl gd);
cir::GlobalLinkageKind getCIRLinkageForDeclarator(const DeclaratorDecl *dd,
GVALinkage linkage,
bool isConstantVariable);
+ void setFunctionLinkage(GlobalDecl gd, cir::FuncOp f) {
+ cir::GlobalLinkageKind l = getFunctionLinkage(gd);
+ f.setLinkageAttr(cir::GlobalLinkageKindAttr::get(&getMLIRContext(), l));
+ mlir::SymbolTable::setSymbolVisibility(f,
+ getMLIRVisibilityFromCIRLinkage(l));
+ }
cir::GlobalLinkageKind getCIRLinkageVarDefinition(const VarDecl *vd,
bool isConstant);
diff --git a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
index 27f4ecb5ab85d..cbb91f747c513 100644
--- a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
+++ b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
@@ -115,9 +115,26 @@ template <typename Ty> struct EnumTraits {};
static unsigned getMaxEnumVal() { return cir::getMaxEnumValFor##Ty(); } \
}
+REGISTER_ENUM_TYPE(GlobalLinkageKind);
+REGISTER_ENUM_TYPE(VisibilityKind);
REGISTER_ENUM_TYPE(SideEffect);
} // namespace
+/// Parse an enum from the keyword, or default to the provided default value.
+/// The return type is the enum type by default, unless overriden with the
+/// second template argument.
+template <typename EnumTy, typename RetTy = EnumTy>
+static RetTy parseOptionalCIRKeyword(AsmParser &parser, EnumTy defaultValue) {
+ llvm::SmallVector<llvm::StringRef, 10> names;
+ for (unsigned i = 0, e = EnumTraits<EnumTy>::getMaxEnumVal(); i <= e; ++i)
+ names.push_back(EnumTraits<EnumTy>::stringify(static_cast<EnumTy>(i)));
+
+ int index = parseOptionalKeywordAlternative(parser, names);
+ if (index == -1)
+ return static_cast<RetTy>(defaultValue);
+ return static_cast<RetTy>(index);
+}
+
/// Parse an enum from the keyword, return failure if the keyword is not found.
template <typename EnumTy, typename RetTy = EnumTy>
static ParseResult parseCIRKeyword(AsmParser &parser, RetTy &result) {
@@ -170,6 +187,26 @@ static bool omitRegionTerm(mlir::Region &r) {
return singleNonEmptyBlock && yieldsNothing();
}
+void printVisibilityAttr(OpAsmPrinter &printer,
+ cir::VisibilityAttr &visibility) {
+ switch (visibility.getValue()) {
+ case cir::VisibilityKind::Hidden:
+ printer << "hidden";
+ break;
+ case cir::VisibilityKind::Protected:
+ printer << "protected";
+ break;
+ default:
+ break;
+ }
+}
+
+void parseVisibilityAttr(OpAsmParser &parser, cir::VisibilityAttr &visibility) {
+ cir::VisibilityKind visibilityKind =
+ parseOptionalCIRKeyword(parser, cir::VisibilityKind::Default);
+ visibility = cir::VisibilityAttr::get(parser.getContext(), visibilityKind);
+}
+
//===----------------------------------------------------------------------===//
// CIR Custom Parsers/Printers
//===----------------------------------------------------------------------===//
@@ -1287,19 +1324,54 @@ cir::GetGlobalOp::verifySymbolUses(SymbolTableCollection &symbolTable) {
// FuncOp
//===----------------------------------------------------------------------===//
+/// Returns the name used for the linkage attribute. This *must* correspond to
+/// the name of the attribute in ODS.
+static llvm::StringRef getLinkageAttrNameString() { return "linkage"; }
+
void cir::FuncOp::build(OpBuilder &builder, OperationState &result,
- StringRef name, FuncType type) {
+ StringRef name, FuncType type,
+ GlobalLinkageKind linkage) {
result.addRegion();
result.addAttribute(SymbolTable::getSymbolAttrName(),
builder.getStringAttr(name));
result.addAttribute(getFunctionTypeAttrName(result.name),
TypeAttr::get(type));
+ result.addAttribute(
+ getLinkageAttrNameString(),
+ GlobalLinkageKindAttr::get(builder.getContext(), linkage));
+ result.addAttribute(getGlobalVisibilityAttrName(result.name),
+ cir::VisibilityAttr::get(builder.getContext()));
}
ParseResult cir::FuncOp::parse(OpAsmParser &parser, OperationState &state) {
llvm::SMLoc loc = parser.getCurrentLocation();
mlir::Builder &builder = parser.getBuilder();
+ mlir::StringAttr visNameAttr = getSymVisibilityAttrName(state.name);
+ mlir::StringAttr visibilityNameAttr = getGlobalVisibilityAttrName(state.name);
+ mlir::StringAttr dsoLocalNameAttr = getDsoLocalAttrName(state.name);
+
+ // Default to external linkage if no keyword is provided.
+ state.addAttribute(getLinkageAttrNameString(),
+ GlobalLinkageKindAttr::get(
+ parser.getContext(),
+ parseOptionalCIRKeyword<GlobalLinkageKind>(
+ parser, GlobalLinkageKind::ExternalLinkage)));
+
+ ::llvm::StringRef visAttrStr;
+ if (parser.parseOptionalKeyword(&visAttrStr, {"private", "public", "nested"})
+ .succeeded()) {
+ state.addAttribute(visNameAttr,
+ parser.getBuilder().getStringAttr(visAttrStr));
+ }
+
+ cir::VisibilityAttr cirVisibilityAttr;
+ parseVisibilityAttr(parser, cirVisibilityAttr);
+ state.addAttribute(visibilityNameAttr, cirVisibilityAttr);
+
+ if (parser.parseOptionalKeyword(dsoLocalNameAttr).succeeded())
+ state.addAttribute(dsoLocalNameAttr, parser.getBuilder().getUnitAttr());
+
StringAttr nameAttr;
if (parser.parseSymbolName(nameAttr, SymbolTable::getSymbolAttrName(),
state.attributes))
@@ -1347,9 +1419,8 @@ ParseResult cir::FuncOp::parse(OpAsmParser &parser, OperationState &state) {
}
bool cir::FuncOp::isDeclaration() {
- // TODO(CIR): This function will actually do something once external
- // function declarations and aliases are upstreamed.
- return false;
+ assert(!cir::MissingFeatures::opFuncGlobalAliases());
+ return isExternal();
}
mlir::Region *cir::FuncOp::getCallableRegion() {
@@ -1359,6 +1430,25 @@ mlir::Region *cir::FuncOp::getCallableRegion() {
}
void cir::FuncOp::print(OpAsmPrinter &p) {
+ if (getComdat())
+ p << " comdat";
+
+ if (getLinkage() != GlobalLinkageKind::ExternalLinkage)
+ p << ' ' << stringifyGlobalLinkageKind(getLinkage());
+
+ mlir::SymbolTable::Visibility vis = getVisibility();
+ if (vis != mlir::SymbolTable::Visibility::Public)
+ p << ' ' << vis;
+
+ cir::VisibilityAttr cirVisibilityAttr = getGlobalVisibilityAttr();
+ if (!cirVisibilityAttr.isDefault()) {
+ p << ' ';
+ printVisibilityAttr(p, cirVisibilityAttr);
+ }
+
+ if (getDsoLocal())
+ p << " dso_local";
+
p << ' ';
p.printSymbolName(getSymName());
cir::FuncType fnType = getFunctionType();
diff --git a/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp b/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp
index a870e6c45b69d..7e0cdae16e2e8 100644
--- a/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp
+++ b/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp
@@ -97,6 +97,18 @@ static mlir::Value createIntCast(mlir::OpBuilder &bld, mlir::Value src,
return bld.create<mlir::LLVM::BitcastOp>(loc, dstTy, src);
}
+static mlir::LLVM::Visibility
+lowerCIRVisibilityToLLVMVisibility(cir::VisibilityKind visibilityKind) {
+ switch (visibilityKind) {
+ case cir::VisibilityKind::Default:
+ return ::mlir::LLVM::Visibility::Default;
+ case cir::VisibilityKind::Hidden:
+ return ::mlir::LLVM::Visibility::Hidden;
+ case cir::VisibilityKind::Protected:
+ return ::mlir::LLVM::Visibility::Protected;
+ }
+}
+
/// Emits the value from memory as expected by its users. Should be called when
/// the memory represetnation of a CIR type is not equal to its scalar
/// representation.
@@ -1014,9 +1026,12 @@ void CIRToLLVMFuncOpLowering::lowerFuncAttributes(
SmallVectorImpl<mlir::NamedAttribute> &result) const {
assert(!cir::MissingFeatures::opFuncCallingConv());
for (mlir::NamedAttribute attr : func->getAttrs()) {
+ assert(!cir::MissingFeatures::opFuncCallingConv());
if (attr.getName() == mlir::SymbolTable::getSymbolAttrName() ||
attr.getName() == func.getFunctionTypeAttrName() ||
attr.getName() == getLinkageAttrNameString() ||
+ attr.getName() == func.getGlobalVisibilityAttrName() ||
+ attr.getName() == func.getDsoLocalAttrName() ||
(filterArgAndResAttrs &&
(attr.getName() == func.getArgAttrsAttrName() ||
attr.getName() == func.getResAttrsAttrName())))
@@ -1032,8 +1047,7 @@ mlir::LogicalResult CIRToLLVMFuncOpLowering::matchAndRewrite(
mlir::ConversionPatternRewriter &rewriter) const {
cir::FuncType fnType = op.getFunctionType();
- assert(!cir::MissingFeatures::opFuncDsoLocal());
- bool isDsoLocal = false;
+ bool isDsoLocal = op.getDsoLocal();
mlir::TypeConverter::SignatureConversion signatureConversion(
fnType.getNumInputs());
@@ -1061,8 +1075,7 @@ mlir::LogicalResult CIRToLLVMFuncOpLowering::matchAndRew...
[truncated]
|
erichkeane
approved these changes
Jun 24, 2025
| @@ -406,14 +406,34 @@ void CIRGenModule::emitGlobalFunctionDefinition(clang::GlobalDecl gd, | |||
| /*DontDefer=*/true, ForDefinition); | |||
| } | |||
|
|
|||
| // Already emitted. | |||
| if (!funcOp.isDeclaration()) | |||
There was a problem hiding this comment.
So this was a little jarring to me... any chance we could add a isDefinition to cir::FuncOp (or isDefined?) and use that instead? Took me 2-3 reads to figure out What is going on here :)
There was a problem hiding this comment.
This function is part of CIRGlobalValueInterface, which is mirroring llvm::GlobalValue. Classic codegen calls llvm::GlobalValue::isDeclaration() here. In other places, we will eventually have calls to isDeclarationForLinker, which is also a method in llvm::GlobalValue.
I'm on the fence as to whether this is sufficient justification for keeping such a problematic name. The other thing to consider here is how FuncOp::isDeclaration is implemented. Currently is just returns FuncOp::isExternal (which is also a bit obscure in meaning. The incubator implementation also has some handling for the case where the FuncOp is an alias, which is part of the llvm::GlobalValue::isDeclaration implementation.
bool GlobalValue::isDeclaration() const {
// Globals are definitions if they have an initializer.
if (const GlobalVariable *GV = dyn_cast<GlobalVariable>(this))
return GV->getNumOperands() == 0;
// Functions are definitions if they have a body.
if (const Function *F = dyn_cast<Function>(this))
return F->empty() && !F->isMaterializable();
// Aliases and ifuncs are always definitions.
assert(isa<GlobalAlias>(this) || isa<GlobalIFunc>(this));
return false;
}
We haven't implemented ifuncs in the incubator, but when we do they'll need to be handled here. I'm not even sure how we get a function that is materializable, or if that's possible from the clang front end.
It turns out that isExternal is a function provided by some MLIR code we're inheriting and it ends up being resolved to getFunctionBody().empty(). I think the FuncOp::isDeclaration implementation would be greatly improved by just making this check directly.
| case cir::VisibilityKind::Hidden: | ||
| printer << "hidden"; | ||
| break; | ||
| case cir::VisibilityKind::Protected: |
There was a problem hiding this comment.
What other values here are there? Can we skip the default here and just have it try to print all?
There was a problem hiding this comment.
VisibilityKind can be hidden, protected, or default, so I don't think there is any value in printing default. I can make it explicit in the switch and remove the default case though.
There was a problem hiding this comment.
Ah, yes, definitely. By 'try to print all', I should have been more clear, a simple/empty break is expected for some cases. I'd vastly rather that over the default.
Lancern
approved these changes
Jun 25, 2025
| // the LLVM GV (corresponding to our FuncOp) computed, so it doesn't have to | ||
| // recompute it here. This is a minimal fix for now. | ||
| if (!isLocalLinkage(getFunctionLinkage(globalDecl))) { | ||
| const auto *decl = globalDecl.getDecl(); |
| TypeAttrOf<CIR_FuncType>:$function_type, | ||
| UnitAttr:$dso_local, |
There was a problem hiding this comment.
Should we elaborate dso_local from a simple boolean flag into something like a RuntimePreemption enum that could be either NonPreemptable or Preemptable?
There was a problem hiding this comment.
There's a 1-to-1 correspondance between this and the dso_local representation in LLVM IR, and we are able to model the computation of this flag after the same computation in classic codegen. I don't think there's benefit in trying to generalize this as an intermediate state when we'll need to recreate it when lowering to LLVM IR anyway.
| UnitAttr:$dso_local, | ||
| DefaultValuedAttr<CIR_GlobalLinkageKind, | ||
| "cir::GlobalLinkageKind::ExternalLinkage">:$linkage, | ||
| OptionalAttr<StrAttr>:$sym_visibility, |
There was a problem hiding this comment.
I believe that it's used by some general MLIR interfaces.
anthonyhatran
pushed a commit
to anthonyhatran/llvm-project
that referenced
this pull request
Jun 26, 2025
This change adds support for function linkage and visibility and related attributes. Most of the test changes are generalizations to allow 'dso_local' to be accepted where we aren't specifically testing for it. Some tests based on CIR inputs have been updated to add 'private' to function declarations where required by newly supported interfaces. The dso-local.c test has been updated to add specific tests for dso_local being set correctly, and a new test, func-linkage.cpp tests other linkage settings. This change sets `comdat` correctly in CIR, but it is not yet applied to functions when lowering to LLVM IR. That will be handled in a later change.
anthonyhatran
pushed a commit
to anthonyhatran/llvm-project
that referenced
this pull request
Jun 26, 2025
Review llvm#145600 and llvm#145770 crossed, which caused compute-copy and combined-copy tests to fail because of an insufficiently written 'check' line for a cir.func, which didn't account for the linkage spec being added. This patch adds that to fix the build.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change adds support for function linkage and visibility and related attributes. Most of the test changes are generalizations to allow 'dso_local' to be accepted where we aren't specifically testing for it. Some tests based on CIR inputs have been updated to add 'private' to function declarations where required by newly supported interfaces.
The dso-local.c test has been updated to add specific tests for dso_local being set correctly, and a new test, func-linkage.cpp tests other linkage settings.
This change sets
comdatcorrectly in CIR, but it is not yet applied to functions when lowering to LLVM IR. That will be handled in a later change.