Skip to content
This repository has been archived by the owner on May 22, 2023. It is now read-only.

[UX][TVMScript] Feature requests #202

Closed
slyubomirsky opened this issue Aug 3, 2022 · 5 comments
Closed

[UX][TVMScript] Feature requests #202

slyubomirsky opened this issue Aug 3, 2022 · 5 comments
Labels

Comments

@slyubomirsky
Copy link
Collaborator

slyubomirsky commented Aug 3, 2022

Earlier discussion between myself and @junrushao1994, @YuchenJin, and others suggested that there are various aspects we are interested in improving about the Relax parser, so we would like to start this thread to keep track of changes we would like to make, particularly as we consider substantial revisions to the parsing infrastructure per apache/tvm-rfcs#79 (thanks to @cyx-6).

Here are some of the issues I've run into with the parser that we may want to consider changing:

  1. The parser considers it an error to assign a variable more than once. I would argue that this can be reasonably parsed as having a new variable shadow the old one.
  2. The parser considers it an error to subscript a tuple literal (e.g., (x,)[0]). While strange, this has a logical interpretation and should not be rejected.
  3. The parser requires the results of all calls to be bound to a variable. Since calls can include ExternFuncs that have side effects, there may not be a return value to bind. This can be parsed as having a binding to a fresh and never-again-used variable. (Unrelated semantics issue: If we do bind the result of a PackedFunc that doesn't return anything to a variable, what type should the variable have? The Python FFI treats the resulting value as None, but there is no notion of None in the Relax language, so should we perhaps turn them into empty tuples?)
  4. The parser requires all functions to have a return. If we are calling ExternFuncs with side effects, it may not make sense to return a value. We could parse this as a SeqExpr whose body field is an empty tuple. (This is related to the above semantics question, I guess.)
@slyubomirsky
Copy link
Collaborator Author

Shadowing example courtesy of @junrushao1994:

@R.function
def f(...):
  a = ...
  a = ... # <===== the previous `a` is shadowed. This is a new variable that happens to also be named `a`.

This brings up a related issue: Perhaps we should be allowed to reuse the same variable names in different scopes, also via shadowing.

a = ...
if condition:
   a = ... # a is shadowed in the then branch's scope
   consume(a) # uses the new value of a
   ...
else:
  a = ... # a is shadowed in the else branch's scope
  consume(a) # uses the new value of a
  ...
b = a + 1 # this is using the original value of a from the outer scope

@junrushao junrushao added the UX label Aug 4, 2022
@junrushao junrushao changed the title [Discuss][Parser] Feature requests for the Relax parser [UX][TVMScript] Feature requests Aug 4, 2022
@slyubomirsky
Copy link
Collaborator Author

Integer literals should be parsed into scalars:

@R.function
def return_int() -> Tensor((), "int32"):
    return 1 # presently fails to parse because it treats 1 as an IntImm, but should be parsed into a constant literal

Similarly, empty tuples should parse:

@R.function
def return_unit() -> Tuple():
    return () # presently fails to parse

@junrushao
Copy link
Member

Integer literals should be parsed into scalars:

@R.function
def return_int() -> Tensor((), "int32"):
    return 1 # presently fails to parse because it treats 1 as an IntImm, but should be parsed into a constant literal

Similarly, empty tuples should parse:

@R.function
def return_unit() -> Tuple():
    return () # presently fails to parse

Those will be fixed with the new parser. CC: @Hzfengsy @yongwww

@slyubomirsky
Copy link
Collaborator Author

slyubomirsky commented Aug 5, 2022

Maybe something to consider per the thought I raise in this comment: Parsing for variadic functions. Maybe printing is the only variadic function we'll want, but it might be something to consider. My mistake, this is already supported. Though the linked PR runs into a quirk with attributes for a variadic function--it could be fixed in the code generator by putting attributes first for a variadic packed func, but that may be more complicated than it's worth.

@tqchen
Copy link
Contributor

tqchen commented Dec 3, 2022

seems are supported through new parser

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants