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

[TIR][Hybrid] Hybrid Script Support for TIR #6227

Merged
merged 11 commits into from
Aug 10, 2020
Merged

[TIR][Hybrid] Hybrid Script Support for TIR #6227

merged 11 commits into from
Aug 10, 2020

Conversation

spectrometerHBH
Copy link
Contributor

In [RFC] Hybrid Script Support for TIR, we plans to utilize a subset of Python AST that can express every TIR node. In this PR, we introduce hybrid script printer&parser with basic infra and a complete parsing/printing feature.

@spectrometerHBH
Copy link
Contributor Author

cc @tqchen @junrushao1994 @Hzfengsy

@tqchen tqchen added the status: need test case need test cases to cover the change label Aug 6, 2020
@tqchen
Copy link
Member

tqchen commented Aug 6, 2020

Thanks @spectrometerHBH , some quick items:

  • run ./tests/lint/git-clang-format.sh -i to format the code
  • add testcases to cover some of the existing logics

@tqchen
Copy link
Member

tqchen commented Aug 7, 2020

cc @Hzfengsy @weberlo @junrushao1994 @were please help to review the PR

@tqchen tqchen added status: need review and removed status: need test case need test cases to cover the change labels Aug 7, 2020
@tqchen tqchen self-assigned this Aug 7, 2020
python/tvm/hybrid/__init__.py Outdated Show resolved Hide resolved
python/tvm/hybrid/parser.py Show resolved Hide resolved
python/tvm/hybrid/parser.py Outdated Show resolved Hide resolved
python/tvm/hybrid/parser.py Outdated Show resolved Hide resolved
python/tvm/hybrid/registry.py Outdated Show resolved Hide resolved
python/tvm/hybrid/scope_handler.py Show resolved Hide resolved
tests/python/unittest/test_hybrid_roundtrip.py Outdated Show resolved Hide resolved
python/tvm/hybrid/parser.py Outdated Show resolved Hide resolved
python/tvm/hybrid/scope_handler.py Outdated Show resolved Hide resolved
python/tvm/hybrid/scope_handler.py Outdated Show resolved Hide resolved
python/tvm/hybrid/utils.py Show resolved Hide resolved
python/tvm/hybrid/utils.py Show resolved Hide resolved
python/tvm/hybrid/parser.py Outdated Show resolved Hide resolved
@junrushao
Copy link
Member

Thanks! I will take a look this weekend :-)

"""

_binop_maker = {
ast.Add: tir.Add,
Copy link
Member

Choose a reason for hiding this comment

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

For followup PR.

To enable automatic conversion in frontend(syntax sugar), we might want to be able change to the operator version instead of the tir.Add(requires both operands to share the same type).

Copy link
Member

@tqchen tqchen left a comment

Choose a reason for hiding this comment

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

Overall LTGTM, we can handle more syntax sugar and error reporting support in the follow up PRs. wait feedbacks from @junrushao1994 @Hzfengsy

return "\n " + src_line + "\n " + " " * col_offset + "^\n" + "ParserError in line " \
+ str(lineno) + " : " + message

def report_error(self, message, lineno=None, col_offset=None):
Copy link
Member

Choose a reason for hiding this comment

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

Please add a test case of parsing error to make sure report error works properly(e.g. report the right line). Use pytest.raises

@tqchen
Copy link
Member

tqchen commented Aug 10, 2020

cc @Hzfengsy @junrushao1994 please take another look. @spectrometerHBH please add the testcase about report error then it looks good from my side.

@@ -60,6 +60,9 @@
# tvm.parser
from . import parser

# tvm tir hybrid script
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# tvm tir hybrid script
# tvm hybrid script

Copy link
Member

@Hzfengsy Hzfengsy 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
Member

@junrushao junrushao left a comment

Choose a reason for hiding this comment

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

LGTM

@tqchen tqchen merged commit 87d6ccd into apache:master Aug 10, 2020
@tqchen
Copy link
Member

tqchen commented Aug 10, 2020

Thanks @spectrometerHBH @junrushao1994 @Hzfengsy . This PR is now merged

wjliu1998 pushed a commit to wjliu1998/incubator-tvm that referenced this pull request Aug 13, 2020
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Aug 26, 2020
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Aug 26, 2020
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Aug 26, 2020
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Sep 2, 2020
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request Sep 3, 2020
@spectrometerHBH spectrometerHBH deleted the hybrid branch September 8, 2020 00:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants