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

Fix InternalError in StaticPlanBlockMemory when visiting DataflowBlockNode #17501

Merged
merged 3 commits into from
Nov 13, 2024

Conversation

Thrsu
Copy link
Contributor

@Thrsu Thrsu commented Nov 1, 2024

This PR fixes an internal error #17488

This error happens because the visitor class StorageAllocatorBaseVisitor does not correctly handle DataflowBlockNode instances. Specifically, the VisitBindingBlock_ method is not overridden for DataflowBlockNode, leading to an empty block_stack_ when it is expected to contain the current block.

To fix this issue, we need to override the VisitBindingBlock_ method for const DataflowBlockNode* in the StorageAllocatorBaseVisitor class. By doing so, we ensure that the block_stack_ is correctly managed when visiting dataflow blocks, similar to how it is managed for regular binding blocks.

@Thrsu
Copy link
Contributor Author

Thrsu commented Nov 4, 2024

Could you please help review this pull request? @tqchen @yongwww

@tqchen
Copy link
Member

tqchen commented Nov 4, 2024

cc @MasterJH5574 can you take a look

@tqchen tqchen requested a review from MasterJH5574 November 4, 2024 14:41
@MasterJH5574
Copy link
Contributor

Hi @Thrsu, thanks for contributing! Actually we expect all dataflow blocks to have been removed in order to properly apply memory planning, given we no longer need the dataflow block information in subsequent lowering passes. Here is our lowering pipeline and you can see that we apply ToNonDataflow before other lowering passes, including the memory planning

transform.ToNonDataflow(),
transform.RemovePurityChecking(),
transform.CallTIRRewrite(),
transform.StaticPlanBlockMemory(),

So if possible, do you mind sharing the reason why you want the memory planning pass to work with dataflow blocks? A code example may be sufficient, thanks!

@tqchen
Copy link
Member

tqchen commented Nov 6, 2024

@MasterJH5574 in that case, would be great to have a proper error message when assumption do not hold

@Thrsu
Copy link
Contributor Author

Thrsu commented Nov 6, 2024

Hi @Thrsu, thanks for contributing! Actually we expect all dataflow blocks to have been removed in order to properly apply memory planning, given we no longer need the dataflow block information in subsequent lowering passes. Here is our lowering pipeline and you can see that we apply ToNonDataflow before other lowering passes, including the memory planning

transform.ToNonDataflow(),
transform.RemovePurityChecking(),
transform.CallTIRRewrite(),
transform.StaticPlanBlockMemory(),

So if possible, do you mind sharing the reason why you want the memory planning pass to work with dataflow blocks? A code example may be sufficient, thanks!

Thanks for your response! @tqchen @MasterJH5574
I want to customize a pass pipeline for exploring the possibility of improving the optimization performance. However, it crashed unexpectedly due to the assumptions about the pass. This PR aims to enable the StaticPlanBlockMemory optimization to run correctly in such conditions.

@MasterJH5574
Copy link
Contributor

@Thrsu I see. Thanks for elaborating! The memory planning pass also has a dependency on the transform.CallTIRRewrite pass, which you may also need to be careful about. Could you please add a unit test to this file, showing that the memory planning works for dataflow blocks? With the unit test we can merge this PR. Thanks!

https://github.com/apache/tvm/blob/main/tests/python/relax/test_transform_static_plan_block_memory.py

@Thrsu
Copy link
Contributor Author

Thrsu commented Nov 11, 2024

@Thrsu I see. Thanks for elaborating! The memory planning pass also has a dependency on the transform.CallTIRRewrite pass, which you may also need to be careful about. Could you please add a unit test to this file, showing that the memory planning works for dataflow blocks? With the unit test we can merge this PR. Thanks!

https://github.com/apache/tvm/blob/main/tests/python/relax/test_transform_static_plan_block_memory.py

@MasterJH5574 Thank you for the guidance! I've added a unit test to the specified file, demonstrating that the memory planning pass works for dataflow blocks as requested. Could you please review the latest changes? Thank you for your time and assistance!

@MasterJH5574
Copy link
Contributor

@Thrsu Thank you for adding the unit test! It looks good to me :-)

@MasterJH5574 MasterJH5574 merged commit 3d96623 into apache:main Nov 13, 2024
14 checks passed
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.

3 participants