[Mlir-commits] [mlir] 835e014 - [mlir] only verify moved symbols in transform (#197882)
llvmlistbot at llvm.org
llvmlistbot at llvm.org
Thu May 21 01:37:53 PDT 2026
Author: Oleksandr "Alex" Zinenko
Date: 2026-05-21T10:37:48+02:00
New Revision: 835e014bc0a0672e71f296c83d6a8af6a080844b
URL: https://github.com/llvm/llvm-project/commit/835e014bc0a0672e71f296c83d6a8af6a080844b
DIFF: https://github.com/llvm/llvm-project/commit/835e014bc0a0672e71f296c83d6a8af6a080844b.diff
LOG: [mlir] only verify moved symbols in transform (#197882)
When merging named sequences from an external module in the transform
interpreter, only run the inliner verification for operations that were
actually moved rather than all pre-existing operations. This avoids
verifying inlining conditions for operations that wouldn't be inlined by
this logic, and is also more parsimonious.
Reverts #195770 but keeps the test. This is a more generic fix.
Added:
Modified:
mlir/lib/Dialect/Transform/IR/Utils.cpp
Removed:
################################################################################
diff --git a/mlir/lib/Dialect/Transform/IR/Utils.cpp b/mlir/lib/Dialect/Transform/IR/Utils.cpp
index de2fc69bedaf0..44238e0b92ff2 100644
--- a/mlir/lib/Dialect/Transform/IR/Utils.cpp
+++ b/mlir/lib/Dialect/Transform/IR/Utils.cpp
@@ -120,34 +120,6 @@ transform::detail::mergeSymbolsInto(Operation *target,
"requires target to implement the 'SymbolTable' trait");
SymbolTable targetSymbolTable(target);
- InlinerInterface inliner(target->getContext());
-
- // Collect all the functions that are called in `target` that cannot be
- // inlined into `target`.
- SmallPtrSet<Operation *, 1> noInlineCalls;
- target->walk([&](CallOpInterface call) {
- Operation *callable = nullptr;
- CallInterfaceCallable callee = call.getCallableForCallee();
- if (auto symRef = dyn_cast<SymbolRefAttr>(callee)) {
- // Fall back to full resolution for nested symbols, the table is
- // one-level only.
- if (isa<FlatSymbolRefAttr>(symRef))
- callable = targetSymbolTable.lookup(symRef.getLeafReference());
- else
- callable = SymbolTable::lookupNearestSymbolFrom(call, symRef);
- } else if (auto value = dyn_cast<Value>(callee)) {
- callable = value.getDefiningOp();
- }
-
- if (!callable)
- return;
-
- if (!inliner.isLegalToInline(call, callable, /*wouldBeCloned=*/false)) {
- noInlineCalls.insert(call.getOperation());
- }
- return;
- });
-
SymbolTable otherSymbolTable(*other);
// Step 1:
@@ -176,7 +148,7 @@ transform::detail::mergeSymbolsInto(Operation *target,
LDBG() << " collision found for @" << name.getValue();
- // Collisions are fine if both opt are functions and can be merged.
+ // Collisions are fine if both ops are functions and can be merged.
if (auto funcOp = dyn_cast<FunctionOpInterface>(op),
collidingFuncOp =
dyn_cast<FunctionOpInterface>(collidingOp.getOperation());
@@ -241,59 +213,61 @@ transform::detail::mergeSymbolsInto(Operation *target,
//
// Move all ops from `other` into target and merge public symbols.
LDBG() << "moving all symbols into target";
- {
- SmallVector<SymbolOpInterface> opsToMove;
- for (Operation &op : other->getRegion(0).front()) {
- if (auto symbol = dyn_cast<SymbolOpInterface>(op))
- opsToMove.push_back(symbol);
+ SmallVector<SymbolOpInterface> processedSymbols;
+ for (Operation &op : other->getRegion(0).front()) {
+ if (auto symbol = dyn_cast<SymbolOpInterface>(op))
+ processedSymbols.push_back(symbol);
+ }
+
+ for (SymbolOpInterface &op : processedSymbols) {
+ // Remember potentially colliding op in the target module.
+ auto collidingOp = cast_or_null<SymbolOpInterface>(
+ targetSymbolTable.lookup(op.getNameAttr()));
+
+ // Move op even if we get a collision.
+ LDBG() << " moving @" << op.getName();
+ op->moveBefore(&target->getRegion(0).front(),
+ target->getRegion(0).front().end());
+
+ // If there is no collision, we are done -- keep the target symbol
+ // table in sync with the moved op so that subsequent lookups (and the
+ // post-merge validation below) remain efficient.
+ if (!collidingOp) {
+ LDBG() << " without collision";
+ targetSymbolTable.insert(op);
+ continue;
}
- for (SymbolOpInterface op : opsToMove) {
- // Remember potentially colliding op in the target module.
- auto collidingOp = cast_or_null<SymbolOpInterface>(
- targetSymbolTable.lookup(op.getNameAttr()));
-
- // Move op even if we get a collision.
- LDBG() << " moving @" << op.getName();
- op->moveBefore(&target->getRegion(0).front(),
- target->getRegion(0).front().end());
-
- // If there is no collision, we are done -- keep the target symbol
- // table in sync with the moved op so that subsequent lookups (and the
- // post-merge validation below) remain efficient.
- if (!collidingOp) {
- LDBG() << " without collision";
- targetSymbolTable.insert(op);
- continue;
- }
+ // The two colliding ops must both be functions because we have already
+ // emitted errors otherwise earlier.
+ auto funcOp = cast<FunctionOpInterface>(op.getOperation());
+ auto collidingFuncOp =
+ cast<FunctionOpInterface>(collidingOp.getOperation());
+
+ // Both ops are in the target module now and can be treated
+ // symmetrically, so w.l.o.g. we can reduce to merging `funcOp` into
+ // `collidingFuncOp`.
+ if (!canMergeInto(funcOp, collidingFuncOp)) {
+ std::swap(funcOp, collidingFuncOp);
+ }
+ assert(canMergeInto(funcOp, collidingFuncOp));
- // The two colliding ops must both be functions because we have already
- // emitted errors otherwise earlier.
- auto funcOp = cast<FunctionOpInterface>(op.getOperation());
- auto collidingFuncOp =
- cast<FunctionOpInterface>(collidingOp.getOperation());
-
- // Both ops are in the target module now and can be treated
- // symmetrically, so w.l.o.g. we can reduce to merging `funcOp` into
- // `collidingFuncOp`.
- if (!canMergeInto(funcOp, collidingFuncOp)) {
- std::swap(funcOp, collidingFuncOp);
- }
- assert(canMergeInto(funcOp, collidingFuncOp));
+ LDBG() << " with collision, trying to keep op at "
+ << collidingFuncOp.getLoc() << ":\n"
+ << collidingFuncOp;
- LDBG() << " with collision, trying to keep op at "
- << collidingFuncOp.getLoc() << ":\n"
- << collidingFuncOp;
+ // Update symbol table. This works with or without the previous `swap`.
+ targetSymbolTable.remove(funcOp);
+ targetSymbolTable.insert(collidingFuncOp);
+ assert(targetSymbolTable.lookup(funcOp.getName()) == collidingFuncOp);
- // Update symbol table. This works with or without the previous `swap`.
- targetSymbolTable.remove(funcOp);
- targetSymbolTable.insert(collidingFuncOp);
- assert(targetSymbolTable.lookup(funcOp.getName()) == collidingFuncOp);
+ // Do the actual merging.
+ if (failed(mergeInto(funcOp, collidingFuncOp)))
+ return failure();
- // Do the actual merging.
- if (failed(mergeInto(funcOp, collidingFuncOp)))
- return failure();
- }
+ // After merging, only collidingFuncOp exists, update the list to reflect
+ // this.
+ op = collidingFuncOp;
}
// Symbol merging only moves callable ops between symbol tables; it does not
@@ -319,6 +293,7 @@ transform::detail::mergeSymbolsInto(Operation *target,
if (preVerify.wasInterrupted())
return failure();
+ InlinerInterface inliner(target->getContext());
WalkResult inlineCheck = target->walk([&](CallOpInterface call) {
Operation *callable = nullptr;
CallInterfaceCallable callee = call.getCallableForCallee();
@@ -335,8 +310,11 @@ transform::detail::mergeSymbolsInto(Operation *target,
if (!callable)
return WalkResult::advance();
- if (!noInlineCalls.contains(call.getOperation()) &&
- !inliner.isLegalToInline(call, callable, /*wouldBeCloned=*/false)) {
+
+ // Only check symbols that we actually moved.
+ if (!llvm::is_contained(processedSymbols, callable))
+ return WalkResult::advance();
+ if (!inliner.isLegalToInline(call, callable, /*wouldBeCloned=*/false)) {
InFlightDiagnostic diag =
call->emitError()
<< "merged call is not legal to inline into its caller";
More information about the Mlir-commits
mailing list