[flang-commits] [flang] [flang] Trust the lowering symbol table in the FirOpBuilder lookups (PR #232678)
Eugene Epshteyn via flang-commits
flang-commits at lists.llvm.org
Wed Oct 21 09:48:07 PDT 2026
https://github.com/eugeneepshteyn updated https://github.com/llvm/llvm-project/pull/232678
>From 5e6ce79b33d9a5ba895fdfb958a776599369806f Mon Sep 17 00:00:00 2001
From: Eugene Epshteyn <eepshteyn at nvidia.com>
Date: Thu, 15 Oct 2026 23:17:48 -0700
Subject: [PATCH] [flang] Trust the lowering symbol table in the FirOpBuilder
lookups
FirOpBuilder::getNamedFunction, getNamedGlobal, fir::createFuncOp,
fir::createGlobalOp and fir::lookupTypeInfoOp consult the mlir::SymbolTable
the bridge maintains, but on a miss they fall back to ModuleOp::lookupSymbol,
a linear walk of the module body. Every new global is a miss, so lowering N
globals walks the module N times. A 12-line subroutine passing a TYPE(TOP)
dummy whose module defines 1000 derived types spends 7.7 s of a 20.1 s
compile in lowering, and a profile puts 29% of all samples under
getNamedGlobal, nearly all of them in the walk (the unit carries about 25
linkonce_odr type-info globals per type).
The fallback existed because the table was not guaranteed to be complete:
fir::createFuncOp took the table by const pointer and never inserted the
function it created, so every runtime, intrinsic and procedure declaration
bypassed it, and three sites in OpenACC and OpenMP lowering created func.func
operations directly. Make the table complete instead of working around it:
createFuncOp and createGlobalOp take a non-const table and register what
they create, the three direct creations register through the new
fir::registerSymbol, the sham function of createBuilderOutsideOfFuncOpAndDo
is erased through the table, and the lookups then return the table's answer
alone. registerSymbol never renames on a clash, unlike SymbolTable::insert,
so a duplicate definition still reaches the verifier the way it does today.
getNamedFunction(SymbolRefAttr) resolves a nested reference through the
module rather than looking its leaf up in the top-level table.
Under EXPENSIVE_CHECKS every trusted lookup asserts that the module walk
agrees, on hits and on misses. A local build with those checks enabled
unconditionally passes check-flang.
With this change the same unit lowers in 0.2 s and the whole compile takes
12.6 s instead of 20.1 s (measured on top of #232555; check-flang unchanged).
---
.../flang/Optimizer/Builder/FIRBuilder.h | 9 +++-
.../flang/Optimizer/Dialect/FIROpsSupport.h | 25 +++++++---
flang/include/flang/Optimizer/Support/Utils.h | 3 +-
flang/lib/Lower/Bridge.cpp | 15 ++++--
flang/lib/Lower/OpenACC.cpp | 10 ++--
flang/lib/Lower/OpenMP/ClauseProcessor.cpp | 3 +-
flang/lib/Optimizer/Builder/CUFCommon.cpp | 5 +-
flang/lib/Optimizer/Builder/FIRBuilder.cpp | 49 +++++++++----------
flang/lib/Optimizer/Dialect/FIROps.cpp | 46 +++++++++++------
flang/lib/Optimizer/Support/Utils.cpp | 11 +++--
10 files changed, 109 insertions(+), 67 deletions(-)
diff --git a/flang/include/flang/Optimizer/Builder/FIRBuilder.h b/flang/include/flang/Optimizer/Builder/FIRBuilder.h
index 3857d2b73c4ee1..92ec7ba14272a0 100644
--- a/flang/include/flang/Optimizer/Builder/FIRBuilder.h
+++ b/flang/include/flang/Optimizer/Builder/FIRBuilder.h
@@ -132,7 +132,11 @@ class FirOpBuilder : public mlir::OpBuilder, public mlir::OpBuilder::Listener {
/// Get a reference to the kind map.
const fir::KindMapping &getKindMap() { return kindMap; }
- /// Get func.func/fir.global symbol table attached to this builder if any.
+ /// Get the func.func/fir.global/fir.type_info symbol table attached to this
+ /// builder if any. The lookups below trust it, so it must contain every
+ /// symbol operation of the module: creating symbols through this builder
+ /// (createFunction, createGlobal, createTypeInfoOp) keeps it complete, any
+ /// other creation must call fir::registerSymbol.
mlir::SymbolTable *getMLIRSymbolTable() { return symbolTable; }
/// Get the default integer type
@@ -418,7 +422,8 @@ class FirOpBuilder : public mlir::OpBuilder, public mlir::OpBuilder::Listener {
}
/// Get a function by name. If the function exists in the current module, it
- /// is returned. Otherwise, a null FuncOp is returned.
+ /// is returned. Otherwise, a null FuncOp is returned. When a symbol table is
+ /// available, the answer comes from it alone (see getMLIRSymbolTable).
mlir::func::FuncOp getNamedFunction(llvm::StringRef name) {
return getNamedFunction(getModule(), getMLIRSymbolTable(), name);
}
diff --git a/flang/include/flang/Optimizer/Dialect/FIROpsSupport.h b/flang/include/flang/Optimizer/Dialect/FIROpsSupport.h
index d5131500ecbbd7..2e3e3a4b235e40 100644
--- a/flang/include/flang/Optimizer/Dialect/FIROpsSupport.h
+++ b/flang/include/flang/Optimizer/Dialect/FIROpsSupport.h
@@ -66,19 +66,32 @@ inline bool pureCall(mlir::Operation *op) {
/// Get or create a FuncOp in a module.
///
/// If `module` already contains FuncOp `name`, it is returned. Otherwise, a new
-/// FuncOp is created, and that new FuncOp is returned. A symbol table can
-/// be provided to speed-up the lookups.
+/// FuncOp is created, and that new FuncOp is returned.
+///
+/// If \p symbolTable is provided, it must be the symbol table of \p module and
+/// it must be complete: the lookup trusts it, and a miss is not re-checked by
+/// walking the module. A function created here is inserted into it so that it
+/// stays complete.
mlir::func::FuncOp createFuncOp(mlir::Location loc, mlir::ModuleOp module,
llvm::StringRef name, mlir::FunctionType type,
llvm::ArrayRef<mlir::NamedAttribute> attrs = {},
- const mlir::SymbolTable *symbolTable = nullptr);
+ mlir::SymbolTable *symbolTable = nullptr);
-/// Get or create a GlobalOp in a module. A symbol table can be provided to
-/// speed-up the lookups.
+/// Get or create a GlobalOp in a module. \p symbolTable has the same contract
+/// as in createFuncOp.
fir::GlobalOp createGlobalOp(mlir::Location loc, mlir::ModuleOp module,
llvm::StringRef name, mlir::Type type,
llvm::ArrayRef<mlir::NamedAttribute> attrs = {},
- const mlir::SymbolTable *symbolTable = nullptr);
+ mlir::SymbolTable *symbolTable = nullptr);
+
+/// Register \p symbol in \p symbolTable if it is not null. \p symbol must
+/// already be in the body of the operation the table was built for. Call this
+/// for every symbol operation that is created without createFuncOp,
+/// createGlobalOp or the FirOpBuilder helpers, so that a symbol table handed
+/// to the lookup helpers stays complete. Unlike mlir::SymbolTable::insert, it
+/// never renames \p symbol: a name clash is a duplicate definition that the
+/// verifier reports, and renaming would hide it.
+void registerSymbol(mlir::SymbolTable *symbolTable, mlir::Operation *symbol);
/// Attribute to mark Fortran entities with the CONTIGUOUS attribute.
constexpr llvm::StringRef getContiguousAttrName() { return "fir.contiguous"; }
diff --git a/flang/include/flang/Optimizer/Support/Utils.h b/flang/include/flang/Optimizer/Support/Utils.h
index b7726fb7440b1f..f7850115296ab1 100644
--- a/flang/include/flang/Optimizer/Support/Utils.h
+++ b/flang/include/flang/Optimizer/Support/Utils.h
@@ -199,7 +199,8 @@ mlirTypeToCategoryKind(mlir::Location loc, mlir::Type type) {
}
/// Find the fir.type_info that was created for this \p recordType in \p module,
-/// if any. \p symbolTable can be provided to speed-up the lookup. This tool
+/// if any. \p symbolTable can be provided to speed-up the lookup; it is
+/// trusted, so it must contain every fir.type_info of \p module. This tool
/// will match record type even if they have been "altered" in type conversion
/// passes.
fir::TypeInfoOp
diff --git a/flang/lib/Lower/Bridge.cpp b/flang/lib/Lower/Bridge.cpp
index e52e690ab4b64f..035b529f776785 100644
--- a/flang/lib/Lower/Bridge.cpp
+++ b/flang/lib/Lower/Bridge.cpp
@@ -6834,7 +6834,12 @@ class FirConverter : public Fortran::lower::AbstractConverter {
createGlobals();
if (mlir::Region *region = func.getCallableRegion())
region->dropAllReferences();
- func.erase();
+ // createFunction registered the sham function in the symbol table: erase
+ // it through the table so that no dangling entry is left behind.
+ if (symbolTable)
+ symbolTable->erase(func);
+ else
+ func.erase();
delete builder;
builder = nullptr;
localSymbols.clear();
@@ -7254,10 +7259,10 @@ class FirConverter : public Fortran::lower::AbstractConverter {
/// utilities to deal with procedure pointer components whose arguments have
/// the type of the containing derived type.
Fortran::lower::TypeConstructionStack typeConstructionStack;
- /// MLIR symbol table of the fir.global/func.func operations. Note that it is
- /// not guaranteed to contain all operations of the ModuleOp with Symbol
- /// attribute since mlirSymbolTable must pro-actively be maintained when
- /// new Symbol operations are created.
+ /// MLIR symbol table of the fir.global/func.func/fir.type_info operations.
+ /// The FirOpBuilder lookups trust it, so it must pro-actively be maintained:
+ /// every one of these operations created during lowering must go through the
+ /// FirOpBuilder creation helpers or be registered with fir::registerSymbol.
mlir::SymbolTable mlirSymbolTable;
/// Used to store context while recursing into regions during lowering.
diff --git a/flang/lib/Lower/OpenACC.cpp b/flang/lib/Lower/OpenACC.cpp
index eec08190aa6a1e..2fe609f327eb52 100644
--- a/flang/lib/Lower/OpenACC.cpp
+++ b/flang/lib/Lower/OpenACC.cpp
@@ -207,6 +207,7 @@ static mlir::func::FuncOp createDeclareFunc(
auto funcOp = mlir::func::FuncOp::create(modBuilder, loc, funcName, funcTy);
funcOp.setVisibility(linkable ? mlir::SymbolTable::Visibility::Public
: mlir::SymbolTable::Visibility::Private);
+ fir::registerSymbol(builder.getMLIRSymbolTable(), funcOp);
builder.createBlock(&funcOp.getRegion(), funcOp.getRegion().end(), argsTy,
locs);
builder.setInsertionPointToEnd(&funcOp.getRegion().back());
@@ -5335,19 +5336,14 @@ void Fortran::lower::declareExternalAccModuleDeclareActionRecipes(
if (isUnifiedMemoryMode(converter))
return;
- mlir::ModuleOp module = builder.getModule();
mlir::Location loc = converter.genLocation(sym.name());
std::string prefix = converter.mangleName(ultimate);
auto declareExternalRecipe = [&](llvm::StringRef suffix) {
std::string name = prefix + suffix.str();
- if (module.lookupSymbol<mlir::func::FuncOp>(name))
- return;
auto funcTy = mlir::FunctionType::get(builder.getContext(), {}, {});
- mlir::OpBuilder modBuilder(module.getBodyRegion());
- modBuilder.setInsertionPointToEnd(module.getBody());
- auto funcOp = mlir::func::FuncOp::create(modBuilder, loc, name, funcTy);
- funcOp.setVisibility(mlir::SymbolTable::Visibility::Private);
+ // Get-or-create: a private declaration at the end of the module.
+ builder.createFunction(loc, name, funcTy);
};
declareExternalRecipe(declarePostAllocSuffix);
diff --git a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
index b1f29bb23361f0..78ae41dbdf31e3 100644
--- a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
+++ b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
@@ -1287,7 +1287,7 @@ createCopyFunc(mlir::Location loc, lower::AbstractConverter &converter,
std::string copyFuncName =
fir::getTypeAsString(varType, builder.getKindMap(), "_copy");
- if (auto decl = module.lookupSymbol<mlir::func::FuncOp>(copyFuncName))
+ if (auto decl = builder.getNamedFunction(copyFuncName))
return decl;
// create function
@@ -1298,6 +1298,7 @@ createCopyFunc(mlir::Location loc, lower::AbstractConverter &converter,
mlir::func::FuncOp funcOp =
mlir::func::FuncOp::create(modBuilder, loc, copyFuncName, funcType);
funcOp.setVisibility(mlir::SymbolTable::Visibility::Private);
+ fir::registerSymbol(builder.getMLIRSymbolTable(), funcOp);
fir::factory::setInternalLinkage(funcOp);
builder.createBlock(&funcOp.getRegion(), funcOp.getRegion().end(), argsTy,
{loc, loc});
diff --git a/flang/lib/Optimizer/Builder/CUFCommon.cpp b/flang/lib/Optimizer/Builder/CUFCommon.cpp
index 389a46de600370..defbbd940b1f74 100644
--- a/flang/lib/Optimizer/Builder/CUFCommon.cpp
+++ b/flang/lib/Optimizer/Builder/CUFCommon.cpp
@@ -132,9 +132,8 @@ bool cuf::isTypeDescriptorGlobal(fir::GlobalOp op) {
void cuf::genPointerSync(const mlir::Value box, fir::FirOpBuilder &builder) {
if (auto declareOp = box.getDefiningOp<hlfir::DeclareOp>()) {
if (auto addrOfOp = declareOp.getMemref().getDefiningOp<fir::AddrOfOp>()) {
- auto mod = addrOfOp->getParentOfType<mlir::ModuleOp>();
- if (auto globalOp =
- mod.lookupSymbol<fir::GlobalOp>(addrOfOp.getSymbol())) {
+ if (auto globalOp = builder.getNamedGlobal(
+ addrOfOp.getSymbol().getLeafReference().getValue())) {
if (cuf::isRegisteredDeviceGlobal(globalOp)) {
cuf::SyncDescriptorOp::create(builder, box.getLoc(),
addrOfOp.getSymbol());
diff --git a/flang/lib/Optimizer/Builder/FIRBuilder.cpp b/flang/lib/Optimizer/Builder/FIRBuilder.cpp
index 7f03f3f050d5fd..0ee8bcb5f4020e 100644
--- a/flang/lib/Optimizer/Builder/FIRBuilder.cpp
+++ b/flang/lib/Optimizer/Builder/FIRBuilder.cpp
@@ -67,14 +67,14 @@ mlir::func::FuncOp
fir::FirOpBuilder::getNamedFunction(mlir::ModuleOp modOp,
const mlir::SymbolTable *symbolTable,
llvm::StringRef name) {
- if (symbolTable)
- if (auto func = symbolTable->lookup<mlir::func::FuncOp>(name)) {
+ if (symbolTable) {
+ auto func = symbolTable->lookup<mlir::func::FuncOp>(name);
#ifdef EXPENSIVE_CHECKS
- assert(func == modOp.lookupSymbol<mlir::func::FuncOp>(name) &&
- "symbolTable and module out of sync");
+ assert(func == modOp.lookupSymbol<mlir::func::FuncOp>(name) &&
+ "symbolTable and module out of sync");
#endif
- return func;
- }
+ return func;
+ }
return modOp.lookupSymbol<mlir::func::FuncOp>(name);
}
@@ -82,15 +82,17 @@ mlir::func::FuncOp
fir::FirOpBuilder::getNamedFunction(mlir::ModuleOp modOp,
const mlir::SymbolTable *symbolTable,
mlir::SymbolRefAttr symbol) {
- if (symbolTable)
- if (auto func = symbolTable->lookup<mlir::func::FuncOp>(
- symbol.getLeafReference())) {
+ // The symbol table only covers the module's own symbols: a nested reference
+ // must be resolved through the nested symbol tables.
+ if (symbolTable && symbol.getNestedReferences().empty()) {
+ auto func =
+ symbolTable->lookup<mlir::func::FuncOp>(symbol.getRootReference());
#ifdef EXPENSIVE_CHECKS
- assert(func == modOp.lookupSymbol<mlir::func::FuncOp>(symbol) &&
- "symbolTable and module out of sync");
+ assert(func == modOp.lookupSymbol<mlir::func::FuncOp>(symbol) &&
+ "symbolTable and module out of sync");
#endif
- return func;
- }
+ return func;
+ }
return modOp.lookupSymbol<mlir::func::FuncOp>(symbol);
}
@@ -98,14 +100,14 @@ fir::GlobalOp
fir::FirOpBuilder::getNamedGlobal(mlir::ModuleOp modOp,
const mlir::SymbolTable *symbolTable,
llvm::StringRef name) {
- if (symbolTable)
- if (auto global = symbolTable->lookup<fir::GlobalOp>(name)) {
+ if (symbolTable) {
+ auto global = symbolTable->lookup<fir::GlobalOp>(name);
#ifdef EXPENSIVE_CHECKS
- assert(global == modOp.lookupSymbol<fir::GlobalOp>(name) &&
- "symbolTable and module out of sync");
+ assert(global == modOp.lookupSymbol<fir::GlobalOp>(name) &&
+ "symbolTable and module out of sync");
#endif
- return global;
- }
+ return global;
+ }
return modOp.lookupSymbol<fir::GlobalOp>(name);
}
@@ -501,8 +503,7 @@ fir::GlobalOp fir::FirOpBuilder::createGlobal(
if (setDefaultAlignment && mlir::isa<fir::SequenceType>(type))
glob.setAlignment(fir::defaultArrayGlobalAlignment);
restoreInsertionPoint(insertPt);
- if (symbolTable)
- symbolTable->insert(glob);
+ fir::registerSymbol(symbolTable, glob);
return glob;
}
@@ -527,8 +528,7 @@ fir::GlobalOp fir::FirOpBuilder::createGlobal(
setInsertionPointToStart(&block);
bodyBuilder(*this);
restoreInsertionPoint(insertPt);
- if (symbolTable)
- symbolTable->insert(glob);
+ fir::registerSymbol(symbolTable, glob);
return glob;
}
@@ -543,8 +543,7 @@ fir::FirOpBuilder::createTypeInfoOp(mlir::Location loc,
InsertPoint insertPoint = saveInsertionPoint();
setInsertionPoint(module.getBody(), module.getBody()->end());
auto typeInfo = fir::TypeInfoOp::create(*this, loc, recordType, parentType);
- if (symbolTable)
- symbolTable->insert(typeInfo);
+ fir::registerSymbol(symbolTable, typeInfo);
return {typeInfo, insertPoint};
}
diff --git a/flang/lib/Optimizer/Dialect/FIROps.cpp b/flang/lib/Optimizer/Dialect/FIROps.cpp
index 606d47631488bb..e59444266bc233 100644
--- a/flang/lib/Optimizer/Dialect/FIROps.cpp
+++ b/flang/lib/Optimizer/Dialect/FIROps.cpp
@@ -6001,45 +6001,63 @@ fir::parseSelector(mlir::OpAsmParser &parser, mlir::OperationState &result,
return mlir::success();
}
+void fir::registerSymbol(mlir::SymbolTable *symbolTable,
+ mlir::Operation *symbol) {
+ if (!symbolTable)
+ return;
+ mlir::StringAttr name = mlir::SymbolTable::getSymbolName(symbol);
+ if (mlir::Operation *existing = symbolTable->lookup(name)) {
+ // SymbolTable::insert would rename the new operation. Leave the duplicate
+ // definition in place for the verifier to report instead.
+ assert(existing == symbol && "duplicate symbol definition");
+ return;
+ }
+ symbolTable->insert(symbol);
+}
+
mlir::func::FuncOp fir::createFuncOp(mlir::Location loc, mlir::ModuleOp module,
llvm::StringRef name,
mlir::FunctionType type,
llvm::ArrayRef<mlir::NamedAttribute> attrs,
- const mlir::SymbolTable *symbolTable) {
- if (symbolTable)
- if (auto f = symbolTable->lookup<mlir::func::FuncOp>(name)) {
+ mlir::SymbolTable *symbolTable) {
+ if (symbolTable) {
+ auto f = symbolTable->lookup<mlir::func::FuncOp>(name);
#ifdef EXPENSIVE_CHECKS
- assert(f == module.lookupSymbol<mlir::func::FuncOp>(name) &&
- "symbolTable and module out of sync");
+ assert(f == module.lookupSymbol<mlir::func::FuncOp>(name) &&
+ "symbolTable and module out of sync");
#endif
+ if (f)
return f;
- }
- if (auto f = module.lookupSymbol<mlir::func::FuncOp>(name))
+ } else if (auto f = module.lookupSymbol<mlir::func::FuncOp>(name)) {
return f;
+ }
mlir::OpBuilder modBuilder(module.getBodyRegion());
modBuilder.setInsertionPointToEnd(module.getBody());
auto result = mlir::func::FuncOp::create(modBuilder, loc, name, type, attrs);
result.setVisibility(mlir::SymbolTable::Visibility::Private);
+ fir::registerSymbol(symbolTable, result);
return result;
}
fir::GlobalOp fir::createGlobalOp(mlir::Location loc, mlir::ModuleOp module,
llvm::StringRef name, mlir::Type type,
llvm::ArrayRef<mlir::NamedAttribute> attrs,
- const mlir::SymbolTable *symbolTable) {
- if (symbolTable)
- if (auto g = symbolTable->lookup<fir::GlobalOp>(name)) {
+ mlir::SymbolTable *symbolTable) {
+ if (symbolTable) {
+ auto g = symbolTable->lookup<fir::GlobalOp>(name);
#ifdef EXPENSIVE_CHECKS
- assert(g == module.lookupSymbol<fir::GlobalOp>(name) &&
- "symbolTable and module out of sync");
+ assert(g == module.lookupSymbol<fir::GlobalOp>(name) &&
+ "symbolTable and module out of sync");
#endif
+ if (g)
return g;
- }
- if (auto g = module.lookupSymbol<fir::GlobalOp>(name))
+ } else if (auto g = module.lookupSymbol<fir::GlobalOp>(name)) {
return g;
+ }
mlir::OpBuilder modBuilder(module.getBodyRegion());
auto result = fir::GlobalOp::create(modBuilder, loc, name, type, attrs);
result.setVisibility(mlir::SymbolTable::Visibility::Private);
+ fir::registerSymbol(symbolTable, result);
return result;
}
diff --git a/flang/lib/Optimizer/Support/Utils.cpp b/flang/lib/Optimizer/Support/Utils.cpp
index 93dc0c569a97d1..ebb18060fe9f16 100644
--- a/flang/lib/Optimizer/Support/Utils.cpp
+++ b/flang/lib/Optimizer/Support/Utils.cpp
@@ -32,9 +32,14 @@ fir::TypeInfoOp fir::lookupTypeInfoOp(fir::RecordType recordType,
fir::TypeInfoOp fir::lookupTypeInfoOp(llvm::StringRef name,
mlir::ModuleOp module,
const mlir::SymbolTable *symbolTable) {
- if (symbolTable)
- if (auto typeInfo = symbolTable->lookup<fir::TypeInfoOp>(name))
- return typeInfo;
+ if (symbolTable) {
+ auto typeInfo = symbolTable->lookup<fir::TypeInfoOp>(name);
+#ifdef EXPENSIVE_CHECKS
+ assert(typeInfo == module.lookupSymbol<fir::TypeInfoOp>(name) &&
+ "symbolTable and module out of sync");
+#endif
+ return typeInfo;
+ }
return module.lookupSymbol<fir::TypeInfoOp>(name);
}
More information about the flang-commits
mailing list