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/09/07 16:01:20 UTC
[GitHub] [incubator-tvm] tom-gall opened a new pull request #6416: add -static-libgcc to target_link_libraries for tvm to fix arm64 nati…
tom-gall opened a new pull request #6416:
URL: https://github.com/apache/incubator-tvm/pull/6416
add -static-libgcc to target_link_libraries for tvm to fix arm64 native build.
The central issue /tvm/build/libtvm.so: undefined symbol: __extendhftf2 manifests itself
when first loading libtvm.so.
This is due to src/relay/transforms/pattern_util.h making use of
reinterpret_cast<__fp16*>(array->data)[i];
It could be argued this is a distro (debian) gcc compiler bug and that avenue
is being looked into. In the meantime, this fixes the issue.
Fixes https://github.com/apache/incubator-tvm/issues/6415
Signed-off-by: Tom Gall <to...@linaro.org>
----------------------------------------------------------------
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 #6416: add -static-libgcc to target_link_libraries for tvm to fix arm64 nati…
Posted by GitBox <gi...@apache.org>.
tqchen closed pull request #6416:
URL: https://github.com/apache/incubator-tvm/pull/6416
----------------------------------------------------------------
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] junrushao1994 commented on pull request #6416: add -static-libgcc to target_link_libraries for tvm to fix arm64 nati…
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #6416:
URL: https://github.com/apache/incubator-tvm/pull/6416#issuecomment-688593324
Generally speaking, it is not a good idea to add this flag by default, because we do want to depend on shared libgcc. Usually there are other way to work around this issue.
----------------------------------------------------------------
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] u99127 edited a comment on pull request #6416: add -static-libgcc to target_link_libraries for tvm to fix arm64 nati…
Posted by GitBox <gi...@apache.org>.
u99127 edited a comment on pull request #6416:
URL: https://github.com/apache/incubator-tvm/pull/6416#issuecomment-689179968
>
>
> @tom-gall @cbalint13 would be great if we can try the compiler-rt route :)
>
>
> > Is it possible to use the compiler RT function instead?
> > https://github.com/apache/incubator-tvm/blob/master/3rdparty/compiler-rt/builtin_fp16.h#L165
> > It would be useful to use this function so that the code itself is platform agnostic.
>
> Or even better, can use internal instead of `reinterpret_cast<__fp16*>` ?
>
> * Functions: https://github.com/apache/incubator-tvm/blob/master/src/runtime/builtin_fp16.cc
>
> * Example: https://github.com/apache/incubator-tvm/blob/master/src/relay/transforms/pattern_util.h#L252
>
>
> @tom-gall ,
> Cc @junrushao1994 , @tqchen
>
> If want i can make a separate PR with the proposed `reinterpret_cast<__fp16*>` replacement.
> However `reinterpret_cast<__fp16*>` is more elegant indeed (if toolchains would agree with us too).
I've spent some time analysing this bug today. The language feature works with static linking but not without it - so that smells of an issue in the toolchain in my book since I wouldn't expect something like reinterpret_cast to depend on linkage methods. I think I have a reproducer to discuss further with the relevant GCC folks in the morning.
However, I've gone back and re-read the man page for --static-libgcc as this isn't something that I've used before and that has the following note which raised my eyebrows and probably needs some more querying.
https://gcc.gnu.org/onlinedocs/gcc-10.2.0/gcc/Link-Options.html#Link-Options
"However, if a library or main executable is supposed to throw or catch exceptions, you must link it using the G++ driver, or using the option -shared-libgcc, such that it is linked with the shared libgcc."
I'm also not convinced we can assert this is safe for all the API consumers of libtvm.so that exceptions wouldn't be received or thrown across the libtvm.so boundary ? On an offline conversation @junrushao1994 indicated that we may throw an exception across if someone were using a C++ API from libtvm.so in a C++ wrapper. I'm not sure about other usecases. Even if we did, I think it's risky to continue down the path of using --static-libgcc on one platform because you've got different behaviours on different hosts and we should look to avoid that for longer term maintenance.
Thus I think we should look for a workaround till this is discussed with the GCC community more. I've not yet studied the compiler-rt one to say more -
More later this week.
regards
Ramana
----------------------------------------------------------------
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 commented on pull request #6416: add -static-libgcc to target_link_libraries for tvm to fix arm64 nati…
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #6416:
URL: https://github.com/apache/incubator-tvm/pull/6416#issuecomment-689887374
https://github.com/apache/incubator-tvm/pull/6431
----------------------------------------------------------------
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] junrushao1994 commented on pull request #6416: add -static-libgcc to target_link_libraries for tvm to fix arm64 nati…
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #6416:
URL: https://github.com/apache/incubator-tvm/pull/6416#issuecomment-688594393
It is a continuation of our discussion in this [thread](https://discuss.tvm.apache.org/t/oserror-tvm-build-libtvm-so-undefined-symbol-extendhftf2/7624/7?u=junrushao1994). Though I suggested using libgcc as a workaround, I believe there are other (more idiomatic and platform agnostic) ways. Also would love to hear from @u99127 if there is updates on this issue :-)
----------------------------------------------------------------
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 commented on pull request #6416: add -static-libgcc to target_link_libraries for tvm to fix arm64 nati…
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #6416:
URL: https://github.com/apache/incubator-tvm/pull/6416#issuecomment-688974031
@tom-gall @cbalint13 would be great if we can try the compiler-rt route :)
----------------------------------------------------------------
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] u99127 edited a comment on pull request #6416: add -static-libgcc to target_link_libraries for tvm to fix arm64 nati…
Posted by GitBox <gi...@apache.org>.
u99127 edited a comment on pull request #6416:
URL: https://github.com/apache/incubator-tvm/pull/6416#issuecomment-689179968
>
>
> @tom-gall @cbalint13 would be great if we can try the compiler-rt route :)
>
>
> > Is it possible to use the compiler RT function instead?
> > https://github.com/apache/incubator-tvm/blob/master/3rdparty/compiler-rt/builtin_fp16.h#L165
> > It would be useful to use this function so that the code itself is platform agnostic.
>
> Or even better, can use internal instead of `reinterpret_cast<__fp16*>` ?
>
> * Functions: https://github.com/apache/incubator-tvm/blob/master/src/runtime/builtin_fp16.cc
>
> * Example: https://github.com/apache/incubator-tvm/blob/master/src/relay/transforms/pattern_util.h#L252
>
>
> @tom-gall ,
> Cc @junrushao1994 , @tqchen
>
> If want i can make a separate PR with the proposed `reinterpret_cast<__fp16*>` replacement.
> However `reinterpret_cast<__fp16*>` is more elegant indeed (if toolchains would agree with us too).
I've spent some time analysing this bug today. The language feature works with static linking but not without it - so that smells of an issue in the toolchain in my book since I wouldn't expect something like reinterpret_cast to depend on linkage methods. I think I have a reproducer to discuss further with the relevant GCC folks in the morning.
However, I've gone back and re-read the man page for --static-libgcc as this isn't something that I've used before and that has the following note which raised my eyebrows and probably needs some more querying.
https://gcc.gnu.org/onlinedocs/gcc-10.2.0/gcc/Link-Options.html#Link-Options
"However, if a library or main executable is supposed to throw or catch exceptions, you must link it using the G++ driver, or using the option -shared-libgcc, such that it is linked with the shared libgcc."
I'm also not convinced we can assert this is safe for all the API consumers of libtvm.so that exceptions wouldn't be received or thrown across the libtvm.so boundary ? Even if we did, I think it's risky to continue down the path of using --static-libgcc on one platform because you've got different behaviours on different hosts and we should look to avoid that for longer term maintenance.
Thus I think we should look for a workaround till this is discussed with the GCC community more. I've not yet studied the compiler-rt one to say more -
More later this week.
regards
Ramana
----------------------------------------------------------------
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 commented on pull request #6416: add -static-libgcc to target_link_libraries for tvm to fix arm64 nati…
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #6416:
URL: https://github.com/apache/incubator-tvm/pull/6416#issuecomment-688527472
Is it possible to use the compiler RT function instead?
https://github.com/apache/incubator-tvm/blob/master/3rdparty/compiler-rt/builtin_fp16.h#L165
----------------------------------------------------------------
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] cbalint13 edited a comment on pull request #6416: add -static-libgcc to target_link_libraries for tvm to fix arm64 nati…
Posted by GitBox <gi...@apache.org>.
cbalint13 edited a comment on pull request #6416:
URL: https://github.com/apache/incubator-tvm/pull/6416#issuecomment-688651372
> Is it possible to use the compiler RT function instead?
>
> https://github.com/apache/incubator-tvm/blob/master/3rdparty/compiler-rt/builtin_fp16.h#L165
>
> It would be useful to use this function so that the code itself is platform agnostic.
Or even better, can use internal instead of ```reinterpret_cast<__fp16*>``` ?
* Functions: https://github.com/apache/incubator-tvm/blob/master/src/runtime/builtin_fp16.cc
* Example: https://github.com/apache/incubator-tvm/blob/master/src/relay/transforms/pattern_util.h#L252
@tom-gall ,
Cc @junrushao1994 , @tqchen
If want i can make a separate PR with the proposed ```reinterpret_cast<__fp16*>``` replacement.
However ```reinterpret_cast<__fp16*>``` is more elegant indeed (if toolchains would agree with us too).
----------------------------------------------------------------
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] u99127 commented on pull request #6416: add -static-libgcc to target_link_libraries for tvm to fix arm64 nati…
Posted by GitBox <gi...@apache.org>.
u99127 commented on pull request #6416:
URL: https://github.com/apache/incubator-tvm/pull/6416#issuecomment-688738147
>
>
> It is a continuation of our discussion in this [thread](https://discuss.tvm.apache.org/t/oserror-tvm-build-libtvm-so-undefined-symbol-extendhftf2/7624/7?u=junrushao1994). Though I suggested using libgcc as a workaround, I believe there are other (more idiomatic and platform agnostic) ways. Also would love to hear from @u99127 if there is updates on this issue :-)
I'll put this in my list of things to look at during this week - I've been on holiday recently so I've not managed to get much done. It is a workaround we may have to live with because of a bug but usually I wouldn't expect to need to put a -static-libgcc on the command line as that's not the usual behaviour I would expect with the GNU toolchain.
----------------------------------------------------------------
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] cbalint13 commented on a change in pull request #6416: add -static-libgcc to target_link_libraries for tvm to fix arm64 nati…
Posted by GitBox <gi...@apache.org>.
cbalint13 commented on a change in pull request #6416:
URL: https://github.com/apache/incubator-tvm/pull/6416#discussion_r484563687
##########
File path: CMakeLists.txt
##########
@@ -393,7 +393,7 @@ if(USE_THREADS AND NOT BUILD_FOR_HEXAGON)
target_link_libraries(tvm_runtime Threads::Threads)
endif()
-target_link_libraries(tvm ${TVM_LINKER_LIBS} ${TVM_RUNTIME_LINKER_LIBS})
+target_link_libraries(tvm -static-libgcc ${TVM_LINKER_LIBS} ${TVM_RUNTIME_LINKER_LIBS})
Review comment:
Can append() the linker flag to TVM_LINKER_LIBS variable using condition when cmake's CMAKE_SYSTEM_PROCESSOR is arm64 ?
----------------------------------------------------------------
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] u99127 commented on pull request #6416: add -static-libgcc to target_link_libraries for tvm to fix arm64 nati…
Posted by GitBox <gi...@apache.org>.
u99127 commented on pull request #6416:
URL: https://github.com/apache/incubator-tvm/pull/6416#issuecomment-689179968
>
>
> @tom-gall @cbalint13 would be great if we can try the compiler-rt route :)
>
>
> > Is it possible to use the compiler RT function instead?
> > https://github.com/apache/incubator-tvm/blob/master/3rdparty/compiler-rt/builtin_fp16.h#L165
> > It would be useful to use this function so that the code itself is platform agnostic.
>
> Or even better, can use internal instead of `reinterpret_cast<__fp16*>` ?
>
> * Functions: https://github.com/apache/incubator-tvm/blob/master/src/runtime/builtin_fp16.cc
>
> * Example: https://github.com/apache/incubator-tvm/blob/master/src/relay/transforms/pattern_util.h#L252
>
>
> @tom-gall ,
> Cc @junrushao1994 , @tqchen
>
> If want i can make a separate PR with the proposed `reinterpret_cast<__fp16*>` replacement.
> However `reinterpret_cast<__fp16*>` is more elegant indeed (if toolchains would agree with us too).
I've spent some time analysing this bug today. The language feature works with static linking but not without it - so that smells of an issue in the toolchain in my book since I wouldn't expect something like reinterpret_cast to depend on linkage methods. I think I have a reproducer to discuss further with the relevant GCC folks in the morning.
However, I've gone back and re-read the man page for --static-libgcc as this isn't something that I've used before and that has the following note which raised my eyebrows and probably needs some more querying.
https://gcc.gnu.org/onlinedocs/gcc-10.2.0/gcc/Link-Options.html#Link-Options
"However, if a library or main executable is supposed to throw or catch exceptions, you must link it using the G++ driver, or using the option -shared-libgcc, such that it is linked with the shared libgcc."
I'm also not convinced we can assert this is safe for all the API consumers of libtvm.so that exceptions wouldn't be received or thrown across the libtvm.so boundary ? Even if we did, I think it's risky to continue down the path of using --static-libgcc on one platform because you've got different behaviours on different hosts and we should look to avoid that for longer term maintenance.
Thus I think we should look for a workaround till this is discussed with the GCC community more. I'm not sure if the compiler-rt one is the right one.
regards
Ramana
----------------------------------------------------------------
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 edited a comment on pull request #6416: add -static-libgcc to target_link_libraries for tvm to fix arm64 nati…
Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on pull request #6416:
URL: https://github.com/apache/incubator-tvm/pull/6416#issuecomment-688527472
Is it possible to use the compiler RT function instead?
https://github.com/apache/incubator-tvm/blob/master/3rdparty/compiler-rt/builtin_fp16.h#L165
It would be useful to use this function so that the code itself is platform agnostic.
----------------------------------------------------------------
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] cbalint13 commented on pull request #6416: add -static-libgcc to target_link_libraries for tvm to fix arm64 nati…
Posted by GitBox <gi...@apache.org>.
cbalint13 commented on pull request #6416:
URL: https://github.com/apache/incubator-tvm/pull/6416#issuecomment-688651372
> Is it possible to use the compiler RT function instead?
>
> https://github.com/apache/incubator-tvm/blob/master/3rdparty/compiler-rt/builtin_fp16.h#L165
>
> It would be useful to use this function so that the code itself is platform agnostic.
Or even better, can use internal instead of ```reinterpret_cast<__fp16*>``` ?
* Function: https://github.com/apache/incubator-tvm/blob/master/src/runtime/builtin_fp16.cc
* Example: https://github.com/apache/incubator-tvm/blob/master/src/relay/transforms/pattern_util.h#L252
@tom-gall ,
Cc @junrushao1994 , @tqchen
If want i can make a separate PR with the proposed ```reinterpret_cast<__fp16*>``` replacement.
However ```reinterpret_cast<__fp16*>``` is more elegant indeed (if toolchains would agree with us too).
----------------------------------------------------------------
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] cbalint13 commented on a change in pull request #6416: add -static-libgcc to target_link_libraries for tvm to fix arm64 nati…
Posted by GitBox <gi...@apache.org>.
cbalint13 commented on a change in pull request #6416:
URL: https://github.com/apache/incubator-tvm/pull/6416#discussion_r484563687
##########
File path: CMakeLists.txt
##########
@@ -393,7 +393,7 @@ if(USE_THREADS AND NOT BUILD_FOR_HEXAGON)
target_link_libraries(tvm_runtime Threads::Threads)
endif()
-target_link_libraries(tvm ${TVM_LINKER_LIBS} ${TVM_RUNTIME_LINKER_LIBS})
+target_link_libraries(tvm -static-libgcc ${TVM_LINKER_LIBS} ${TVM_RUNTIME_LINKER_LIBS})
Review comment:
Can append() the linker flag to TVM_LINKER_LIBS variable using condition on cmake's CMAKE_SYSTEM_PROCESSOR is arm64 ?
----------------------------------------------------------------
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