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

[Refactor] Move VarUseDefAnalysis to header file #14185

Merged
merged 11 commits into from
Mar 7, 2023

Conversation

yzh119
Copy link
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
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
include/tvm/tir/analysis.h Outdated Show resolved Hide resolved
@yzh119
Copy link
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
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.

include/tvm/tir/analysis.h Outdated Show resolved Hide resolved
src/tir/analysis/var_use_def_analysis.h Outdated Show resolved Hide resolved
src/tir/analysis/var_use_def_analysis.h Show resolved Hide resolved
src/tir/transforms/split_host_device.cc Show resolved Hide resolved
@tqchen
Copy link
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