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 2020/03/27 20:16:34 UTC
[GitHub] [incubator-tvm] dpankratz opened a new pull request #5156: [Bugfix]
Fixed rewrite_simplify to correctly cast integers known at compile time.
dpankratz opened a new pull request #5156: [Bugfix] Fixed rewrite_simplify to correctly cast integers known at compile time.
URL: https://github.com/apache/incubator-tvm/pull/5156
## Bug
Casting values which are known at compile time results in strange behaviour. For example `1 * tir.Cast("bool", 77)` becomes `77` after the rewrite_simplify pass instead of `1`.
However if that value was passed in at runtime then the expect result of `1` would be printed.
## Example
```
print(tvm.arith.Analyzer().rewrite_simplify(1 * tir.Cast('bool', 77))) #prints 77
a =te.var('a')
shape = (1,)
c = te.compute(shape,lambda i: 1 * tir.Cast('bool',a))
s = te.create_schedule([c.op])
f = tvm.build(s,[a,c])
c_tvm = tvm.nd.array(np.zeros(shape,dtype='int32'))
f(77,c_tvm)
assert c_tvm.asnumpy()[0] == 1
```
## Explanation
When a `Cast(dtype, value)` call is used on a compile time value then it is replaced with an `IntImm` with the `dtype` and `value` from the call. However, the `value` is stored as a C++ `int64_t` and thus does not limit the `value` to a sensible range . If the resulting `IntImm` is later used in an expression that results in a type promotion unexpected behaviour such as the example above occurs where the `Cast` effectively does nothing.
## Fix
My fix is to ensure that as the `IntImm` is created it takes the type bounds into account. After the fix the above example would behave as expected.
```
print(tvm.arith.Analyzer().rewrite_simplify(1 * tir.Cast('bool', 77))) #prints 1
```
A review would be much appreciated!
@tqchen @vinx13 @ZihengJiang
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] dpankratz commented on issue #5156: [Bugfix] Fixed
rewrite_simplify to correctly cast integers known at compile time.
Posted by GitBox <gi...@apache.org>.
dpankratz commented on issue #5156: [Bugfix] Fixed rewrite_simplify to correctly cast integers known at compile time.
URL: https://github.com/apache/incubator-tvm/pull/5156#issuecomment-605310597
>
>
> Perhaps it would be better to check the OOB directly in MakeConstScalar, so that we thrown an ValueError directly when the value goes out of bound
I agree, that's likely better since the values are known anyway.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] dpankratz commented on issue #5156: [Bugfix] Fixed
rewrite_simplify to correctly cast integers known at compile time.
Posted by GitBox <gi...@apache.org>.
dpankratz commented on issue #5156: [Bugfix] Fixed rewrite_simplify to correctly cast integers known at compile time.
URL: https://github.com/apache/incubator-tvm/pull/5156#issuecomment-605486011
@tqchen @tmoreau89
It looks like the VTA environment is initializing a constant with an out-of-bounds value that is tripping my checks. Could you recommend someone to take a look?
The line at fault seems to be here:
https://github.com/apache/incubator-tvm/blob/master/vta/python/vta/environment.py#L290
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen edited a comment on issue #5156: [TIR] Add
bound check for make_const
Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on issue #5156: [TIR] Add bound check for make_const
URL: https://github.com/apache/incubator-tvm/pull/5156#issuecomment-608610942
@dpankratz @kazum @Huyuwei can you also followup? Seems the main error was due to the fact that we are using int64_t::max value in some places to indicate infty, and that goes out of bound in i32
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on issue #5156: [Bugfix] Fixed
rewrite_simplify to correctly cast integers known at compile time.
Posted by GitBox <gi...@apache.org>.
tqchen commented on issue #5156: [Bugfix] Fixed rewrite_simplify to correctly cast integers known at compile time.
URL: https://github.com/apache/incubator-tvm/pull/5156#issuecomment-605308237
Thanks @dpankratz please fix the CI error, likely due to the fact that stackvm does not yet support large uint
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on issue #5156: [TIR] Add bound
check for make_const
Posted by GitBox <gi...@apache.org>.
tqchen commented on issue #5156: [TIR] Add bound check for make_const
URL: https://github.com/apache/incubator-tvm/pull/5156#issuecomment-608610942
@dpankratz @kazum @Huyuwei can you also followup? Seems the main error was due to the fact that we are using int64_t::max value in some places to indicate infty, and that goes out of bound in i64
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on issue #5156: [Bugfix] Fixed
rewrite_simplify to correctly cast integers known at compile time.
Posted by GitBox <gi...@apache.org>.
tqchen commented on issue #5156: [Bugfix] Fixed rewrite_simplify to correctly cast integers known at compile time.
URL: https://github.com/apache/incubator-tvm/pull/5156#issuecomment-606250411
@dpankratz the issue should be fixed by https://github.com/apache/incubator-tvm/pull/5181
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on pull request #5156: [TIR] Add bound check for make_const
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #5156:
URL: https://github.com/apache/incubator-tvm/pull/5156#issuecomment-706747764
This PR has been inactive for a bit. I still think we should try to bring the change to the mainline. Thanks @dpankratz for raising it initially.
In the spirit of moving things forward and make most things actionable, I will close this PR and mark as inactive. A new PR is more than welcomed!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] dpankratz commented on pull request #5156: [TIR] Add bound check for make_const
Posted by GitBox <gi...@apache.org>.
dpankratz commented on pull request #5156:
URL: https://github.com/apache/incubator-tvm/pull/5156#issuecomment-619560753
@tqchen Can you recommend someone to look at the Pytorch Frontend please? I was able to track down the creation of constants that are set to 2**63 -1 to [this call](https://github.com/apache/incubator-tvm/blob/1014fefa54b5f0a359501b6d19ea3b5a52d6dca6/tests/python/frontend/pytorch/test_forward.py#L164). However, my attempts to fix it result in whatever static analysis is occurring to fail in certain tests.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen closed pull request #5156: [TIR] Add bound check for make_const
Posted by GitBox <gi...@apache.org>.
tqchen closed pull request #5156:
URL: https://github.com/apache/incubator-tvm/pull/5156
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org