Skip to content

[TIR, TVMScript] Update printer / parser to make T.allocate return buffer var - #12412

Merged
Lunderberg merged 8 commits into
apache:mainfrom
vinx13:feat/decl-buffer-1-tvmscript
Aug 31, 2022
Merged

[TIR, TVMScript] Update printer / parser to make T.allocate return buffer var#12412
Lunderberg merged 8 commits into
apache:mainfrom
vinx13:feat/decl-buffer-1-tvmscript

Conversation

@vinx13

Copy link
Copy Markdown
Member
  • Updated TVMScript syntax of T.allocate to return buffer var.
  • Added syntax sugar for T.decl_buffer. When data field is not specified, data will be implicitly created via Allocate stmt.
  • Updated the existing test cases. Most test cases can be updated by changing T.allocate to T.decl_buffer. T.allocate in some tests are updated to T.allocate + T.buffer_decl, to maintain the legacy behavior of allocation and implicit buffer declaration (will be followed up in future PR to adopt T.decl_buffer).

Related RFC: apache/tvm-rfcs#87

cc @junrushao1994@Lunderberg@wrongtest-intellif@cyx-6@tqchen

@vinx13
vinx13force-pushed the feat/decl-buffer-1-tvmscript branch from 628b844 to f2a0a1aCompareAugust 12, 2022 20:41
@vinx13vinx13 mentioned this pull request Aug 12, 2022
4 tasks
@vinx13
vinx13force-pushed the feat/decl-buffer-1-tvmscript branch 8 times, most recently from 1f07d88 to 45a5ac8CompareAugust 16, 2022 04:02

@LunderbergLunderberg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good! I have a couple of questions on the implementation side, but overall it looks very good!

}

void VisitStmt_(const DeclBufferNode* op) final {
buffers_declared_.insert(op->buffer.get());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we also track which buffers have gone out of scope? If I'm understanding it correctly, a single DeclBufferNode would also allow for usage outside of the DeclBufferNode::body, where I'd expect it to only apply within the scope of the node.

Comment threadsrc/printer/tvmscript_printer.cc Outdated
Comment threadsrc/printer/tvmscript_printer.cc
Comment threadtests/python/contrib/test_ethosu/test_copy_compute_reordering.py Outdated
Comment threadtests/python/contrib/test_ethosu/test_hoist_allocates.py
Comment threadtests/python/contrib/test_ethosu/test_tir_to_cs_translator.py Outdated
Comment threadtests/python/unittest/test_tir_renew_defs.py
Comment threadtests/python/unittest/test_tir_transform_flatten_buffer.py
Comment threadtests/python/unittest/test_tir_transform_unroll_loop.py
Comment threadsrc/printer/tvmscript_printer.cc Outdated

@vinx13vinx13 left a comment

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

To answer the question about T.allocate vs T.decl_buffer update:
There are two ways to update the existing TVM scripts.

  1. Update T.allocate to T.allocate + T.buffer_decl. This is strictly equivalent to the TIR before.
  2. Update T.allocate to T.allocate + T.decl_buffer, (or the syntax sugar form, only a single T.decl_buffer statement without data argument). This adds a DeclBuffer node in TIR, which is not strictly equivalent to the TIR before. This is the preferred way (as it uses explicit DeclBuffer) but might require updating the existing passes. To minimize changes to existing passes in this PR, in the case this method doesn't work out-of-box, we will choose method 1 for now.

Comment threadtests/python/unittest/test_tir_transform_unroll_loop.py
Comment threadsrc/printer/tvmscript_printer.cc Outdated
Comment threadsrc/printer/tvmscript_printer.cc
Comment threadsrc/printer/tvmscript_printer.cc Outdated
@Lunderberg

Copy link
Copy Markdown
Contributor

That makes sense for minimizing the changes to the generated TIR, so that those can be handled in an independent change. Marking all of those conversations as resolved.

@vinx13
vinx13force-pushed the feat/decl-buffer-1-tvmscript branch from 45a5ac8 to 071b81dCompareAugust 30, 2022 21:54

@LunderbergLunderberg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

xinetzone pushed a commit to daobook/tvm that referenced this pull request Nov 25, 2022
…ffer var (apache#12412)
* Updated TVMScript syntax of `T.allocate` to return buffer var.
* Added syntax sugar for `T.decl_buffer`. When `data` field is not
specified, `data` will be implicitly created via `Allocate` stmt.
* Updated the existing test cases. Most test cases can be updated by
changing `T.allocate` to `T.decl_buffer`. `T.allocate` in some tests
are updated to `T.allocate` + `T.buffer_decl`, to maintain the
legacy behavior of allocation and implicit buffer declaration (will
be followed up in future PR to adopt `T.decl_buffer`).
Sign up for freeto 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.

2 participants

@vinx13@Lunderberg