Johnson9009 commented on pull request #8733: URL: https://github.com/apache/tvm/pull/8733#issuecomment-899441624
> Looks like the PR consistently fails on the following two test cases, and it doesn't seem to be precision issues: > > ``` > FAILED tests/python/frontend/onnx/test_forward.py::test_random_uniform[llvm] > FAILED tests/python/frontend/onnx/test_forward.py::test_random_uniform[cuda] > ``` > > Would you please take a closer look at the specific cases and get them fixed so that we could merge this PR? Thanks a lot! I have found these two test cases will be fail with my patch all the time, after some investigation I found it isn't easy to fix, it will involve at least 2 PR #7083, PR #8426. The implementation of these 2 test cases depends on #7083, and the implementation of #7083 involve lots of integer implicit conversion between "uint64" and "int32". In short, we need the authors(@jwfromm) of the test cases to confirm whether the golden is correct, and this maybe need the authors(@tkonolige) of PR #7083 to confirm whether the previous integer implicit conversion logic is what they expected firstly. Let me explain it more detail https://github.com/apache/tvm/blob/3e0c461f26b5174759dbb932986006f73a94a816/python/tvm/topi/random/kernel.py#L264-L268 For the line 266 code, the type of the expression `tir.const(2 ** 64 - 1, dtype=gen.dtype) - out_len` is something like `uint64 - int32`, this subtraction will call function `BinaryOpMatchTypes` to do the integer implicit conversion, then the expression will be changed to something like `int64(uint64) - int64(int32)`, (this logic is consistent before and after the PR #8706, so we can ignore the changes made by PR #8706 for easier analysis). So @tkonolige please help to check whether this conversion logic is what you expected(in my opinion, it is wrong for converting tir.const(2**64 - 1, dtype="uint64) to "int64", this will make your number to a negative number). If it is wrong, then it prove that the implementation of PR #7083 maybe can't work because of the original integer implicit conversion logic, and the golden of these test cases maybe wrong. I found the integer implicit conversion is very important for the implementation of PR #7083, so we should investigate it in more details and discuss whether the C standard integer implicit conversion can meet all our requirements or not. -- 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]
