[PATCH] D78522: [mlir][Symbol] Change Symbol from a Trait into an OpInterface.

[Uday Bondhugula via Phabricator via llvm-commits]

bondhugula added a comment.





================
Comment at: mlir/include/mlir/IR/SymbolInterfaces.td:26
+    This interface describes an operation that may define a `Symbol`. A `Symbol`
+    is a named operation that resides immediately within a region that defines
+    a `SymbolTable`. See [Symbols and SymbolTables](SymbolsAndSymbolTables.md)
----------------
Can an operation ever not have a name? Did you mean to say "A `Symbol` operation resides ..."?


================
Comment at: mlir/include/mlir/IR/SymbolInterfaces.td:106
+      }],
+      "bool", "canDiscardOnUseEmpty", (ins), [{}],
+      /*defaultImplementation=*/[{
----------------
"UseEmpty" seems to be an artifact of use_empty() - not perhaps a great fit here. `canDiscardOnNoUses` / `canDiscardOnZeroUses`?


================
Comment at: mlir/include/mlir/IR/SymbolInterfaces.td:123
+    }
+    return ::mlir::OpTrait::impl::verifySymbol($_op);
+  }];
----------------
Why is this still under a trait implementation namespace? On another note, I think `verifySymbol` is missing a zero region check and the code is assuming there is at least one region. 


================
Comment at: mlir/include/mlir/IR/SymbolTable.h:232-234
-/// A trait used to define a symbol that can be used on operations within a
-/// symbol table. Operations using this trait must adhere to the following:
-///   * Have a StringAttr attribute named 'SymbolTable::getSymbolAttrName()'.
----------------
This information has I think gone missing from your operation interface doc comment. 


================
Comment at: mlir/lib/Transforms/SymbolDCE.cpp:81
   // are known to be live.
   for (auto &block : symbolTableOp->getRegion(0)) {
+    // Add all non-symbols or symbols that can't be discarded.
----------------
Are you verifying somewhere on the op interface that a SymbolTableOp has at least one region?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78522