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

[Refactor] Split transformer.py into StmtBuilder and ExprBuilder (Stage 1) #2495

Merged
merged 41 commits into from
Jul 29, 2021

Conversation

xumingkuan
Copy link
Contributor

@xumingkuan xumingkuan commented Jul 6, 2021

Related issue = #2193

This PR replaces ASTTransformerPreprocess with StmtBuilder and ExprBuilder.

This PR doesn't modify the behavior (and the code) of the other 2 passes in transformer.py.

Detailed changeset:

  • Move all members of the class ASTTransformerPreprocess to BuilderContext. These members should be context instead of members of a pass.
  • Move ScopeGuard, parse_stmt, parse_expr to ast_builder_utils.py.
  • Rename all functions in ASTTransformerPreprocess from visit_Xxx to build_Xxx and move them to expr_builder.py and stmt_builder.py.
  • Remove all generic_visit in ASTTransformerPreprocess in transformer.py. Since we need to know if it's a statement or an expr before building the subtree of AST, we cannot use a general generic_visit. The invocations of this function is rewritten as build_expr, build_exprs, build_stmt, build_stmts.
  • Note that build_exprs and build_stmts create a variable_scope. Also note that I do not use a list comprehension in these two functions because with will not work properly otherwise.
  • Note that build_Module does not use build_stmts, just like its not using generic_visit before this PR. This is because if we create a variable_scope associated with the statement list (Python list here), the parameters passed into the Taichi kernel will be deleted when destructing the variable scope.
  • Fix some missing fields required by the Python AST but not required when executing it (compiling Taichi). (Otherwise the AST may not be printed properly.)
  • Throw exception if we encounter an AST node without corresponding builder (e.g., if build_Name is not implemented in ExprBuilder and we invoke build_expr with a Name node as the parameter).
  • Throw TaichiSyntaxError if the Python keyword global/nonlocal is used or when a Python set (including set comprehension) is used.
  • Add many builder functions which look trivial and in fact unnecessary if we use visit_Xxx before this PR. I think there's a benefit of writing these functions explicitly -- they show which features in Python are supported in Taichi, and throw exceptions when users use features in Python that are not considered by Taichi developers, which may or may not be supported.

Note:

  • Not supported both before and after this PR:
    • a = f'aaa{123}' (ast.JoinedStr, together with ast.FormattedValue)
    • b = a[1:2] (ast.Slice)
    • b = (n**2 for n in aa if n>5 if n<10) (ast.GeneratorExp)
    • a = lambda x, y: x + y (ast.Lambda)
  • (Probably incorrectly) supported before this PR but unsupported after this PR:
    • (b): ti.i32 = 1 (ast.AnnAssign)
    • yield a and yield from a (ast.Yield and ast.YieldFrom)
    • {1, 2, 3} and {x for x in numbers} (ast.Set and ast.SetComp)
    • class Foo: ... (ast.ClassDef)
    • def foo() (ast.FunctionDef) in a kernel
  • Not supported but did not throw syntax error before this PR:
    • ast.AsyncFunctionDef, ast.Await, ast.AsyncFor, ast.AsyncWith do not throw syntax error before this PR. They are not considered in this PR.
    • global and nonlocal throw TaichiSyntaxError after this PR. They may crash later before this PR.

@CLAassistant
Copy link

CLAassistant commented Jul 20, 2021

CLA assistant check
All committers have signed the CLA.

@xumingkuan xumingkuan changed the title [Refactor] Split transformer.py into StmtBuilder and ExprBuilder [Refactor] Split transformer.py into StmtBuilder and ExprBuilder (Stage 1) Jul 23, 2021
Copy link
Member

@k-ye k-ye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this huge refactoring!

  1. How many stages do you expect there will be?
  2. Could you write some comments to guide the review process?

# note that we can only return an ast.Expr instead of an ast.Call.
return node
# A statement with a single expression.
# TODO(#2495): Deprecate maybe_transform_ti_func_call_to_stmt
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder what's the situation with maybe_transform_ti_func_call_to_stmt after this PR? Also does this method generate a ExprStmt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. This PR should not modify the behavior of the Python frontend (the whole transformer.py together with new files introduced in this PR).

@xumingkuan
Copy link
Contributor Author

Thanks for this huge refactoring!

  1. How many stages do you expect there will be?
  2. Could you write some comments to guide the review process?
  1. I would expect that there will be only 2 stages -- the other one is to remove maybe_transform_ti_func_call_to_stmt and merge the multiple-return check into the new ast builders. I think there will be only one pass in Python after this refactoring.
  2. Sure!

@xumingkuan
Copy link
Contributor Author

Do we support Python 3.9 now?

@k-ye
Copy link
Member

k-ye commented Jul 27, 2021

Do we support Python 3.9 now?

Yep

@k-ye k-ye requested a review from ljcc0930 July 28, 2021 00:48
@xumingkuan
Copy link
Contributor Author

Is NamedExpr unsupported in Python 3.6?

Copy link
Member

@k-ye k-ye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!!

Copy link
Contributor

@ljcc0930 ljcc0930 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@k-ye
Copy link
Member

k-ye commented Jul 29, 2021

FYI you don't have to [skip ci] now.... New commit will automatically cancel the previous CI 😆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
large changeset The PR contains a large changeset and reviewing may take times
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants