Johnson9009 commented on pull request #8706: URL: https://github.com/apache/tvm/pull/8706#issuecomment-898280034
@ganler @YuchenJin @tqchen I happens to file a PR relevant to this change, after reading all of your preceding discussing, I think there maybe have something not clear. From reading the PR, I found, for the test case here `tir.const(1, "int32").__eq__(tir.const(1, "uint1"))`, the old behavior of BinaryOpMatchTypes is `int32 == uint1 -> int32 == int32(uint1)`, it is exactly same with the job that this PR does. Then I feel it is strange why this PR and my own PR can make `assert tir.const(1) == tir.const(True)` work, after debug it step-by-step I found the reason is the function we use "cast" instead the "tir::Cast" used by "SimpleCast". https://github.com/apache/tvm/blob/a06863ac9406c027b29f346fb6177268f612912d/src/tir/op/op.cc#L273-L284 We can see it is the logic around line 279 that eliminate the CastNode, so the function `cast` is do some constant fold optimization. > Hi @ganler, Overall LGTM! Thanks for elaborating on the issue and implementing the fix! > > One question is that whether we want to also make `assert tir.const(1, "uint2") == tir.const(1, "int2")` work? I think since `lhs` and `rhs` have the same bits, `SimpleCast` will be applied to both sides, but the same error throws. @YuchenJin For the scenario that unsigned and signed integer have the same width, I think we should consistent with C standard (e.g., "int32 + uint32" -> "uint32(int32) + uint32") instead of current logic "int32 + uint32" -> "int32 + int32(uint32)", please have a look of my PR #8733, we implement a new AI chip backend of TVM, now we meet this issue, we need make a pass to change the implicit conversion consistent with C standard. -- 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]
