You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by "Lunderberg (via GitHub)" <gi...@apache.org> on 2024/03/27 19:47:42 UTC
[PR] [Bugfix][Relax] BlockBuilder may not assume unique input functions [tvm]
Lunderberg opened a new pull request, #16805:
URL: https://github.com/apache/tvm/pull/16805
Prior to this commit, the implementation of
`relax::BlockBuilder::AddFunction` implicitly assumed that the input `IRModule` does not contain duplicate copies of the same function. This commit updates the implementation, removing the reliance on this assumption. This commit resolves the error by tracking all `GlobalVar` that map to the same function, rather than an just one.
A well-formed IRModule may contain duplicate function definitions. This is rare, as most functions can be disambiguated by the the function attribute `tvm::attr::kGlobalSymbol`. However, private functions do not have this attribute, and a well-formed IRModule may contain multiple copies of the same function.
The regression test added in this PR calls `BlockBuilder::UpdateFunc` and `BlockBuilder::AddFunc` in a specific order to reproduce this issue. In practice, this failure was sporadic, depending on the order in which a transformation pass visited functions in a module. This was first observed in `VMShapeLower`, with sporadic errors depending on the order of iteration over `mod->functions`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Bugfix][Relax] BlockBuilder may not assume unique input functions [tvm]
Posted by "Lunderberg (via GitHub)" <gi...@apache.org>.
Lunderberg commented on PR #16805:
URL: https://github.com/apache/tvm/pull/16805#issuecomment-2028982383
I don't think there's any indication of this being a bug in the pass itself. I also don't know if it's a rare case, only that I encountered it within one specific pass. I think this is a bug in the general `relax::BlockBuilder` utility, as nothing in the API suggests that use of `AddFunc` and `UpdateFunc` within the same transform is unsupported. If we want to avoid complexity in the `relax::BlockBuilder`, then we should remove the function-level de-duplication from `relax::BlockBuilder` altogether, and handle it in a dedicated transform.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Bugfix][Relax] BlockBuilder may not assume unique input functions [tvm]
Posted by "tqchen (via GitHub)" <gi...@apache.org>.
tqchen commented on PR #16805:
URL: https://github.com/apache/tvm/pull/16805#issuecomment-2029845758
OK, after looking through again i agree, so merging this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Bugfix][Relax] BlockBuilder may not assume unique input functions [tvm]
Posted by "tqchen (via GitHub)" <gi...@apache.org>.
tqchen commented on PR #16805:
URL: https://github.com/apache/tvm/pull/16805#issuecomment-2028793540
In this case, I think we should perhaps fix the pass instead if that is a rare case. The main consideration is the added cost and complexity
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Bugfix][Relax] BlockBuilder may not assume unique input functions [tvm]
Posted by "tqchen (via GitHub)" <gi...@apache.org>.
tqchen merged PR #16805:
URL: https://github.com/apache/tvm/pull/16805
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Bugfix][Relax] BlockBuilder may not assume unique input functions [tvm]
Posted by "Lunderberg (via GitHub)" <gi...@apache.org>.
Lunderberg commented on PR #16805:
URL: https://github.com/apache/tvm/pull/16805#issuecomment-2029767747
Rephrasing, I agree that we should minimize the complexity of implementations. However, I think that must be done by choosing which features to offer within an implementation. Since `relax::BlockBuilder` already provides de-duplication, and allows passes to freely use both `AddFunc` and `UpdateFunc`, the complexity in this implementation is complexity that we've already committed to.
I think we should merge this PR, as it resolves a bug in the current feature set provided by `relax::BlockBuilder`. To reduce the complexity of `relax::BlockBuilder`, a follow-up change can try moving the de-duplication of functions into a separate pass.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org