[PATCH] D72042: [mlir] Update the use-list algorithms in SymbolTable to support nested references.

[Jacques Pienaar via Phabricator via llvm-commits]

jpienaar accepted this revision.
jpienaar added a comment.
This revision is now accepted and ready to land.
Herald added subscribers: lucyrfox, mgester, arpith-jacob.

I don't know this part as well as Mehdi does, but it looks good.



================
Comment at: mlir/lib/IR/SymbolTable.cpp:29
+    // Check that this is a valid op and isn't an unknown symbol table.
+    if (!from || isPotentiallyUnknownSymbolTable(from))
+      return nullptr;
----------------
So we'd have nullptr returned if either a op has no parent or if its parent is a unknown op? This seems sort of difficult to interpret for a caller (e.g., I found no symbol table or I found an unregistered op). Or what should be done when nullptr is returned? [and could you add that to the comment describing the functions]

Also do we have a test that exercises this?


================
Comment at: mlir/lib/IR/SymbolTable.cpp:201
+  // Lookup the root reference for this symbol.
+  symbolTableOp = lookupSymbolIn(symbolTableOp, symbol.getRootReference());
+  if (!symbolTableOp)
----------------
So we don't need to assert here as to the type as we are effectively invoking line 188 here? I'd be pro asserting here too just as it makes the precondition clear.


================
Comment at: mlir/lib/IR/SymbolTable.cpp:396
 
+/// Walks all of the symbol scopes from 'symbol' to(inclusive) 'limit' invoking
+/// the provided callback at each one with a properly scoped reference to
----------------
 missing space ?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72042/new/

https://reviews.llvm.org/D72042