Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Check about negative uint constant #10484

Merged
merged 11 commits into from
Mar 8, 2022
Merged

Add Check about negative uint constant #10484

merged 11 commits into from
Mar 8, 2022

Conversation

haoyang9804
Copy link
Contributor

@haoyang9804 haoyang9804 commented Mar 4, 2022

Focusing the problem in here.
As the author said, when we negative after ones_like a uint variable, tvm crashes as expected. But negative a uint const would run smoothly. So I tried the following script, only changing relay.var to relay.const, which should crash but not.

import tvm
from tvm import relay
var_7 = relay.const(1, dtype="uint64")
var_10 = relay.ones_like(var_7)
y = relay.negative(var_10)
F = relay.Function([], y)

mod = tvm.IRModule()
mod['main'] = F
print(mod)
graph, lib, params = relay.build(mod, target='llvm')  # crash

I have make the following change to the codebase:
In TryConstFold<tir::Sub>, which is needed for negative operator, I add a check about potential negative uint value: If minuend is of uint and it's value is 0 while the subtrahend is of uint, then the difference would be negative and of uint. This situation should be forbidden.

@haoyang9804
Copy link
Contributor Author

haoyang9804 commented Mar 4, 2022

Could any reviewer help take a look about why my commit fails in sanity check? I do not have adequate experience in commiting fixes. Thanks in advance. cc @junrushao1994 @jroesch

@leeexyz
Copy link
Contributor

leeexyz commented Mar 4, 2022

@haoyang9804 You can find the Sanity Check Log here.

change

[2022-03-04T06:18:11.419Z] -    ICHECK(!(pa && pa->dtype.is_uint() && pa->value == 0U && b.dtype().is_uint())) << \
[2022-03-04T06:18:11.419Z] -     "Checked failed. Minuend 's value is 0U and it's dtype is uint " << \
[2022-03-04T06:18:11.419Z] -     "while Subtrahend's dtype is uint; which will cause a negative unit";

to

[2022-03-04T06:18:11.419Z] +    ICHECK(!(pa && pa->dtype.is_uint() && pa->value == 0U && b.dtype().is_uint()))
[2022-03-04T06:18:11.419Z] +        << "Checked failed. Minuend 's value is 0U and it's dtype is uint "
[2022-03-04T06:18:11.419Z] +        << "while Subtrahend's dtype is uint; which will cause a negative uint";

You'd better run the check locally before sending a PR.

bash ./tests/scripts/task_lint.sh

@haoyang9804
Copy link
Contributor Author

@haoyang9804 haoyang9804 closed this Mar 4, 2022
@haoyang9804 haoyang9804 reopened this Mar 4, 2022
@haoyang9804
Copy link
Contributor Author

@leeexyz Thanks for you kind reply. Though I cannot run this lint script locally due to inaccessibility of some files. But finally it works after I change the format of the code.
BTW, could you please help check my commit? I am not sure whether or not my fix violates some principles of TVM

@haoyang9804 haoyang9804 changed the title Add Check about negative uint Add Check about negative uint constant Mar 4, 2022
@haoyang9804
Copy link
Contributor Author

Hi @masahi. Would you mind to take a look at this pr. Maybe it's a small problem, but I think it shows an inconsistency between uint const and uint var.

@masahi masahi merged commit f4b74ba into apache:main Mar 8, 2022
Lunderberg added a commit to Lunderberg/tvm that referenced this pull request Mar 8, 2022
A check for unsigned integer overflow would throw an error if it
encountered 0U - 0U.

apache#10484, which introduced the check,
and apache#9727, which introduced this
edge case, were in CI at the same time, and each was tested against a
merge candidate that did not include the other.  The unittest failure
only occurred when both PRs were merged.
Lunderberg added a commit to Lunderberg/tvm that referenced this pull request Mar 8, 2022
A check for unsigned integer overflow would throw an error if it
encountered 0U - 0U.

apache#10484, which introduced the check,
and apache#9727, which introduced this
edge case, were in CI at the same time, and each was tested against a
merge candidate that did not include the other.  The unittest failure
only occurred when both PRs were merged.
masahi pushed a commit that referenced this pull request Mar 9, 2022
A check for unsigned integer overflow would throw an error if it
encountered 0U - 0U.

#10484, which introduced the check,
and #9727, which introduced this
edge case, were in CI at the same time, and each was tested against a
merge candidate that did not include the other.  The unittest failure
only occurred when both PRs were merged.
ziqiangxu8457 pushed a commit to ziqiangxu8457/tvm that referenced this pull request Mar 9, 2022
* fix InferType bug

* fix InferType related bug

* check if uint variable is negative

* check if uint variable is negative

* check if uint variable is negative

* check if uint variable is negative

* check if uint variable is negative

* check if uint variable is negative

* check if uint variable is negative

* check if uint variable is negative

* check if uint variable is negative
ziqiangxu8457 pushed a commit to ziqiangxu8457/tvm that referenced this pull request Mar 9, 2022
A check for unsigned integer overflow would throw an error if it
encountered 0U - 0U.

apache#10484, which introduced the check,
and apache#9727, which introduced this
edge case, were in CI at the same time, and each was tested against a
merge candidate that did not include the other.  The unittest failure
only occurred when both PRs were merged.
pfk-beta pushed a commit to pfk-beta/tvm that referenced this pull request Apr 11, 2022
* fix InferType bug

* fix InferType related bug

* check if uint variable is negative

* check if uint variable is negative

* check if uint variable is negative

* check if uint variable is negative

* check if uint variable is negative

* check if uint variable is negative

* check if uint variable is negative

* check if uint variable is negative

* check if uint variable is negative
pfk-beta pushed a commit to pfk-beta/tvm that referenced this pull request Apr 11, 2022
A check for unsigned integer overflow would throw an error if it
encountered 0U - 0U.

apache#10484, which introduced the check,
and apache#9727, which introduced this
edge case, were in CI at the same time, and each was tested against a
merge candidate that did not include the other.  The unittest failure
only occurred when both PRs were merged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants