This is an archive of the discontinued LLVM Phabricator instance.

[mlir][PDLL] Add an initial frontend for PDLL
ClosedPublic

Authored by rriddle on Dec 3 2021, 7:40 PM.

Details

Summary

This is a new pattern rewrite frontend designed from the ground
up to support MLIR constructs, and to target PDL. This frontend
language was proposed in https://llvm.discourse.group/t/rfc-pdll-a-new-declarative-rewrite-frontend-for-mlir/4798

This commit starts sketching out the base structure of the
frontend, and is intended to be a minimal starting point for
building up the language. It essentially contains support for
defining a pattern, variables, and erasing an operation. The
features mentioned in the proposal RFC (including IDE support)
will be added incrementally in followup commits.

I intend to upstream the documentation for the language in a
followup when a bit more of the pieces have been landed.

Diff Detail

Event Timeline

rriddle created this revision.Dec 3 2021, 7:40 PM
rriddle requested review of this revision.Dec 3 2021, 7:40 PM

Bikeshed: Is PDLL itself also the name of the frontend? As an analogy, is PDLL the clang or C++ or both?

mlir/include/mlir/Tools/PDLL/AST/Diagnostic.h
2

Since this looks similar to other diagnostics, is there a common impl that this should just reuse?
I assume not, so I'd be interested in the features that are PDLL specific.

mlir/include/mlir/Tools/PDLL/AST/Nodes.h
691

nit: nl

mlir/include/mlir/Tools/PDLL/Parser/Parser.h
34

nit: nl

mlir/lib/Tools/PDLL/AST/Nodes.cpp
21

I have a hard deciding what happens here, I see this ctor calling into strLen that looks for \0:

class StringRef {
    /// Construct a string ref from a cstring.
    /*implicit*/ constexpr StringRef(const char *Str)
        : Data(Str), Length(Str ? strLen(Str) : 0) {}
}

Consider return StringRef("", 0); ?

75

I assume this is the stuff you'd want to fill once you also have a grammar, but for now this CL is the minimal basic shell and plumbing.

mlir/lib/Tools/PDLL/AST/Types.cpp
125

nit: nl

mlir/lib/Tools/PDLL/Parser/Lexer.h
221

nit: nl

221

nit: nl

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

Should there be some minimal grammar doc here and below, or is it part of the followup doc commit?

706

Seems to not be covered by tests?

nicolasvasilache accepted this revision.Dec 6 2021, 3:42 AM
This revision is now accepted and ready to land.Dec 6 2021, 3:42 AM
Mogball added inline comments.
mlir/lib/Tools/PDLL/AST/Nodes.cpp
225

Why not a unique_ptr?

mlir/lib/Tools/PDLL/Parser/Lexer.cpp
133

*curBuffer.end() is fine because the string is null-terminated right?

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

I think this function, at least, could use some documentation.

267

+1

Bikeshed: Is PDLL itself also the name of the frontend? As an analogy, is PDLL the clang or C++ or both?

It's both. Kind of like how Tablegen is both the language and frontend. For small/specialized languages I (personally) don't see a reason to make a distinction.

jpienaar added inline comments.Dec 7 2021, 3:39 PM
mlir/include/mlir/Tools/PDLL/AST/Context.h
44

Micro nit: newline

mlir/include/mlir/Tools/PDLL/AST/Diagnostic.h
2

Good question, would be good to capture why we couldn't use those - currently I'm not sure why we have this in mlir/include/mlir vs in mlir/tools/pdll (or similar) , do we actually expect these headers to be used by any other parts of MLIR infra or is this a standalone tool?

mlir/lib/Tools/PDLL/Parser/Lexer.cpp
378

Newline?

rriddle added inline comments.Dec 7 2021, 4:20 PM
mlir/include/mlir/Tools/PDLL/AST/Diagnostic.h
2

