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

Remove comparison of unsigned expression < 0 warning #6319

Merged
merged 2 commits into from
Sep 8, 2020

Conversation

wrongtest-intellif
Copy link
Contributor

@wrongtest-intellif wrongtest-intellif commented Aug 21, 2020

remove "comparison of unsigned expression < 0" warning

@wrongtest-intellif wrongtest-intellif changed the title remove comparison of unsigned expression < 0 warning Remove comparison of unsigned expression < 0 warning Aug 21, 2020
@tqchen
Copy link
Member

tqchen commented Aug 21, 2020

Thanks @wrongtest . This might revealed another problem, as the default data structure for ndim should be int32_t as in https://github.com/dmlc/dlpack/blob/master/include/dlpack/dlpack.h#L136 So perhaps we want to change the API to use the new value.

There is always a temptation to use unsigned integers to represent values that can are non-negative. And there can be an active debate. On one hand, having unsigned values are great because we don't need to worry about negative checks like this one. On the other hand, unsigned values also creates potential problems, when there are a lot of implicit conversion between unsigned and signed, and potential underflow during subtraction.

So gradually my view has changed from use unsigned for non-neg values to use signed when possible. To make things consistent, perhaps we should change the arguments of these APIs to int or int32_t.

cc @liangfu @areusch

@areusch
Copy link
Contributor

areusch commented Aug 21, 2020

i guess the root problem here is we aren't able to use a higher-level language construct to describe the memory layout since we are serializing/deserializing. because of that, we're casting and can't rely on -Werror (my view is that most of these should be caught using -Werror) to really catch an underlying data format discrepancy; though, it should catch this bug. it looks like we aren't using -Werror with apps/bundle_deploy in master right now, we should probably turn that on and clean up all the warnings in a future PR.

@@ -76,7 +76,7 @@ int TVMNDArray_Load(TVMNDArray* ret, const char** strm) {
*strm += sizeof(ndim);
dtype = ((DLDataType*)*strm)[0]; // NOLINT(*)
*strm += sizeof(dtype);
if ((ndim < 0) || (ndim > TVM_CRT_MAX_NDIM)) {
if (ndim > TVM_CRT_MAX_NDIM) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wrongtest I think it would make more sense here to change the type of ndim to int to match the dlpack.h header:
https://github.com/dmlc/dlpack/blob/master/include/dlpack/dlpack.h#L136

then we'd need to keep the original check. would you be up for making this change?

Copy link
Contributor Author

@wrongtest-intellif wrongtest-intellif Sep 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for not have noticed your comment later. I update related types of ndarray.c to int32_t. While original dlpack ndim is just defined as int, line 71 use also int instead of int32_t.

I run a local compile with -Wsign-compare -Wsign-conversion to ensure no inconsistency introduced internally in this file.

@tqchen tqchen merged commit 718a9a7 into apache:master Sep 8, 2020
@tqchen
Copy link
Member

tqchen commented Sep 8, 2020

Thanks @wrongtest @areusch !

kevinthesun pushed a commit to kevinthesun/tvm that referenced this pull request Sep 17, 2020
* fix: remove warning for if (unsigned < 0...)

* change used types related to dlpack ndim to int32_t
kevinthesun pushed a commit to kevinthesun/tvm that referenced this pull request Sep 18, 2020
* fix: remove warning for if (unsigned < 0...)

* change used types related to dlpack ndim to int32_t
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request Sep 18, 2020
* fix: remove warning for if (unsigned < 0...)

* change used types related to dlpack ndim to int32_t
@zhanghaohit zhanghaohit deleted the fix/FixAUnsignCmpWarning branch December 17, 2020 02:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants