You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2022/02/21 17:59:36 UTC
[GitHub] [tvm] lazycal opened a new pull request #10336: [BUGFIX][ARITH] Fix FloorMod Simplifier
lazycal opened a new pull request #10336:
URL: https://github.com/apache/tvm/pull/10336
`relay.layout_transform(x, 'NCHW4c', 'NCHW2c')` this op is not simplified correctly in TIR by canonical simplifier. Specifically, there is a term `i%20/2*2` being simplified to `i%4` incorrectly. This is caused by an incomplete fix in #5505, which incorrectly swapped the order of `/2` and `*2`. A reproducible example:
```python
import tvm
from tvm import relay
import numpy as np
from tvm import relay
def main():
import random
random.seed(0)
np.random.seed(0)
O1 = 20
xsh = (2, O1 // 4, 2, 2, 4)
x = relay.var("x", shape=xsh)
y = relay.layout_transform(x, 'NCHW4c', 'NCHW2c')
func = relay.Function([x], y)
mod = tvm.IRModule.from_expr(func)
inp = np.zeros(xsh).astype(np.float32)
with tvm.transform.PassContext(opt_level=0):
res = relay.build_module.create_executor(
'graph', mod, target='llvm', device=tvm.cpu()).evaluate()(inp)
with tvm.transform.PassContext(opt_level=0, disabled_pass=['tir.Simplify']): #this pass invokes canonical simplifier
res1 = relay.build_module.create_executor(
'debug', mod, target='llvm', device=tvm.cpu()).evaluate()(inp)
print('opt=\n', res.numpy())
print('debug=\n', res1.numpy())
np.testing.assert_allclose(res.numpy(), res1.numpy())
for i in range(10):
main()
print(f'{i}-th run passed')
```
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] spectrometerHBH commented on pull request #10336: [BUGFIX][ARITH] Fix FloorMod Simplifier
Posted by GitBox <gi...@apache.org>.
spectrometerHBH commented on pull request #10336:
URL: https://github.com/apache/tvm/pull/10336#issuecomment-1047143198
Thanks for the catch!
I think the comment in Line 987 is wrong
```
// (x / c1) % c2 => (x % (c1 * c2)) / c2
```
should be
```
// (x / c1) % c2 => (x % (c1 * c2)) / c1
```
And for this SplitModConst, the rationale should be
```
(index % upper) / lower * scale % cval, given cval = scaled_cval * scale
= (index % upper) / lower % scaled_cval * scale
= (index % upper) % (new_upper_factor) / lower * scale
```
Would you mind updating the comments to make it clearer?
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] spectrometerHBH edited a comment on pull request #10336: [BUGFIX][ARITH] Fix FloorMod Simplifier
Posted by GitBox <gi...@apache.org>.
spectrometerHBH edited a comment on pull request #10336:
URL: https://github.com/apache/tvm/pull/10336#issuecomment-1047143198
Thanks for the catch!
I think the comment in Line 987 is wrong (A counterexample is to let c2 = 1)
```
// (x / c1) % c2 => (x % (c1 * c2)) / c2
```
should be
```
// (x / c1) % c2 => (x % (c1 * c2)) / c1
```
And for this SplitModConst, the rationale should be
```
(index % upper) / lower * scale % cval, given cval = scaled_cval * scale
= (index % upper) / lower % scaled_cval * scale
= (index % upper) % (new_upper_factor) / lower * scale
```
Would you mind helping fix the comments in this PR to make it clearer?
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on pull request #10336: [BUGFIX][ARITH] Fix FloorMod Simplifier
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #10336:
URL: https://github.com/apache/tvm/pull/10336#issuecomment-1047121351
CC: @spectrometerHBH @Hzfengsy
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] spectrometerHBH edited a comment on pull request #10336: [BUGFIX][ARITH] Fix FloorMod Simplifier
Posted by GitBox <gi...@apache.org>.
spectrometerHBH edited a comment on pull request #10336:
URL: https://github.com/apache/tvm/pull/10336#issuecomment-1047143198
Thanks for the catch!
I think the comment in Line 987 is wrong
```
// (x / c1) % c2 => (x % (c1 * c2)) / c2
```
should be
```
// (x / c1) % c2 => (x % (c1 * c2)) / c1
```
And for this SplitModConst, the rationale should be
```
(index % upper) / lower * scale % cval, given cval = scaled_cval * scale
= (index % upper) / lower % scaled_cval * scale
= (index % upper) % (new_upper_factor) / lower * scale
```
Would you mind helping fix the comments in this PR to make it clearer?
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] spectrometerHBH edited a comment on pull request #10336: [BUGFIX][ARITH] Fix FloorMod Simplifier
Posted by GitBox <gi...@apache.org>.
spectrometerHBH edited a comment on pull request #10336:
URL: https://github.com/apache/tvm/pull/10336#issuecomment-1047143198
Thanks for the catch!
I think the comment in Line 987 is wrong (A counterexample is to let c2 = 1)
```
// (x / c1) % c2 => (x % (c1 * c2)) / c2
```
It should be
```
// (x / c1) % c2 => (x % (c1 * c2)) / c1
```
And for this SplitModConst, the rationale should be
```
(index % upper) / lower * scale % cval, given cval = scaled_cval * scale
= (index % upper) / lower % scaled_cval * scale
= (index % upper) % (new_upper_factor) / lower * scale
```
Would you mind helping fix the comments in this PR to make it clearer?
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi merged pull request #10336: [BUGFIX][ARITH] Fix FloorMod Simplifier
Posted by GitBox <gi...@apache.org>.
masahi merged pull request #10336:
URL: https://github.com/apache/tvm/pull/10336
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] lazycal commented on pull request #10336: [BUGFIX][ARITH] Fix FloorMod Simplifier
Posted by GitBox <gi...@apache.org>.
lazycal commented on pull request #10336:
URL: https://github.com/apache/tvm/pull/10336#issuecomment-1047171793
@spectrometerHBH Comments have been updated.
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org