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

Fix compile warnings. #6204

Merged
merged 1 commit into from
Aug 5, 2020
Merged

Fix compile warnings. #6204

merged 1 commit into from
Aug 5, 2020

Conversation

cbalint13
Copy link
Contributor

@cbalint13 cbalint13 commented Aug 4, 2020

Issue:

  • GCC compiler warns about "return statement prevents copy elision" like below.
  • Since this is a common header file many warnings like one below will be fired.
/home/cbalint/work/TVM/tvm.marlann/include/tvm/ir/attrs.h:694:29:   required from ‘void tvm::AttrsNode<DerivedType>::InitByPackedArgs(const tvm::runtime::TVMArgs&, bool) [with DerivedType = tvm::relay::Conv2DAttrs]’
/home/cbalint/work/TVM/tvm.marlann/include/tvm/ir/attrs.h:658:8:   required from here
/home/cbalint/work/TVM/tvm.marlann/include/tvm/ir/attrs.h:478:25: warning: moving a local object in a return statement prevents copy elision [-Wpessimizing-move]
/home/cbalint/work/TVM/tvm.marlann/include/tvm/ir/attrs.h:478:25: note: remove ‘std::move’ call

Details:

  • GCC version used:
$ gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/libexec/gcc/x86_64-redhat-linux/10/lto-wrapper
OFFLOAD_TARGET_NAMES=nvptx-none
OFFLOAD_TARGET_DEFAULT=1
Target: x86_64-redhat-linux
Configured with: ../configure --enable-bootstrap --enable-languages=c,c++,fortran,objc,obj-c++,ada,go,d,lto --prefix=/usr --mandir=/usr/share/man --infodir=/usr/share/info --with-bugurl=http://bugzilla.redhat.com/bugzilla --enable-shared --enable-threads=posix --enable-checking=release --enable-multilib --with-system-zlib --enable-__cxa_atexit --disable-libunwind-exceptions --enable-gnu-unique-object --enable-linker-build-id --with-gcc-major-version-only --with-linker-hash-style=gnu --enable-plugin --enable-initfini-array --with-isl --enable-offload-targets=nvptx-none --without-cuda-driver --enable-gnu-indirect-function --enable-cet --with-tune=generic --with-arch_32=i686 --build=x86_64-redhat-linux
Thread model: posix
Supported LTO compression algorithms: zlib zstd
gcc version 10.2.1 20200723 (Red Hat 10.2.1-1) (GCC) 

Origins:
Mentioned line was introduced here by @hanzz2007 on PR #6128 .

@hanzz2007 , @tqchen please help with the review.

Thank You !

@MarisaKirisame
Copy link
Contributor

warning: moving a local object in a return statement prevents copy elision [-Wpessimizing-move]
/home/cbalint/work/TVM/tvm.marlann/include/tvm/ir/attrs.h:478:25: note: remove ‘std::move’ call

https://stackoverflow.com/questions/19267408/why-does-stdmove-prevent-rvo
I think the right solution is to just remove the std::move.

@hanzz2007
Copy link
Contributor

warning: moving a local object in a return statement prevents copy elision [-Wpessimizing-move]
/home/cbalint/work/TVM/tvm.marlann/include/tvm/ir/attrs.h:478:25: note: remove ‘std::move’ call

https://stackoverflow.com/questions/19267408/why-does-stdmove-prevent-rvo
I think the right solution is to just remove the std::move.

remove std::move may help in common modern compilers, but the standard said this kind of optimization is allowed, not must, i am afraid it is not a good way to rely on certain implementations not the standard

When certain criteria are met, an implementation is allowed to omit the copy/move construction of a class object, even if the constructor selected for the copy/move operation and/or the destructor for the object have side effects. In such cases, the implementation treats the source and target of the omitted copy/move operation as simply two different ways of referring to the same object, and the destruction of that object occurs at the later of the times when the two objects would have been destroyed without the optimization. This elision of copy/move operations, called copy elision, is permitted in the following circumstances (which may be combined to eliminate multiple copies):

in a return statement in a function with a class return type, when the expression is the name of a non-volatile automatic object (other than a function or catch-clause parameter) with the same cv-unqualified type as the function return type, the copy/move operation can be omitted by constructing the automatic object directly into the function’s return value
[...]
when a temporary class object that has not been bound to a reference (12.2) would be copied/moved to a class object with the same cv-unqualified type, the copy/move operation can be omitted by constructing the temporary object directly into the target of the omitted copy/move
[...]

@hanzz2007
Copy link
Contributor

i suppose there are two ways to solve:

  1. add some pragma to inhibit this warning
  2. neither rely on move nor rvo, just refactor the AttrInitEntry , add ref counted internal state to delay destructor, as codes showing bellow
    https://gist.github.com/hanzz2007/803b28a7915c31686cffce0ac6450b73
    https://gist.github.com/hanzz2007/e42b14b51c8c9c765cca107867e64d49

@cbalint13
Copy link
Contributor Author

cbalint13 commented Aug 4, 2020

i suppose there are two ways to solve:

  1. add some pragma to inhibit this warning
  2. neither rely on move nor rvo, just refactor the AttrInitEntry , add ref counted internal state to delay destructor, as codes showing bellow
    https://gist.github.com/hanzz2007/803b28a7915c31686cffce0ac6450b73
    https://gist.github.com/hanzz2007/e42b14b51c8c9c765cca107867e64d49

@hanzz2007 , @MarisaKirisame

  • Updated PR, added a GCC specific single-line effect pragma for now.
  • I myself like 2nd solution (clean & readable & trackable) but @hanzz2007 please make it in a separate PR.

Thank You !

PS: 😅

  • Ended up with 2x pragma: one silence "too dumb" (old CI GCC), second waves the "too smart" (latest GCC)
  • Beside CI, can confirm that PR is fine now with GCC 10.2.1 too, "the pessimistic" have zero warnings now.

@hanzz2007
Copy link
Contributor

Any advice? @tqchen

@tqchen tqchen merged commit edc5d8f into apache:master Aug 5, 2020
@tqchen
Copy link
Member

tqchen commented Aug 5, 2020

let us go with the warning disable for now. Given that the move can be more efficient(as efficient as RVO) and we do not want to introduce additional complexity here

@tqchen
Copy link
Member

tqchen commented Aug 5, 2020

Thanks @cbalint13 @MarisaKirisame @hanzz2007

wjliu1998 pushed a commit to wjliu1998/incubator-tvm that referenced this pull request Aug 13, 2020
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Aug 26, 2020
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Aug 26, 2020
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Aug 26, 2020
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Sep 2, 2020
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request Sep 3, 2020
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.

4 participants