Skip to content

[Unity] Allow eliminating only call nodes in CSE pass - #14895

Merged
masahi merged 1 commit into
apache:unityfrom
masahi:cse-call-only
May 24, 2023
Merged

[Unity] Allow eliminating only call nodes in CSE pass#14895
masahi merged 1 commit into
apache:unityfrom
masahi:cse-call-only

Conversation

@masahi

@masahi masahi commented May 19, 2023

Copy link
Copy Markdown
Member

The CSE pass eliminates expressions very eagerly. This can result in an undesirable result, for example relax.arange(...) op which requires all of its inputs to be PrimValue complains when its inputs are CSE variables.

https://github.com/apache/tvm/blob/unity/src/relax/op/tensor/create.cc#L248-L253

  File "/Users/masa/projects/dev/tvm/src/relax/ir/block_builder.cc", line 138
TVMError: Arange expects the `start` to be a PrimValue, but got relax.expr.DataflowVar

That error can be easily fixed, but I've also hit similar issues with other ops like strided_slice as well. Since I only need common CallNodes to be eliminated, I'm adding an option to avoid the default fine-grained elimination.

cc @slyubomirsky @sunggg

@tvm-bot

tvm-bot commented May 19, 2023

Copy link
Copy Markdown
Collaborator

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

@github-actions
github-actions Bot requested a review from slyubomirsky May 19, 2023 22:10
@slyubomirsky

slyubomirsky commented May 23, 2023

Copy link
Copy Markdown
Contributor

The proposed changes are implemented correctly and I don't object to them per se. But I wonder if this is the right solution. Perhaps we should exclude PrimValues from CSE in general instead? There isn't really a good reason to CSE them in the first place. Maybe there are other types of expressions we shouldn't CSE either. (One other candidate I could think of: scalar constants. Shape expressions might be another.)

@slyubomirsky slyubomirsky left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I've decided to approve because, while we might consider a setting to leave only scalar constants, shape constants, and PrimValues (we could call them atomic values), calls-only is indeed a reasonable simplification that will account for almost all expensive computations. (Other situations besides calls, though, where CSE is useful include large tensor constants, repeated inner functions, and large tuples.)

@masahi
masahi merged commit 6bb531f into apache:unity May 24, 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.

3 participants