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

[River Riddle via Phabricator via llvm-commits]

rriddle added inline comments.


================
Comment at: mlir/include/mlir/IR/SymbolInterfaces.td:106
+      }],
+      "bool", "canDiscardOnUseEmpty", (ins), [{}],
+      /*defaultImplementation=*/[{
----------------
bondhugula wrote:
> "UseEmpty" seems to be an artifact of use_empty() - not perhaps a great fit here. `canDiscardOnNoUses` / `canDiscardOnZeroUses`?
This is intended to match the same verbiage as in LLVM.


================
Comment at: mlir/include/mlir/IR/SymbolInterfaces.td:123
+    }
+    return ::mlir::OpTrait::impl::verifySymbol($_op);
+  }];
----------------
bondhugula wrote:
> 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. 
Moved to mlir::detail. verifySymbolTable is the one that checks for regions, symbols don't need to have regions(e.g. global variables).


================
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()'.
----------------
bondhugula wrote:
> This information has I think gone missing from your operation interface doc comment. 
I added the link to SymbolsAndSymbolTables, which contains information on the constraints of symbols. Made an explicit mention of constraints.


================
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.
----------------
bondhugula wrote:
> Are you verifying somewhere on the op interface that a SymbolTableOp has at least one region?
Yes, this is checked as part of verifySymbolTable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78522