This is actually in mlir/Tools which was created specifically for code that is used by multiple tools (i.e. not used by the infra itself), which is useful because it allows for proper library layering (w.r.t internal/external includes), among other reasons. There are at least 2 individual tools that will use the code exposed here (mlir-pdll, and the language server).

Will add a comment as to why we can't share with the Diagnostics in mlir/IR (mainly because we don't want to use MLIR infra here, which is slower (e.g. uniquing things that we don't need to, we use the direct SMLoc/SMRange), and brings an otherwise unnecessary dependency. Essentially this is just a stripped down version, which is different enough that sharing would be more work than it's worth (with templates and things).

jpienaar accepted this revision.Dec 9 2021, 2:54 PM

Nice, thanks

mlir/include/mlir/Tools/PDLL/AST/Types.h
97

So this is the meet ? Also any errors possible here?

mlir/lib/Tools/PDLL/AST/Nodes.cpp
167

Context is a little bit generic for me, if kept within the tool fine. If this will be used LSP side (where we already have MLIRContext) then I'd rather be explicit PDLLContext

mlir/lib/Tools/PDLL/Parser/Lexer.h
48

Section heading?

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

Would it be pissible to add here what the difference is? (e.g., why do we care about these 2 and only these two, why not have it have be a nesting of parser context etc)

164

Is this newline indicative of something?

240

(I sort of would have expected that this would just be a ScopedHashTable)

944

I'm not sure if TypeSwitch here brings as much value (feels like it is using a lot of horizontal whitespace and makes it less readable than plain variant, but OK)

1066

Why is this needed?

mlir/tools/mlir-pdll/mlir-pdll.cpp
26

Could we move these inside main (as we did with MlirOptMain)?

78

Do you need an InitLLVM?

rriddle updated this revision to Diff 393622.Dec 10 2021, 4:08 PM
rriddle marked 26 inline comments as done.

resolve comments

mlir/include/mlir/Tools/PDLL/AST/Types.h
97

Essentially, added a note on failure returning null.

mlir/lib/Tools/PDLL/AST/Nodes.cpp
75

Yeah, this all is generally just filler for separating things.

167

I think it is fine given that the Context is nested within mlir::pdll::ast. When used with the LSP, it would either be ast::Context (if we add using namespace mlir::pdll, which could be fine given the context.. hehe) or pdll::ast::Context. If you feel strongly I can change it, but I plan to another context in a followup; i.e. mlir::pdll::ods::Context. Having ast::Context and ods::Context felt cleaner than ast::PDLLASTContext, ods::PDLLODSContext (given that I don't plan to add usings for either ast or ods namespaces in generic code).

225

For Module in particular? or all AST nodes? For AST nodes, using trailing storage and BumpPtrAllocator is much more efficient than std::unique_ptr given the memory locality and greatly reduced malloc traffic. For Module in particular, given that it is an AST node there is not much benefit in treating it differently than the others IMO (AST nodes don't really need to be de-allocated).

mlir/lib/Tools/PDLL/Parser/Lexer.cpp
133

Yep, indexing the end of the buffer is safe because it's null-terminated.

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

There will be a few more added in followups, this is essentially used just for verification purposes so we don't need to nest the parser or anything like that. For example, this makes it easier to verify that we don't use a rewrite statement inside of a constraint, call user defined rewrites from inside of a "match" section, etc.

240

It isn't a scoped hash table because the scope may live beyond its use in the parser process. For example, we often give a scope to the LSP for things like code completion. For templates, we may temporarily switch the scope back to the parent of the template when doing instantiations.

(Simplified scopes a bit by moving management to the parser, I originally had them in the AST in case we need to manage scopes from an AST node directly, but I don't have a direct need for that yet)

267

It's part of a followup that adds proper frontend documentation (I want to make sure everything really important is captured in user facing docs). Will add a few comments here for now though.

This revision was landed with ongoing or failed builds.Dec 15 2021, 6:17 PM
This revision was automatically updated to reflect the committed changes.