[Bugfix][Relax] BlockBuilder may not assume unique input functions #16805
Conversation
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`.
|
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 |
|
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 |
|
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 I think we should merge this PR, as it resolves a bug in the current feature set provided by |
|
OK, after looking through again i agree, so merging this |
…pache#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`.
Prior to this commit, the implementation of
relax::BlockBuilder::AddFunctionimplicitly assumed that the inputIRModuledoes 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 allGlobalVarthat 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::UpdateFuncandBlockBuilder::AddFuncin 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 inVMShapeLower, with sporadic errors depending on the order of iteration overmod->functions.