[Mlir-commits] [mlir] [mlir] Make symbol-user type verification proportional to participating IR (PR #212354)

[Mehdi Amini]

================
@@ -476,49 +476,57 @@ raw_ostream &mlir::operator<<(raw_ostream &os,
 // SymbolTable Trait Types
 //===----------------------------------------------------------------------===//
 
-/// Verify the symbol uses held by the types owned by `op`: its operand,
-/// result, and block-argument types, and any types nested within its
-/// attributes. `op` is the anchor used for symbol lookups. `verifiedTypes`
-/// records the types already verified within the current symbol table so that
-/// each type, which may be uniqued and shared across many positions or
-/// operations, is verified at most once. Verification fails fast on the first
-/// invalid symbol use.
+/// Verify the symbol uses held by the types owned by `op`: its operand, result,
+/// and block-argument types, and any types nested within its attributes.
+/// `typeWalker` carries the SymbolUserTypeInterface check as a walk callback,
+/// anchored at `op` for symbol lookups, and its shared visited set makes each
+/// uniqued type, which may recur across many positions and operations, verified
+/// against the enclosing symbol table at most once. A type or attribute whose
+/// interning-time bit is clear is skipped: it provably contains no
+/// SymbolRefAttr, and a SymbolUserTypeInterface type is required to spell its
+/// references as SymbolRefAttr sub-elements, so walking it would verify
+/// nothing. Verification fails fast on the first invalid symbol use.
 static LogicalResult verifyOpTypeSymbolUses(Operation *op,
-                                            SymbolTableCollection &symbolTable,
-                                            SetVector<Type> &verifiedTypes) {
-  // Walk `type` and any nested type parameters reachable from it, verifying
-  // each not-yet-seen type and interrupting on the first failure.
-  auto verify = [&](Type type) {
-    return type.walk<WalkOrder::PreOrder>([&](Type nestedType) {
-      if (!verifiedTypes.insert(nestedType))
-        return WalkResult::advance();
-      if (auto user = dyn_cast<SymbolUserTypeInterface>(nestedType))
-        if (failed(user.verifySymbolUses(op, symbolTable)))
-          return WalkResult::interrupt();
+                                            AttrTypeWalker &typeWalker) {
+  auto verifyType = [&](Type type) {
+    if (!type.mayContainSymbolRefs())
       return WalkResult::advance();
-    });
+    return typeWalker.walk<WalkOrder::PreOrder>(type);
+  };
+  auto verifyAttr = [&](Attribute attr) {
+    if (!attr || !attr.mayContainSymbolRefs())
+      return WalkResult::advance();
+    return typeWalker.walk<WalkOrder::PreOrder>(attr);
   };
 
   for (Type type : op->getOperandTypes())
-    if (verify(type).wasInterrupted())
+    if (verifyType(type).wasInterrupted())
       return failure();
   for (Type type : op->getResultTypes())
-    if (verify(type).wasInterrupted())
+    if (verifyType(type).wasInterrupted())
       return failure();
   for (Region &region : op->getRegions())
     for (Block &block : region)
       for (BlockArgument argument : block.getArguments())
-        if (verify(argument.getType()).wasInterrupted())
+        if (verifyType(argument.getType()).wasInterrupted())
           return failure();
 
-  // Verify types nested within the operation's attributes.
-  WalkResult attrResult =
-      op->getAttrDictionary().walk<WalkOrder::PreOrder>([&](Type type) {
-        if (verify(type).wasInterrupted())
-          return WalkResult::interrupt();
-        return WalkResult::advance();
-      });
-  return failure(attrResult.wasInterrupted());
+  // Verify types nested within the operation's attributes. Read the raw stored
+  // attribute dictionary rather than getAttrDictionary(): the latter allocates
+  // and uniques a fresh dictionary for every operation that keeps its inherent
+  // attributes in properties. The raw dictionary already covers inherent
+  // attributes for operations that do not use properties; the properties-held
+  // inherent attributes are walked separately below.
+  if (verifyAttr(op->getRawDictionaryAttrs()).wasInterrupted())
+    return failure();
+  if (op->getPropertiesStorageSize()) {
+    NamedAttrList inherentAttrs;
+    op->getName().populateInherentAttrs(op, inherentAttrs);
----------------
joker-eph wrote:

That seems expensive: converting properties to attribute does not seem like something we'd want to do. We should try to instead generate some dispatch hook with every property to avoid this.

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