This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Symbol] Change Symbol from a Trait into an OpInterface.
ClosedPublic

Authored by rriddle on Apr 20 2020, 3:39 PM.

Details

Summary

This provides a much cleaner interface into Symbols, and allows for users to start injecting op-specific information. For example, derived op can now inject when a symbol can be discarded if use_empty. This would let us drop unused external functions, which generally have public visibility.

This revision also adds a new extraTraitClassDeclaration field to the ODS OpInterface class to allow for injecting declarations into the trait class that gets attached to the operations.

Depends On D78447

Diff Detail

Event Timeline

rriddle created this revision.Apr 20 2020, 3:39 PM
mlir/include/mlir/IR/SymbolInterfaces.td
26

Can an operation ever not have a name? Did you mean to say "A Symbol operation resides ..."?

106

"UseEmpty" seems to be an artifact of use_empty() - not perhaps a great fit here. canDiscardOnNoUses / canDiscardOnZeroUses?

123

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.

mlir/include/mlir/IR/SymbolTable.h
232–234

This information has I think gone missing from your operation interface doc comment.

mlir/lib/Transforms/SymbolDCE.cpp
81

Are you verifying somewhere on the op interface that a SymbolTableOp has at least one region?

rriddle updated this revision to Diff 259204.Apr 22 2020, 1:41 AM
rriddle marked 9 inline comments as done.

Resolve comments

rriddle added inline comments.Apr 22 2020, 1:42 AM
mlir/include/mlir/IR/SymbolInterfaces.td
106

This is intended to match the same verbiage as in LLVM.

123

Moved to mlir::detail. verifySymbolTable is the one that checks for regions, symbols don't need to have regions(e.g. global variables).

mlir/include/mlir/IR/SymbolTable.h
232–234

I added the link to SymbolsAndSymbolTables, which contains information on the constraints of symbols. Made an explicit mention of constraints.

mlir/lib/Transforms/SymbolDCE.cpp
81

Yes, this is checked as part of verifySymbolTable.

DavidTruby resigned from this revision.Apr 22 2020, 6:39 AM
ftynse resigned from this revision.Apr 27 2020, 6:38 AM

Can you document in the commit message the addition of extraTraitClassDeclaration? This seems like a new ODS feature independent of the symbol table change (someone using git blame in the future would end up in this commit)

mehdi_amini accepted this revision.Apr 27 2020, 12:05 PM
This revision is now accepted and ready to land.Apr 27 2020, 12:05 PM
rriddle edited the summary of this revision. (Show Details)Apr 27 2020, 12:36 PM
This revision was automatically updated to reflect the committed changes.