-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: downgrade MLIR generic parser from syntax categories to closed syntax #364
base: main
Are you sure you want to change the base?
Conversation
@goens @bollu @tobiasgrosser I'm curious what you think about this refactor? |
Alive Statistics: 54 / 93 (39 failed) |
I think this is a great idea. |
@alexkeizer thanks for thinking about this, I'm generally happy with this change in terms of API surface area. I don't quite understand:
Could you elaborate on this? Is it that each syntax category is a construction for our meta-level-AST, and having many constructors makes this unmanagable? |
Alive Statistics: 54 / 93 (39 failed) |
So what I'm thinking about is having a function like Footnotes
|
Apparently closed `syntax` is namespaced, while syntax categories live in the root namespace
Alive Statistics: 54 / 93 (39 failed) |
Alive Statistics: 54 / 93 (39 failed) |
1 similar comment
Alive Statistics: 54 / 93 (39 failed) |
Apparently the closed syntax breaks `opt`
oh that's pretty cool! I didn't know about this difference (between closed syntax and extensible syntax categories). I think it makes a lot of sense, thanks for the very thorough explanation @alexkeizer! About the meta level ast, I also don't quite understand the point. Even if we choose to do things at elaboration time, what's the drawback of using our (current) AST data structure? Or do you mean just that we can do (full) pattern matching on the closed syntax categories, so it's about the |
So the main point is to correctly communicate what is and isn't supported. With a closed syntax we know exactly what forms of syntax to expect when we pattern-match on the An option that totally works, is to have a syntax category and just only match on the syntax we've defined. Then, when the category is extended, and this new syntax is actually used, we just throw the error that says "well, this syntax category is not actually meant to be extended". This option would be strictly easiest to implement, but it's unsatisfying IMO. If we have syntax categories, and would like to properly support extensions, then in the function that parses the A slight road bump: apparently the machinery in the |
This reverts commit cc1112d.
Currently almost all parts of the generic MLIR parser uses syntax categories, meaning these parts of the (generic!) grammar are in theory extensible.
In theory this can be very convenient: if a dialect wanted to customize only their operands it could choose to hook into just that part of the parser. In practice, I would expect that even the low level elements (such as
mlir_op_operand
) being extensible is an anti-pattern:macro "&" n:num : mlir_op_operand => `(mlir_op_operand| % $n:num)
).AST
, if we eventually decide to go that way.Thus, I propose we keep
mlir_op
(and potentiallymlir_type
) as the top-level syntax category that can be hooked into, but make low-level elements of the grammar closed. This PR is still a draft, but it show-cases what this change would look like for the case ofmlir_op_operand
.This matches Lean itself, where
term
andcommand
are extensible, but things likedef
are closed syntax.Also note that this is exactly the level at which the current LLVM pretty syntax operates: that only extends
mlir_op
and would still work as-is after this refactor.