This is an archive of the discontinued LLVM Phabricator instance.

[PDLL] Add support for tablegen includes and importing ODS information
ClosedPublic

Authored by rriddle on Feb 15 2022, 3:20 PM.

Details

Summary

This commit adds support for processing tablegen include files, and importing
various information from ODS. This includes operations, attribute+type constraints,
attribute/operation/type interfaces, etc. This will allow for much more robust tooling,
and also allows for referencing ODS constructs directly within PDLL (imported interfaces
can be used as constraints, operation result names can be used for member access, etc).

Diff Detail

Event Timeline

rriddle created this revision.Feb 15 2022, 3:20 PM
rriddle requested review of this revision.Feb 15 2022, 3:20 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 15 2022, 3:20 PM
rriddle updated this revision to Diff 409419.Feb 16 2022, 2:42 PM
rriddle edited the summary of this revision. (Show Details)

Just half a scan (sorry have to context switch again but asking some in interim)

llvm/include/llvm/Support/SourceMgr.h
103

Could we split these out?

mlir/include/mlir/Tools/PDLL/ODS/Constraint.h
42

Are these std::string due to life time concerns?

mlir/include/mlir/Tools/PDLL/ODS/Context.h
65

Contrary to MLIR IR side is there any expectation that this is useful beyond debugging?

mlir/include/mlir/Tools/PDLL/ODS/Operation.h
115

What does opaque here signify?

jpienaar accepted this revision.Feb 28 2022, 6:59 PM
jpienaar added inline comments.
mlir/include/mlir/Tools/PDLL/ODS/Constraint.h
1

PDLL instead of PD:L

7

(PDLL ODS constraints is such a mouthful that it may make sense to add file header comment)

mlir/include/mlir/Tools/PDLL/ODS/Context.h
42

What happens if a constraint by that name already exists?

48

The above have add being const reference return, while here add is for mutable and lookup for non-mutable without insertion. What mutation is expected here/why isn't that expected above?

57

insertOperation? (This is why your comment even says :-)). Up to you.

mlir/include/mlir/Tools/PDLL/ODS/Dialect.h
36

Ditto

43

Out dated comment? (The name makes more sense to me with this comment)

mlir/include/mlir/Tools/PDLL/ODS/Operation.h
55

Same question wrt string vs stringref here.

71

"represents a ... representation" feels off

124

And here adds don't return anything (these feel inconsistent to have same names/prefixes enable different things)

mlir/lib/Tools/PDLL/ODS/Context.cpp
102

Could we use the scoped & indented ostream emitter? (Seems would make it more less likely to forget a space)

mlir/lib/Tools/PDLL/Parser/Parser.cpp
716

Sneaky :-) And include paths are not an issue?

This revision is now accepted and ready to land.Feb 28 2022, 6:59 PM
nicolasvasilache accepted this revision.Mar 2 2022, 11:46 PM

@rriddle could you land your stack?

Also, what do you expect the support to look like for something like class RankedFloatElementsAttr<int width, list<int> dims> : ?
Is the idea that pdll will understand int as a first class citizen or that we'll go through e.g. I64?
What about list<int> ?

Thanks!

Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2022, 11:46 PM

@rriddle could you land your stack?

Yep, sorry for the delay. Now that Jacques reviewed, PDL(L) is back at the top of my stack, so I'll be going through things tomorrow (PST).

Also, what do you expect the support to look like for something like class RankedFloatElementsAttr<int width, list<int> dims> : ?
Is the idea that pdll will understand int as a first class citizen or that we'll go through e.g. I64?
What about list<int> ?

For tablegen import, we can only really import defs at this point; i.e., we won't be able to import templated classes. So for now, if we want to
use them we'll generally need to explicitly define the templates we care about in .td and then import the fully specialized def.

This revision was landed with ongoing or failed builds.Mar 3 2022, 4:14 PM
This revision was automatically updated to reflect the committed changes.
rriddle marked 15 inline comments as done.
rriddle added inline comments.Mar 3 2022, 4:18 PM
mlir/include/mlir/Tools/PDLL/ODS/Constraint.h
42

Yeah, in a follow up I intend to make the summary/description stuff all optional.

mlir/include/mlir/Tools/PDLL/ODS/Context.h
42

Just returns the previous one. Added in asserts that the info is the same on duplicate insertion.

48

Constraints are self-contained, and don't really need any mutation after insertion. Dialects can have additional constructs inserted (operations/etc). We could make the constraints non-const on return, but there isn't a reason to right now.

65

Not really. Printing here is really only for debugging/testing.

mlir/include/mlir/Tools/PDLL/ODS/Operation.h
55

Yep, this is for lifetime issues.

115

Just dropped all of the opaque.

124

Keeping these as add now that the functions returning things are called "insert".