[Unity][BlockBuilder] Depracate BlockBuilder.get() and change it to BlockBuilder.finalize()#16090
Merged
Hzfengsy merged 5 commits intoapache:unityfrom Nov 28, 2023
Merged
Conversation
Ubospica
force-pushed
the
unity-dev/2023-11-06-builder-rename
branch
from
6b36059 to
a5a7d78
Compare
Hzfengsy
reviewed
Nov 9, 2023
python/tvm/relax/block_builder.py
Outdated
| # avoid circular import | ||
| from .transform import NormalizeGlobalVar | ||
|
|
||
| mod = _ffi_api.BlockBuilderGetContextIRModule(self) # type: ignore |
Member
There was a problem hiding this comment.
Is it possible to embed the pass inside the BlockBuilderGetContextIRModule? aka we can have the same behavior on both python and cpp side
Contributor
Author
There was a problem hiding this comment.
We have considered that. Here the problem is NormalizeGlobalVar will invalidate all global vars previously acquired. But I cannot ensure all pass will follow the assumption that no global var will be used again after BlockBuilderGetContextIRModule. Actually, some passes like AllocateWorkspace might already break the assumption.
On the other hand, on python side BlockBuilder is mainly to build a new IRModule from scratch, and not involve too complex logic. So apply NormalizeGlobalVar to bb.get() could be a good choice.
Member
There was a problem hiding this comment.
I think that we should not turn it on by default. Because there are not too many cases we create new public functions, and renormlaization can be costly. We should however add wellform check to ensure normal passes do not generate code that violate this assumption
Maybe we do not make it part of BlockBuilder.get() but instead add a new function BlockBuilder.finalize() and add remark that it will also normalizes the calls.
Contributor
Author
There was a problem hiding this comment.
For the cost of renormalization, currently if no function needs renaming, the pass will return directly and not get into the AST. In this case I think the cost is not that high.
The wellformed check is already added.
And for bb.finalize(), I think this will make it confused to use bb.finalize() or bb.get(), because their only difference is whether apply NormalizeGlobalVar or not. If we require users only use bb.finalize() at the end of the IRModule building process, that means a major behaviour change and all related tests might need to be changed.
Member
There was a problem hiding this comment.
OK, we can keep this for now and keep the documentaiton.
Ubospica
changed the title
BlockBuilder.get()BlockBuilder.get() and change it to BlockBuilder.finalize()
Contributor
Author
|
cc @Hzfengsy @tqchen @junrushao Plz take a look |
Contributor
Author
|
@tvm-bot rerun |
Contributor
|
Failed to re-run CI in https://github.com/apache/tvm/actions/runs/6917012591 Detailswith response |
Contributor
Author
|
@tvm-bot rerun |
Contributor
|
Failed to re-run CI in https://github.com/apache/tvm/actions/runs/6917034777 Detailswith response |
Contributor
Author
|
@tvm-bot rerun |
Contributor
|
Failed to re-run CI in https://github.com/apache/tvm/actions/runs/6920387363 Detailswith response |
Member
|
@Ubospica you need to push a new commit |
Ubospica
force-pushed
the
unity-dev/2023-11-06-builder-rename
branch
from
2b6b68c to
0c5d3e2
Compare
Hzfengsy
approved these changes
Nov 28, 2023
Archermmt
pushed a commit
to Archermmt/tvm
that referenced
this pull request
Dec 18, 2023
… `BlockBuilder.finalize()` (apache#16090) * 1108 * fix ci * 1109 * finished * fix ci
vinx13
added a commit
to vinx13/tvm
that referenced
this pull request
Jan 9, 2024
…ge it to `BlockBuilder.finalize()` (apache#16090)" This reverts commit 2dcb871.
masahi
pushed a commit
to masahi/tvm
that referenced
this pull request
Jan 10, 2024
…ge it to `BlockBuilder.finalize()` (apache#16090)" This reverts commit 2dcb871.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previously BlockBuilder would have a problem about naming of public function if we
Previously, BlockBuilder would rename the latter, i.e. the public function. That led the name of the public function did not match the name of its global_symbol attribute.
To tackle this problem, this PR introduces a new pass NormalizeGlobalVar. This pass will possibly rename the GlobalVar in an IRModule to ensure these properties:
This PR adds a member function
bb.finalize()to be used at the end of the building process. It will call NormalizeGlobalVar to ensure the invariant, and returns the final IRModule. It essentially substitutes the functionality ofbb.get(), so we mark the latter deprecated.About naming: we can still use the interface
bb.get()and execute a NormalizeGlobalVar pass in it. But suchbb.get()can only be called once at the end of the building process. So we rename it tobb.finalize()for clearity and alignment with C++ side.cc @junrushao @tqchen