Skip to content
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

[Fix][TVMScript] Fix LetStmt printing logic #13900

Merged
merged 1 commit into from
Feb 3, 2023
Merged

Conversation

cyx-6
Copy link
Contributor

@cyx-6 cyx-6 commented Feb 1, 2023

This PR is the bug fix reported in #13892. Initially, we mix the logic of LetStmt docsifying method with and without concise scoping. For example, in

x = T.var("int32")
with T.let(x, 0):

x in the LetStmt works as a right value, while in

x: T.int32 = 0

x in the LetStmt works as a left value as result.
Our old logic mixed them together to generate the wrong code for the first case.
Meanwhile, during the fix, we found another bug in concise scoping check. For example, we have

x = T.var("int32")
y = T.var("int32")
with T.let(x, y):
  with T.let(y, 0):

here we should not output

x = T.var("int32")
y = T.var("int32")
with T.let(x, y):
  y: int32 = 0

becase this will define a new y_1: int32 = 0 indeed, due the the variable shadowing logic of the parser, which is different from the y we define and refer to.
Our concise scoping v: ... = ... should launch if and only if the v is never defined before.
Otherwise, we use with T.let(v, ...): instead.

@tvm-bot
Copy link
Collaborator

tvm-bot commented Feb 1, 2023

Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.

Generated by tvm-bot

@junrushao
Copy link
Member

@wrongtest-intellif
Copy link
Contributor

x = T.var("int32")
y = T.var("int32")
with T.let(x, y):
  with T.let(y, 0):

Thank you for the fix! Here is my small question for the example, does it shoud be

with T.let(y, 0):
  with T.let(x, y):

@junrushao
Copy link
Member

Off the topic, in the future, we might want to change the syntax to:

with T.LetStmt(a) as b:

@cyx-6
Copy link
Contributor Author

cyx-6 commented Feb 3, 2023

@wrongtest-intellif Oh, that is just an example of the case that a LetStmt->var - y has usage before the its new value. I picked the outer LetStmt->value field for the usage of y for convenience. So never mind its logic, it makes no sense, except its format.

@lightzhan-intellif
Copy link
Contributor

Off the topic, in the future, we might want to change the syntax to:

with T.LetStmt(a) as b:

I prefer this. The current one looks a little bit confusing from the view of native python users.

@junrushao
Copy link
Member

Sounds good! I am merging this bugfix in first given it seems non-controversial.

Right now I am focusing on implementing TVMScript printer for Relax (coming soon). And after this is done, the syntax we mentioned above will be introduced with backward compatibility guarantee.

@junrushao junrushao merged commit 98008c2 into apache:main Feb 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants