-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[Relay][RFC] Relay IR Text Format #1781
Conversation
CC @dmlc/tvm-team |
The main point we want to address is how to print the code in graph form (without all the lets). This is important because most people construct their net in this way. As an example, we could have
but a preferred way of printing it should be
|
Out of curiosity, what's wrong with |
I just feel there is a tension that user still starts with the old habit of constructing things via DSL(graph form), which makes them cluttered in that case. The printer need to print out the program faithfully still in this case without having to generate a long line of nested calls |
Let us move the discussion to the corresponding RFC issue. @joshpoll can you cross post the proposal there and use the PR to mainly comment on implementation details? |
src/relay/ir/Relay.g4
Outdated
|
||
// non-negative ints | ||
// INT: '0' | [1-9] DIGIT* ; // no leading zeros | ||
INT: DIGIT+ ; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems it will still accept leading zeros.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We now intentionally parse accept leading zeros to be more forgiving. I'll delete the old version.
src/relay/ir/Relay.g4
Outdated
// TODO: is this really the desired outline? if there are defns should there be an expression at the end? | ||
prog: option* (expr | defn+) EOF ; | ||
|
||
option: 'set' ident BOOL_LIT ; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is option's purpose? Why it is tied to bool literal only?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are intended to be boolean compiler options not none/some options. I think @jroesch can speak more directly to their purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the previous iteration our design goal was to enable the text format to be able to set pragmas/options for the program.
src/relay/ir/Relay.g4
Outdated
: '(' expr ')' # parens | ||
| '-' expr # neg | ||
| expr op=('*'|'/') expr # binOp | ||
| expr op=('+'|'-') expr # binOp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need explicit operator associativity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. The ANTLR grammar is used to automatically generate the lexer and the bulk of the parser, so it's important to encode associativity here.
src/relay/ir/Relay.g4
Outdated
| expr op=('=='|'!=') expr # binOp | ||
|
||
// function definition and application | ||
| expr '(' (expr (',' expr)*)? ')' # call |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like we need the function name as an indent instead of expr since expr will accept too much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kind of agree; however, that becomes restrictive if we want to use curried functions. It's possible to restrict what expressions can be used in calls in the parser itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to control the form of the program we should not do it in the parser as it makes the parser overly restrictive and forces semantic information into the parser instead of later analysis phases.
It also disallows programs which are semantically equivalent.
For example:
let x = f(x, y, z);
x(z, y)
Instead of:
f(x, y, z)(z, y)
Even if it looks ugly we should let people generate the text format how they see fit imo.
For example a simple compiler targeting Relay might generate code like:
(fn (...) { ... })(x, y, z)
src/relay/ir/Relay.g4
Outdated
| 'if' expr body 'else' body # ifElse | ||
|
||
// sequencingg | ||
| 'let' MUT? ident (':' type_)? '=' expr ';' expr # let |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does expr ';' expr mean here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relay doesn't have statements, only expressions. Thus let %x = 1;
is not valid Relay, because it doesn't have a value. let
expressions must always contain an identity, an assignment, and a body that might use that assignment (which is the value of the let expression). For example,
let %x = 1;
%x
is valid Relay and evaluates to 1
.
This is similar to the syntax used by ReasonML, which is a version of OCaml made to look like JavaScript.
src/relay/ir/Relay.g4
Outdated
| ident # identExpr | ||
| scalar # scalarExpr | ||
| expr '.' INT # project | ||
| 'debug' # debug |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think expr should also accept body ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good idea, and I'll look into it. It's not immediately clear what the semantics of the body should be, since Relay has no concept of it.
src/relay/ir/Relay.g4
Outdated
fun: 'fn' paramList '=>' type_? body ; | ||
defn: 'def' ident paramList '=>' type_? body ; | ||
|
||
paramList: '(' param (',' param)* ')' ; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fun does not accept zero parameters.
src/relay/ir/Relay.g4
Outdated
// bool | ||
|
||
baseType | ||
: 'int(' INT ',' INT ')' # intType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Int can be any arbitrary number, is that expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
INT
is really a natural number, which is what we want for projection and the inputs here, which represent bits and lanes. Maybe I should change the name to reflect that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the type checker should enforce constraints on this, it will yield a better error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about adding a type-level function syntax and moving this sort of parsing into the python parser?
I am a bit confused. doesnt the lower code has two let? |
src/relay/ir/Relay.g4
Outdated
|
||
type_ | ||
: '(' type_ ')' # parensType | ||
| type_ op=('*'|'/') type_ # binOpType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May I ask to provide short examples in comments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think examples might be better suited to docs, but I'm open to this.
src/relay/ir/Relay.g4
Outdated
|
||
// a program is a list of options followed by either an expression or a list of definitions | ||
// TODO: is this really the desired outline? if there are defns should there be an expression at the end? | ||
prog: option* (expr | defn+) EOF ; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to expr at the end. Why don't we want local defns?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
defn
s are global function definitions. The idiomatic way to do local definitions is
let foo = fn (x, y) => {
...
};
...
It's difficult to see a better syntactic way to express them, but I'm open to ideas.
src/relay/ir/Relay.g4
Outdated
LE: '<=' ; | ||
GE: '>=' ; | ||
EQ: '==' ; | ||
NE: '!=' ; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we define operators as a sequence of "+-*/%..." symbols plus associativity? This would probably reduce the size of grammar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The purpose of these lines is to provide symbols that represent these pieces of syntax in the rest of the parser. I'm considering splitting the lexer and parser files as this would probably improve readability. This is idiomatic ANTLR as far as I can tell. A wealth of example grammars can be found here.
src/relay/ir/Relay.g4
Outdated
// sugar for let _ = WriteRef(ident, expr); expr | ||
| ident '=' expr ';' expr # writeRef | ||
|
||
| ident # identExpr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should global expressions consist of global idents only? If so, how to express it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the rest of the parser, we restrict the kinds of idents that can appear in certain positions. The ANTLR grammar is more lenient, though, because it allows us to produce better error messages when someone accidentally uses the wrong type of ident.
e6951dc
to
faa79bc
Compare
|
Let us avoid include building artifacts for now, as long as the parser is a separate dep it is fine. Later we can to do a binary release of artifact where the build can depend on |
e824e2f
to
e75de2c
Compare
src/relay/pass/alpha_eq.cc
Outdated
@@ -69,7 +69,7 @@ struct TypeAlphaEq : TypeVisitor<const Type&> { | |||
|
|||
void VisitType_(const IncompleteTypeNode* bt1, const Type& t2) final { | |||
if (const IncompleteTypeNode* bt2 = t2.as<IncompleteTypeNode>()) { | |||
equal = equal && bt1 == bt2; | |||
equal = equal && bt1->kind == bt2->kind; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you have this in a seprate 1-line pr? I need it to restore anf testing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
7642084
to
ce72502
Compare
Thanks, @jroesch @joshpoll @grwlf @yuruofeifei @junrushao1994 @MarisaKirisame , this is now merged |
[RFC]: Relay IR Text Format
Please comment on syntax in #1782.
This PR introduces the Relay IR text format. It is intended to be used similarly to LLVM's text format. For example, a text format makes it easier to debug Relay programs and optimization passes as well as serve as a target language for machine learning front ends.
This PR will include
An ANTLR grammar for the Relay IR text format.
A parser that uses the grammar to produce a Relay AST.
A pretty printer that prints a Relay AST in text format. Follow-up PR.
The syntax is heavily inspired by LLVM, Rust, and ReasonML.
Note: The ANTLR grammar accepts some programs that aren't valid Relay programs in order to have better error messages at parse time.