[Mlir-commits] [mlir] [mlir][IR] Fix symbol visibility API (PR #218910)
Matthias Springer
llvmlistbot at llvm.org
Wed Aug 26 05:00:17 PDT 2026
https://github.com/matthias-springer created https://github.com/llvm/llvm-project/pull/218910
Fix `SymbolTable::get/setSymbolVisibility`. These functions used to read/write the hard-coded `sym_visibility` attribute, even though the op may store visibility in a different way. The new implementations dispatch to `SymbolOpInterface::get/setVisibility`.
No change in functionality for the interface methods: Their default implementations still read/write `sym_visibility`. (That may change in a follow-up commit.) This commit just aligns the `SymbolTable::get/setSymbolVisibility` API with the op interface.
Also turn `SymbolOpInterface::isNested/setNested/...` into non-overridable helper functions. Users should implement `SymbolOpInterface::get/setVisibility` instead.
>From 776df1e1b0621e7afeda9137a5555f50c6c24d78 Mon Sep 17 00:00:00 2001
From: Matthias Springer <me at m-sp.org>
Date: Wed, 26 Aug 2026 11:49:36 +0000
Subject: [PATCH] [mlir][IR] Fix symbol table visibility API
---
mlir/docs/SymbolsAndSymbolTables.md | 11 ++-
mlir/include/mlir-c/IR.h | 5 +-
mlir/include/mlir/IR/SymbolInterfaces.td | 91 +++++++++++--------
mlir/include/mlir/IR/SymbolTable.h | 16 +++-
mlir/lib/Bindings/Python/IRCore.cpp | 4 +-
mlir/lib/CAPI/IR/IR.cpp | 4 +-
mlir/lib/IR/BuiltinDialect.cpp | 5 +-
mlir/lib/IR/SymbolTable.cpp | 84 ++++++++++-------
.../lib/Interfaces/FunctionImplementation.cpp | 7 +-
mlir/unittests/IR/SymbolTableTest.cpp | 7 ++
10 files changed, 142 insertions(+), 92 deletions(-)
diff --git a/mlir/docs/SymbolsAndSymbolTables.md b/mlir/docs/SymbolsAndSymbolTables.md
index aa7b1a71547bf..4e269d75eb6ea 100644
--- a/mlir/docs/SymbolsAndSymbolTables.md
+++ b/mlir/docs/SymbolsAndSymbolTables.md
@@ -45,10 +45,13 @@ following properties:
* A `StringAttr` attribute named
'SymbolTable::getSymbolAttrName()'(`sym_name`).
- This attribute defines the symbolic 'name' of the operation.
-* An optional `StringAttr` attribute named
- 'SymbolTable::getVisibilityAttrName()'(`sym_visibility`)
- - This attribute defines the [visibility](#symbol-visibility) of the
- symbol, or more specifically in-which scopes it may be accessed.
+* A [visibility](#symbol-visibility) (`getVisibility`/`setVisibility`)
+ - The visibility defines in which scopes the symbol may be accessed.
+ - By default this is stored in an optional `StringAttr` attribute named
+ 'SymbolOpInterface::getDefaultVisibilityAttrName()'(`sym_visibility`),
+ where the absence of the attribute means public visibility. Operations
+ may override `getVisibility`/`setVisibility` to use a different
+ representation.
* No SSA results
- Intermixing the different ways to `use` an operation quickly becomes
unwieldy and difficult to analyze.
diff --git a/mlir/include/mlir-c/IR.h b/mlir/include/mlir-c/IR.h
index 866d90e621384..7e9be93474296 100644
--- a/mlir/include/mlir-c/IR.h
+++ b/mlir/include/mlir-c/IR.h
@@ -1322,9 +1322,10 @@ MLIR_CAPI_EXPORTED MlirStringRef mlirIdentifierStr(MlirIdentifier ident);
/// symbol tables.
MLIR_CAPI_EXPORTED MlirStringRef mlirSymbolTableGetSymbolAttributeName(void);
-/// Returns the name of the attribute used to store symbol visibility.
+/// Returns the name of the attribute used by default to store symbol
+/// visibility.
MLIR_CAPI_EXPORTED MlirStringRef
-mlirSymbolTableGetVisibilityAttributeName(void);
+mlirSymbolTableGetDefaultVisibilityAttributeName(void);
/// Creates a symbol table for the given operation. If the operation does not
/// have the SymbolTable trait, returns a null symbol table.
diff --git a/mlir/include/mlir/IR/SymbolInterfaces.td b/mlir/include/mlir/IR/SymbolInterfaces.td
index ebe0c26637ad3..75b47a99fcd9e 100644
--- a/mlir/include/mlir/IR/SymbolInterfaces.td
+++ b/mlir/include/mlir/IR/SymbolInterfaces.td
@@ -46,52 +46,25 @@ def Symbol : OpInterface<"SymbolOpInterface"> {
mlir::SymbolTable::getSymbolAttrName(), name);
}]
>,
- InterfaceMethod<"Gets the visibility of this symbol.",
+ InterfaceMethod<[{
+ Gets the visibility of this symbol. The default implementation reads
+ the `getDefaultVisibilityAttrName()` attribute and assumes public
+ visibility if it is absent.
+ }],
"mlir::SymbolTable::Visibility", "getVisibility", (ins), [{}],
/*defaultImplementation=*/[{
- return mlir::SymbolTable::getSymbolVisibility(this->getOperation());
- }]
- >,
- InterfaceMethod<"Returns true if this symbol has nested visibility.",
- "bool", "isNested", (ins), [{}],
- /*defaultImplementation=*/[{
- return $_op.getVisibility() == mlir::SymbolTable::Visibility::Nested;
+ return ::mlir::detail::defaultGetSymbolVisibility(
+ this->getOperation());
}]
>,
- InterfaceMethod<"Returns true if this symbol has private visibility.",
- "bool", "isPrivate", (ins), [{}],
- /*defaultImplementation=*/[{
- return $_op.getVisibility() == mlir::SymbolTable::Visibility::Private;
- }]
- >,
- InterfaceMethod<"Returns true if this symbol has public visibility.",
- "bool", "isPublic", (ins), [{}],
- /*defaultImplementation=*/[{
- return $_op.getVisibility() == mlir::SymbolTable::Visibility::Public;
- }]
- >,
- InterfaceMethod<"Sets the visibility of this symbol.",
+ InterfaceMethod<[{
+ Sets the visibility of this symbol. The default implementation writes
+ the `getDefaultVisibilityAttrName()` attribute and drops it for public
+ visibility.
+ }],
"void", "setVisibility", (ins "mlir::SymbolTable::Visibility":$vis), [{}],
/*defaultImplementation=*/[{
- mlir::SymbolTable::setSymbolVisibility(this->getOperation(), vis);
- }]
- >,
- InterfaceMethod<"Sets the visibility of this symbol to be nested.",
- "void", "setNested", (ins), [{}],
- /*defaultImplementation=*/[{
- $_op.setVisibility(mlir::SymbolTable::Visibility::Nested);
- }]
- >,
- InterfaceMethod<"Sets the visibility of this symbol to be private.",
- "void", "setPrivate", (ins), [{}],
- /*defaultImplementation=*/[{
- $_op.setVisibility(mlir::SymbolTable::Visibility::Private);
- }]
- >,
- InterfaceMethod<"Sets the visibility of this symbol to be public.",
- "void", "setPublic", (ins), [{}],
- /*defaultImplementation=*/[{
- $_op.setVisibility(mlir::SymbolTable::Visibility::Public);
+ ::mlir::detail::defaultSetSymbolVisibility(this->getOperation(), vis);
}]
>,
InterfaceMethod<[{
@@ -181,6 +154,14 @@ def Symbol : OpInterface<"SymbolOpInterface"> {
return success();
}];
+ let extraClassDeclaration = [{
+ /// Return the name of the attribute used for symbol visibility by the
+ /// default implementations of `getVisibility` and `setVisibility`.
+ static ::mlir::StringRef getDefaultVisibilityAttrName() {
+ return "sym_visibility";
+ }
+ }];
+
let extraSharedClassDeclaration = [{
using Visibility = mlir::SymbolTable::Visibility;
@@ -193,6 +174,36 @@ def Symbol : OpInterface<"SymbolOpInterface"> {
void setName(::mlir::StringRef name) {
setName(::mlir::StringAttr::get($_op->getContext(), name));
}
+
+ /// Returns true if this symbol has nested visibility.
+ bool isNested() {
+ return $_op.getVisibility() == mlir::SymbolTable::Visibility::Nested;
+ }
+
+ /// Returns true if this symbol has private visibility.
+ bool isPrivate() {
+ return $_op.getVisibility() == mlir::SymbolTable::Visibility::Private;
+ }
+
+ /// Returns true if this symbol has public visibility.
+ bool isPublic() {
+ return $_op.getVisibility() == mlir::SymbolTable::Visibility::Public;
+ }
+
+ /// Sets the visibility of this symbol to be nested.
+ void setNested() {
+ $_op.setVisibility(mlir::SymbolTable::Visibility::Nested);
+ }
+
+ /// Sets the visibility of this symbol to be private.
+ void setPrivate() {
+ $_op.setVisibility(mlir::SymbolTable::Visibility::Private);
+ }
+
+ /// Sets the visibility of this symbol to be public.
+ void setPublic() {
+ $_op.setVisibility(mlir::SymbolTable::Visibility::Public);
+ }
}];
// Add additional classof checks to properly handle "optional" symbols.
diff --git a/mlir/include/mlir/IR/SymbolTable.h b/mlir/include/mlir/IR/SymbolTable.h
index a174062d8d019..9d3fdb98248d5 100644
--- a/mlir/include/mlir/IR/SymbolTable.h
+++ b/mlir/include/mlir/IR/SymbolTable.h
@@ -78,9 +78,6 @@ class SymbolTable {
/// Returns the associated operation.
Operation *getOp() const { return symbolTableOp; }
- /// Return the name of the attribute used for symbol visibility.
- static StringRef getVisibilityAttrName() { return "sym_visibility"; }
-
//===--------------------------------------------------------------------===//
// Symbol Utilities
//===--------------------------------------------------------------------===//
@@ -131,9 +128,11 @@ class SymbolTable {
setSymbolName(symbol, StringAttr::get(symbol->getContext(), name));
}
- /// Returns the visibility of the given symbol operation.
+ /// Returns the visibility of the given symbol operation, which is required to
+ /// implement `SymbolOpInterface`.
static Visibility getSymbolVisibility(Operation *symbol);
- /// Sets the visibility of the given symbol operation.
+ /// Sets the visibility of the given symbol operation, which is required to
+ /// implement `SymbolOpInterface`.
static void setSymbolVisibility(Operation *symbol, Visibility vis);
/// Returns the nearest symbol table from a given operation `from`. Returns
@@ -438,6 +437,13 @@ class SymbolUserMap {
namespace detail {
LogicalResult verifySymbolTable(Operation *op);
LogicalResult verifySymbol(Operation *op);
+
+/// Default implementations of `SymbolOpInterface::getVisibility` and
+/// `SymbolOpInterface::setVisibility`, which keep the visibility in the
+/// `SymbolOpInterface::getDefaultVisibilityAttrName()` attribute. Public
+/// visibility is represented by the absence of that attribute.
+SymbolTable::Visibility defaultGetSymbolVisibility(Operation *symbol);
+void defaultSetSymbolVisibility(Operation *symbol, SymbolTable::Visibility vis);
} // namespace detail
namespace OpTrait {
diff --git a/mlir/lib/Bindings/Python/IRCore.cpp b/mlir/lib/Bindings/Python/IRCore.cpp
index 0adad4a349478..b6d92dc8064a2 100644
--- a/mlir/lib/Bindings/Python/IRCore.cpp
+++ b/mlir/lib/Bindings/Python/IRCore.cpp
@@ -2104,7 +2104,7 @@ void PySymbolTable::setSymbolName(PyOperationBase &symbol,
PyStringAttribute PySymbolTable::getVisibility(PyOperationBase &symbol) {
PyOperation &operation = symbol.getOperation();
operation.checkValid();
- MlirStringRef attrName = mlirSymbolTableGetVisibilityAttributeName();
+ MlirStringRef attrName = mlirSymbolTableGetDefaultVisibilityAttributeName();
MlirAttribute existingVisAttr =
mlirOperationGetAttributeByName(operation.get(), attrName);
if (mlirAttributeIsNull(existingVisAttr))
@@ -2120,7 +2120,7 @@ void PySymbolTable::setVisibility(PyOperationBase &symbol,
"Expected visibility to be 'public', 'private' or 'nested'");
PyOperation &operation = symbol.getOperation();
operation.checkValid();
- MlirStringRef attrName = mlirSymbolTableGetVisibilityAttributeName();
+ MlirStringRef attrName = mlirSymbolTableGetDefaultVisibilityAttributeName();
MlirAttribute existingVisAttr =
mlirOperationGetAttributeByName(operation.get(), attrName);
if (mlirAttributeIsNull(existingVisAttr))
diff --git a/mlir/lib/CAPI/IR/IR.cpp b/mlir/lib/CAPI/IR/IR.cpp
index f862efa420daa..3e11efb151b9f 100644
--- a/mlir/lib/CAPI/IR/IR.cpp
+++ b/mlir/lib/CAPI/IR/IR.cpp
@@ -1406,8 +1406,8 @@ MlirStringRef mlirSymbolTableGetSymbolAttributeName() {
return wrap(SymbolTable::getSymbolAttrName());
}
-MlirStringRef mlirSymbolTableGetVisibilityAttributeName() {
- return wrap(SymbolTable::getVisibilityAttrName());
+MlirStringRef mlirSymbolTableGetDefaultVisibilityAttributeName() {
+ return wrap(SymbolOpInterface::getDefaultVisibilityAttrName());
}
MlirSymbolTable mlirSymbolTableCreate(MlirOperation operation) {
diff --git a/mlir/lib/IR/BuiltinDialect.cpp b/mlir/lib/IR/BuiltinDialect.cpp
index c88b328282275..e795b3ceedbdd 100644
--- a/mlir/lib/IR/BuiltinDialect.cpp
+++ b/mlir/lib/IR/BuiltinDialect.cpp
@@ -161,8 +161,9 @@ LogicalResult ModuleOp::verify() {
for (auto attr : (*this)->getAttrs()) {
if (!attr.getName().strref().contains('.') &&
!llvm::is_contained(
- ArrayRef<StringRef>{mlir::SymbolTable::getSymbolAttrName(),
- mlir::SymbolTable::getVisibilityAttrName()},
+ ArrayRef<StringRef>{
+ mlir::SymbolTable::getSymbolAttrName(),
+ mlir::SymbolOpInterface::getDefaultVisibilityAttrName()},
attr.getName().strref()))
return emitOpError() << "can only contain attributes with "
"dialect-prefixed names, found: '"
diff --git a/mlir/lib/IR/SymbolTable.cpp b/mlir/lib/IR/SymbolTable.cpp
index 078401c8380f4..cdf5a7d7c1469 100644
--- a/mlir/lib/IR/SymbolTable.cpp
+++ b/mlir/lib/IR/SymbolTable.cpp
@@ -304,34 +304,15 @@ void SymbolTable::setSymbolName(Operation *symbol, StringAttr name) {
/// Returns the visibility of the given symbol operation.
SymbolTable::Visibility SymbolTable::getSymbolVisibility(Operation *symbol) {
- // If the attribute doesn't exist, assume public.
- StringAttr vis = symbol->getAttrOfType<StringAttr>(getVisibilityAttrName());
- if (!vis)
- return Visibility::Public;
-
- // Otherwise, switch on the string value.
- return StringSwitch<Visibility>(vis.getValue())
- .Case("private", Visibility::Private)
- .Case("nested", Visibility::Nested)
- .Case("public", Visibility::Public);
+ auto symbolOp = dyn_cast<SymbolOpInterface>(symbol);
+ assert(symbolOp && "expected valid symbol operation");
+ return symbolOp.getVisibility();
}
/// Sets the visibility of the given symbol operation.
void SymbolTable::setSymbolVisibility(Operation *symbol, Visibility vis) {
- MLIRContext *ctx = symbol->getContext();
-
- // If the visibility is public, just drop the attribute as this is the
- // default.
- if (vis == Visibility::Public) {
- symbol->removeAttr(StringAttr::get(ctx, getVisibilityAttrName()));
- return;
- }
-
- // Otherwise, update the attribute.
- assert((vis == Visibility::Private || vis == Visibility::Nested) &&
- "unknown symbol visibility kind");
-
- StringRef visName = vis == Visibility::Private ? "private" : "nested";
- symbol->setAttr(getVisibilityAttrName(), StringAttr::get(ctx, visName));
+ auto symbolOp = dyn_cast<SymbolOpInterface>(symbol);
+ assert(symbolOp && "expected valid symbol operation");
+ symbolOp.setVisibility(vis);
}
/// Returns the nearest symbol table from a given operation `from`. Returns
@@ -427,9 +408,11 @@ static LogicalResult lookupSymbolInImpl(
if (!symbolOp->hasTrait<OpTrait::SymbolTable>())
return failure();
symbolOp = lookupSymbolFn(symbolOp, ref.getAttr());
+ if (!symbolOp)
+ return failure();
// If the nested symbol is private, lookup failed.
- if (!symbolOp || SymbolTable::getSymbolVisibility(symbolOp) ==
- SymbolTable::Visibility::Private)
+ auto nestedSymbol = dyn_cast<SymbolOpInterface>(symbolOp);
+ if (nestedSymbol && nestedSymbol.isPrivate())
return failure();
symbols.push_back(symbolOp);
}
@@ -531,12 +514,14 @@ LogicalResult detail::verifySymbol(Operation *op) {
<< mlir::SymbolTable::getSymbolAttrName() << "'";
// Verify the visibility attribute.
- if (Attribute vis = op->getAttr(mlir::SymbolTable::getVisibilityAttrName())) {
+ StringRef visAttrName =
+ mlir::SymbolOpInterface::getDefaultVisibilityAttrName();
+ if (Attribute vis = op->getAttr(visAttrName)) {
StringAttr visStrAttr = llvm::dyn_cast<StringAttr>(vis);
if (!visStrAttr)
- return op->emitOpError() << "requires visibility attribute '"
- << mlir::SymbolTable::getVisibilityAttrName()
- << "' to be a string attribute, but got " << vis;
+ return op->emitOpError()
+ << "requires visibility attribute '" << visAttrName
+ << "' to be a string attribute, but got " << vis;
if (!llvm::is_contained(ArrayRef<StringRef>{"public", "private", "nested"},
visStrAttr.getValue()))
@@ -548,6 +533,41 @@ LogicalResult detail::verifySymbol(Operation *op) {
return success();
}
+SymbolTable::Visibility detail::defaultGetSymbolVisibility(Operation *symbol) {
+ StringAttr vis = symbol->getAttrOfType<StringAttr>(
+ SymbolOpInterface::getDefaultVisibilityAttrName());
+ // If the attribute doesn't exist, assume public.
+ if (!vis)
+ return SymbolTable::Visibility::Public;
+
+ // Otherwise, switch on the string value.
+ return StringSwitch<SymbolTable::Visibility>(vis.getValue())
+ .Case("private", SymbolTable::Visibility::Private)
+ .Case("nested", SymbolTable::Visibility::Nested)
+ .Case("public", SymbolTable::Visibility::Public);
+}
+
+void detail::defaultSetSymbolVisibility(Operation *symbol,
+ SymbolTable::Visibility vis) {
+ StringRef attrName = SymbolOpInterface::getDefaultVisibilityAttrName();
+
+ // If the visibility is public, just drop the attribute as this is the
+ // default.
+ if (vis == SymbolTable::Visibility::Public) {
+ symbol->removeAttr(attrName);
+ return;
+ }
+
+ // Otherwise, update the attribute.
+ assert((vis == SymbolTable::Visibility::Private ||
+ vis == SymbolTable::Visibility::Nested) &&
+ "unknown symbol visibility kind");
+
+ StringRef visName =
+ vis == SymbolTable::Visibility::Private ? "private" : "nested";
+ symbol->setAttr(attrName, StringAttr::get(symbol->getContext(), visName));
+}
+
//===----------------------------------------------------------------------===//
// Symbol Use Lists
//===----------------------------------------------------------------------===//
@@ -1126,7 +1146,7 @@ ParseResult impl::parseOptionalVisibilityKeyword(OpAsmParser &parser,
StringAttr visibilityAttr = parser.getBuilder().getStringAttr(visibility);
attrs.push_back(parser.getBuilder().getNamedAttr(
- SymbolTable::getVisibilityAttrName(), visibilityAttr));
+ SymbolOpInterface::getDefaultVisibilityAttrName(), visibilityAttr));
return success();
}
diff --git a/mlir/lib/Interfaces/FunctionImplementation.cpp b/mlir/lib/Interfaces/FunctionImplementation.cpp
index 90f32896e8181..76380aa3d8f3c 100644
--- a/mlir/lib/Interfaces/FunctionImplementation.cpp
+++ b/mlir/lib/Interfaces/FunctionImplementation.cpp
@@ -131,8 +131,8 @@ ParseResult function_interface_impl::parseFunctionOp(
// Disallow attributes that are inferred from elsewhere in the attribute
// dictionary.
for (StringRef disallowed :
- {SymbolTable::getVisibilityAttrName(), SymbolTable::getSymbolAttrName(),
- typeAttrName.getValue()}) {
+ {SymbolOpInterface::getDefaultVisibilityAttrName(),
+ SymbolTable::getSymbolAttrName(), typeAttrName.getValue()}) {
if (parsedAttributes.get(disallowed))
return parser.emitError(attributeDictLocation, "'")
<< disallowed
@@ -181,7 +181,8 @@ void function_interface_impl::printFunctionOp(
.getValue();
p << ' ';
- StringRef visibilityAttrName = SymbolTable::getVisibilityAttrName();
+ StringRef visibilityAttrName =
+ SymbolOpInterface::getDefaultVisibilityAttrName();
if (auto visibility = op->getAttrOfType<StringAttr>(visibilityAttrName))
p << visibility.getValue() << ' ';
p.printSymbolName(funcName);
diff --git a/mlir/unittests/IR/SymbolTableTest.cpp b/mlir/unittests/IR/SymbolTableTest.cpp
index 864eb40898335..ee3e726b3855b 100644
--- a/mlir/unittests/IR/SymbolTableTest.cpp
+++ b/mlir/unittests/IR/SymbolTableTest.cpp
@@ -147,6 +147,8 @@ TEST(SymbolOpInterface, Visibility) {
ASSERT_FALSE(symOp.isPublic());
ASSERT_FALSE(symOp.isNested());
ASSERT_TRUE(symOp.canDiscardOnUseEmpty());
+ ASSERT_EQ(SymbolTable::getSymbolVisibility(symOp),
+ SymbolTable::Visibility::Private);
std::string diagStr;
context.getDiagEngine().registerHandler(
@@ -162,8 +164,13 @@ TEST(SymbolOpInterface, Visibility) {
symOp.setPrivate();
expectedDiag += "'test.overridden_symbol_visibility' op cannot change "
"visibility of symbol to private";
+ SymbolTable::setSymbolVisibility(symOp, SymbolTable::Visibility::Nested);
+ expectedDiag += "'test.overridden_symbol_visibility' op cannot change "
+ "visibility of symbol to nested";
ASSERT_EQ(diagStr, expectedDiag);
+ ASSERT_FALSE(
+ symOp->hasAttr(SymbolOpInterface::getDefaultVisibilityAttrName()));
}
} // namespace
More information about the Mlir-commits
mailing list