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

[Oleksandr Alex Zinenko]

Ingo =?utf-8?q?Müller?= <ingomueller at google.com>
Message-ID:
In-Reply-To: <llvm/llvm-project/pull/67241/mlir at github.com>


================
@@ -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();
+      }
+
----------------
ftynse wrote:

We need to be careful here, as this basically a proto-linker. One low-tech solution could be to just clone all symbols from the library file(s) into the main one, not just the declared/referenced ones. The difficult issue in any case is name clashes between files. AFAIR, `named_sequence` doesn't have the visibility attribute and it probably should for this purpose. It would explicitly tell us where renaming a symbol to avoid the name clash is acceptable (private symbol) and when it isn't. When it isn't, the process needs to run in three stages: (1) collect all definitions (i.e., with bodies) of named sequences that need to be cloned without actually cloning them + check that there is exactly one definition of each if renaming is not allowed; (2) if needed and allowed, rename private symbols and update their uses in each module individually; (3) actually clone (also let's consider moving them instead).

Limiting the process to only the symbols that are actually needed is an optimization. We can do it, but can also punt. In the general case, we need something like connected components for all named sequences in the main file as it may also have unused declarations. There's `CallGraph` for that and LLVM has algorithms to compute connected components so no need to write this manually.

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