lazycal commented on pull request #8921: URL: https://github.com/apache/tvm/pull/8921#issuecomment-912787653
@masahi Thanks for fixing this issue! Just one random thought out of this issue that may further improve TVM: is the layout annotation imposing too many unnecessary constraints (i.e., asserting semantics of each dimension)? Will it be better to not annotate layout itself, but instead annotate how the layout got transformed? As in this case, previously TVM assumes `NK` for the second operand, which essentially constraints the 1st dimension to have the *semantic* of "batch" and the 2nd "units". This is why this issue happens: another operation assumes a semantic of `NC` which conflicts with `NK`. However, I feel like what `AlterOpLayout` needs is only how the layouts are transformed before and after the pass (e.g., the 2nd dimension is split by a factor of xxx), and it does not care about the semantics at all. The current fix solves the issue for `nn.dense`, but it seems the fundamental issue still exists. For example, `F.conv2d(x, F.conv2d(y, z))` will be broken too (though I guess this is a weird pattern that may not occur in practice?). -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
