Skip to content

Conversation

@masahi
Copy link
Member

@masahi masahi commented Mar 30, 2023

Compared to the typical, structural matching (e.g. matching is_op based pattern against CallNode), graph pattern matching requires additionally specifying used_by constraints. This leads to an obvious redundancy in the usage:

matmul1 = is_op("relax.matmul")(inp_pat, Q_weight_pat)
matmul2 = is_op("relax.matmul")(inp_pat, K_weight_pat)
matmul3 = is_op("relax.matmul")(inp_pat, V_weight_pat)

inp_pat.used_by(matmul1, 0)
inp_pat.used_by(matmul2, 0)
inp_pat.used_by(matmul3, 0)

Q_weight_pat.only_used_by(matmul1, 1)
K_weight_pat.only_used_by(matmul2, 1)
V_weight_pat.only_used_by(matmul3, 1)

This PR hacks is_op constructor by automatically adding such used_by constraints between caller and callee patterns. See the simplified test case for QOL improvement.

@ganler @sunggg

@tvm-bot
Copy link
Collaborator

tvm-bot commented Mar 30, 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.

Generated by tvm-bot

inline tvm::runtime::Map<DFPattern, Var> MatchGraphDefault(const DataflowBlock& dfb,
Optional<Var> start_hint = NullOpt,
bool must_include_hint = false) {
return MatchGraph(PatternContext::Current(), dfb, start_hint, must_include_hint);
Copy link
Member Author

Choose a reason for hiding this comment

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

This gets removed since it is not used anywhere but it touches PatternContext::Current() (which is now Optional)

Comment on lines -1042 to -1044
Q_weight_pat.only_used_by(matmul1, 1)
K_weight_pat.only_used_by(matmul2, 1)
V_weight_pat.only_used_by(matmul3, 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand used_by can be removed now as it is implied by a function call. However, only_used_by is a stronger constraint that cannot be implied via a fn call right?

Copy link
Contributor

Choose a reason for hiding this comment

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

But yeah I guess you meant oftentimes we don't fold Q, K, and V so it is fine if they are being used by others at the same time.

Copy link
Contributor

@ganler ganler left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the improvement!

@cyx-6 cyx-6 merged commit adf256d into apache:unity Mar 30, 2023
tqchen pushed a commit that referenced this pull request Apr 1, 2023
…is_op` pattern (#14439)

* Automatically add used-by constraints for is_op pattern

* [DOC fix] pass context -> constraint context
tqchen pushed a commit that referenced this pull request Apr 1, 2023
…is_op` pattern (#14439)

* Automatically add used-by constraints for is_op pattern

* [DOC fix] pass context -> constraint context
tqchen pushed a commit that referenced this pull request Apr 1, 2023
…is_op` pattern (#14439)

* Automatically add used-by constraints for is_op pattern

* [DOC fix] pass context -> constraint context
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.

4 participants