[Mlir-commits] [mlir] [mlir] Walk nested non-symbol table ops in symbol dce (PR #143353)

[Jeremy Kun]

================
@@ -108,15 +112,42 @@ LogicalResult SymbolDCE::computeLiveness(Operation *symbolTableOp,
   // that are referenced within.
   while (!worklist.empty()) {
     Operation *op = worklist.pop_back_val();
+    LLVM_DEBUG(llvm::dbgs() << "processing: " << op->getName() << "\n");
 
     // If this is a symbol table, recursively compute its liveness.
     if (op->hasTrait<OpTrait::SymbolTable>()) {
       // The internal symbol table is hidden if the parent is, if its not a
       // symbol, or if it is a private symbol.
       SymbolOpInterface symbol = dyn_cast<SymbolOpInterface>(op);
       bool symIsHidden = symbolTableIsHidden || !symbol || symbol.isPrivate();
+      LLVM_DEBUG(llvm::dbgs() << "\tsymbol table: " << op->getName()
+                              << " is hidden: " << symIsHidden << "\n");
       if (failed(computeLiveness(op, symbolTable, symIsHidden, liveSymbols)))
         return failure();
+    } else {
+      LLVM_DEBUG(llvm::dbgs()
+                 << "\tnon-symbol table: " << op->getName() << " is hidden\n");
+      // If the op is not a symbol table, then, unless op itself is dead which
+      // would be handled by DCE, we need to check all the regions and blocks
+      // within the op to find the uses (e.g., consider visibility within op as
+      // if top level rather than relying on pure symbol table visibility). This
+      // is more conservative than SymbolTable::walkSymbolTables in the case
+      // where there is again SymbolTable information to take advantage of.
+      for (auto &region : op->getRegions()) {
+        for (auto &block : region.getBlocks()) {
+          for (Operation &op : block) {
+            SymbolOpInterface symbol = dyn_cast<SymbolOpInterface>(&op);
----------------
j2kun wrote:

I'm mildly confused, but this is mostly for my education, not a request to change anything: isn't a symbol operation defined by the property that it "resides immediately within a region that defines a SymbolTable"? [[1]] If that is the case, it's not clear to me why this cast would ever trigger (the else block this path lies within implies the parent operation does not define a symbol table)

[1]: https://github.com/llvm/llvm-project/blob/9c52b3c52eb760a52826aad1368a4b0b46d924d3/mlir/include/mlir/IR/SymbolInterfaces.td#L26

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