[Mlir-commits] [mlir] [mlir][transform] Fix handling of transitive includes in interpreter. (PR #67241)

[Ingo Müller]

================
@@ -367,10 +378,52 @@ static LogicalResult defineDeclaredSymbols(Block &block, ModuleOp definitions) {
       }
     }
 
-    OpBuilder builder(&op);
-    builder.setInsertionPoint(&op);
+    OpBuilder builder(symbol);
+    builder.setInsertionPoint(symbol);
     builder.clone(*externalSymbol);
     symbol->erase();
+
+    LLVM_DEBUG(DBGS() << "scanning definition of @"
+                      << externalSymbolFunc.getNameAttr().getValue()
+                      << " for symbol usages\n");
+    externalSymbolFunc.walk([&](CallOpInterface callOp) {
+      LLVM_DEBUG(DBGS() << "  found symbol usage in:\n" << callOp << "\n");
+      CallInterfaceCallable callable = callOp.getCallableForCallee();
+      if (!isa<SymbolRefAttr>(callable)) {
+        LLVM_DEBUG(DBGS() << "    not a 'SymbolRefAttr'\n");
+        return WalkResult::advance();
+      }
+
+      StringRef callableSymbol =
+          cast<SymbolRefAttr>(callable).getLeafReference();
+      LLVM_DEBUG(DBGS() << "    looking for @" << callableSymbol
+                        << " in definitions: ");
+
+      Operation *callableOp = definitionsSymbolTable.lookup(callableSymbol);
+      if (!isa<SymbolRefAttr>(callable)) {
+        LLVM_DEBUG(llvm::dbgs() << "not found\n");
+        return WalkResult::advance();
+      }
+      LLVM_DEBUG(llvm::dbgs() << "found " << callableOp << " from "
+                              << callableOp->getLoc() << "\n");
+
+      if (!block.getParent() || !block.getParent()->getParentOp()) {
+        LLVM_DEBUG(DBGS() << "could not get parent of provided block");
+        return WalkResult::advance();
+      }
+
+      SymbolTable targetSymbolTable(block.getParent()->getParentOp());
+      if (targetSymbolTable.lookup(callableSymbol)) {
+        LLVM_DEBUG(DBGS() << "    symbol @" << callableSymbol
+                          << " already present in target\n");
+        return WalkResult::advance();
+      }
+
----------------
ingomueller-net wrote:

OK, I think that you propose makes sense. Linking in all symbols instead of just the referenced once is something that I need for #67120.

One question, though: `SymbolOpInterface` alone does not know the concept of "external" (aka the distinction between definition vs declaration); only `FunctionOpInterface` does. However, the previous implementation of `defineDeclaredSymbols` [tested](https://github.com/llvm/llvm-project/blob/2f8690b1e2b/mlir/lib/Dialect/Transform/Transforms/TransformInterpreterPassBase.cpp#L316-L320) for `SymbolOpInterface` and `symbol->getNumRegions() == 1 && !symbol->getRegion(0).empty()`, which corresponds to the [definition of `isExternal`](https://github.com/llvm/llvm-project/blob/2f8690b1e2b/mlir/include/mlir/Interfaces/FunctionInterfaces.td#L163) in `FunctionOpInterface`. I think the more general case to deal with that is to implement the case that allows external functions (aka declarations) for the special case `FunctionOpInterface` and to not allow clashes of public symbols otherwise. Do you agree?

https://github.com/llvm/llvm-project/pull/67241