masahi commented on pull request #9438: URL: https://github.com/apache/tvm/pull/9438#issuecomment-989438211
I mean, just looking at the two implementations, it's obvious that you started by copy-pasting Loop converter (even comments are identical). Can we refactor common code into a helper function, or is it difficult to cleanly extract common bits? This is not just for usual code-hygiene reason. I believe there are some issues in Loop converter in preserving static shape information. I've never seen a practical ONNX model with `Loop` op that can be successfully compiled by TVM - I've seen / heard that ONNX models converted from TF ssd mobilenet v1, MaskRCNN, and mlperf DLRM has `Loop` op, all of them failed to compile with TVM. So I don't want to see the Loop converter implementation in question duplicated for other ops. See for example this issue regarding ssd mobilenet v1 https://github.com/apache/tvm/issues/8284#issuecomment-864467897 Note that the original tf ssd mobilenet v1 model compiles and runs fine when imported directly by TF frontend, so I can only assume that this is a ONNX converter issue. -- 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]
