Skip to content

[Refactor] Move VarUseDefAnalysis to header file#14185

Merged
Hzfengsy merged 11 commits intoapache:mainfrom
yzh119:refactor-var-use-dep-analysis
Mar 7, 2023
Merged

[Refactor] Move VarUseDefAnalysis to header file#14185
Hzfengsy merged 11 commits intoapache:mainfrom
yzh119:refactor-var-use-dep-analysis

Conversation

@yzh119
Copy link
Copy Markdown
Member

@yzh119 yzh119 commented Mar 3, 2023

Motivation

UndefinedVars is a frequently used function in our codebase and its implementation relies on VarUseDefAnalysis class which is more general, currently we expose UndefinedVars in analysis.h, but both the definitions of UndefinedVars and VarUseDefAnalysis resides in split_host_device.cc.

This PR moves VarUseDefAnalysis class to analysis.h so that developers can use it in other files that requires use/def analysis than split_host_devices.cc. We create a var_use_def_analysis.cc under src/src/analysis for the implementations of both UndefinedVars and VarUseDefAnalysis.

Notes

We rename VarUseDefAnalysis to VarUseDefAnalyzer.

cc @Hzfengsy @Lunderberg

@tvm-bot
Copy link
Copy Markdown
Collaborator

tvm-bot commented Mar 3, 2023

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: refactor See #10317 for details

Generated by tvm-bot

@github-actions github-actions Bot requested review from Hzfengsy and Lunderberg March 3, 2023 10:19
Comment thread include/tvm/tir/analysis.h Outdated
@yzh119
Copy link
Copy Markdown
Member Author

yzh119 commented Mar 4, 2023

@Hzfengsy @Lunderberg @tqchen I have decoupled the previous VarDefUseAnalyzer class into three classes:

  1. VarUseDefAnalyzer which is a StmtExprVisitor, we expose it to our files via a header under src
  2. DeviceInfoCollector which is a StmtVisitor that collects program information about shared memory usage, thread axis/extents etc.
  3. UnreferencedLetRemover which removes redundant let stmt/exprs.

I know this class has been used for long time and we don't want any changes on its behavior, so we need to be careful in this refactor, and please correct me if I made any mistakes. One thing I'm not sure is if we need to couple the three functionalities together in a single pass, or we can decouple them like what I did in the refactor.

If my refactor doesn't work, I'm totally okay if we keep the original class and create a simpler VarDefUseAnalyzer for light-weight analysis use (e.g. UndefinedVars).

Copy link
Copy Markdown
Contributor

@Lunderberg Lunderberg left a comment

Choose a reason for hiding this comment

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

LGTM! Just a couple of small nitpicks.

Comment thread include/tvm/tir/analysis.h Outdated
Comment thread src/tir/analysis/var_use_def_analysis.h Outdated
Comment thread src/tir/analysis/var_use_def_analysis.h
Comment thread src/tir/transforms/split_host_device.cc
@tqchen
Copy link
Copy Markdown
Member

tqchen commented Mar 7, 2023

cc @Hzfengsy please take another look and manage the PR

@Hzfengsy Hzfengsy merged commit 082c443 into apache:main Mar 7, 2023
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