Skip to content

Conversation

@cyx-6
Copy link
Contributor

@cyx-6 cyx-6 commented Oct 25, 2022

This PR introduces parser for TIR as part of #12442

Co-authored-by: yongwww [email protected]

This PR introduces parser for TIR as part of apache#12442

Co-authored-by: yongwww <[email protected]>
@tvm-bot
Copy link
Collaborator

tvm-bot commented Oct 25, 2022

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.

  • No users to tag found in teams: tvmscript See #10317 for details
  • Built docs for commit 4e5a2ac can be found here.

Generated by tvm-bot

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
Contributor

@MasterJH5574 MasterJH5574 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @cyx-6 for the upstreaming effort!

@MasterJH5574 MasterJH5574 merged commit 7950271 into apache:main Oct 25, 2022
@junrushao
Copy link
Member

Thanks for the great contribution!

I recognize that most of the TIR syntax have been covered by the IRBuilder tests accordingly, for example, T.block has been carefully tested in IRBuilder.

This decoupling approach is great because it largely simplifies testing effort, avoids end-to-end integration tests and favors more fine grained unittests. Therefore, given this PR is tested against all the nuanced cases not covered by the IRBuilder tests, so i would say the test coverage is good enough.

On the other hand, I do think it’s desirable for us to see some end-to-end tests to make sure the parser is functioning properly, especially when e2e tests are running super fast for in the parser. A couple of test cases I would love to see in a follow-up PR:

  • 1 + 1 is not simplified to 2, but honestly translated to tvm.tir.Add
  • T.Buffer() in function signature is correctly parsed
  • Parser error is properly reported in IRBuilder calls, for example, calling an non-existing intrinsic.

Would be great if we could have a follow-up PR for such tests! Thanks a lot!

@MasterJH5574
Copy link
Contributor

@junrushao Thanks for bring it up! I agree that it makes sense to follow up with more comprehensive test cases.

xinetzone pushed a commit to daobook/tvm that referenced this pull request Nov 10, 2022
xinetzone pushed a commit to daobook/tvm that referenced this pull request Nov 25, 2022
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.

5 participants