Skip to content

[Unity] Remove unused local function definitions#15507

Merged
kparzysz-quic merged 1 commit intoapache:unityfrom
Lunderberg:unity_remove_unused_local_functions
Aug 10, 2023
Merged

[Unity] Remove unused local function definitions#15507
kparzysz-quic merged 1 commit intoapache:unityfrom
Lunderberg:unity_remove_unused_local_functions

Conversation

@Lunderberg
Copy link
Copy Markdown
Contributor

Prior to this commit, the relax.analysis.remove_all_unused utility only removed variable bindings that occur within a dataflow block. This satisfies that the condition that a variable can be removed if is unused and if its computation has no side effects. However, it doesn't include any values from a BindingBlock.

This commit updates the RemoveUnusedVars pass to also inspect BindingBlock instances for removeable variables. Currently, this is limited to unused local function definitions.

Prior to this commit, the `relax.analysis.remove_all_unused` utility
only removed variable bindings that occur within a dataflow block.
This satisfies that the condition that a variable can be removed if is
unused and if its computation has no side effects.  However, it
doesn't include any values from a `BindingBlock`.

This commit updates the `RemoveUnusedVars` pass to also inspect
`BindingBlock` instances for removeable variables.  Currently, this is
limited to unused local function definitions.
@tvm-bot
Copy link
Copy Markdown
Collaborator

tvm-bot commented Aug 8, 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


optimized = remove_all_unused(IdentityUnused["main"])

GroundTruth = IdentityUnused
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.

Shouldn't the two unuseds be removed? I think this whole algorithm (not your change specifically) is somewhat deficient if it doesn't keep track of newly-unused objects.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

For this specific change, because (to my understanding), the call_dps_packed doesn't guarantee that it has no side effects. Within a R.dataflow() environment, the expression may not have side effects, so it is safe to remove.

For newly-unused objects, I believe that is handled by the GetUnusedVars function, called during construction. It iteratively walks from the function outputs to the predecessors required to generate those outputs, finding the set of all variables required to generate the output. Anything not within that set is considered unused and can be removed, even it occurs within another expression (because that expression is also going to be removed).

@kparzysz-quic kparzysz-quic merged commit bcc9a68 into apache:unity Aug 10, 2023
@Lunderberg Lunderberg deleted the unity_remove_unused_local_functions branch August 10, 2023 14:03
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