Generate GAS-compatible 32-bit ARM ASM files for libvpx on Linux. #53
Conversation
cmake/libvpx.cmake
Outdated
| foreach(in_file ${libvpx_arm32_asm_in}) | ||
| set(out_file ${in_file}.S) | ||
|
|
||
| set(in_file_path ${libvpx_arm32_asm_dir_src}/${in_file}) | ||
| set(out_file_path ${libvpx_arm32_asm_dir_obj}/${out_file}) | ||
|
|
||
| add_custom_command( | ||
| OUTPUT ${out_file_path} | ||
| COMMAND sh -c \"cat \'${in_file_path}\' | ${PERL_EXECUTABLE} \'${ads2gas}\' > \'${out_file_path}\'\" | ||
| DEPENDS ${in_file_path} | ||
| ) | ||
|
|
||
| nice_target_sources(libvpx ${libvpx_arm32_asm_dir_obj} | ||
| PRIVATE | ||
| ${out_file} | ||
| ) | ||
| endforeach() |
There was a problem hiding this comment.
Probably the right way is to create a target_ads2gas_sources, just like target_yasm_source and give it .asm files
There was a problem hiding this comment.
Thanks for the suggestion, I'll try to re-factor it on the following weekend if I'll have time
There was a problem hiding this comment.
Finally found some free time, and agreed, it looks much better now. (my experience with cmake is rather limited)
I put it into the libvpx.cmake file, since the ads2gas scripts are specific to libvpx, but if you feel like it should be in a separate file, I guess we could move it. (it would probably need to be made more generic then, with parameters for a script file and such)
There was a problem hiding this comment.
I think it's fine. Gladly, we have #55 also now. Thank you very much.
There was a problem hiding this comment.
You're welcome, I'm sorry it took so long.
dusxmt
force-pushed
the
fix-arm-libvpx-asm-files
branch
from
f730fe5 to
9825220
Compare
cmake/libvpx.cmake
Outdated
| if (ads2gas) | ||
| find_package(Perl) | ||
| if (NOT PERL_FOUND) | ||
| message(FATAL_ERROR "Unable to find perl, needed by ${ads2gas} on 32-bit ARM systems when NEON SIMD is enabled.") |
There was a problem hiding this comment.
why not just find_package(Perl REQUIRED)?
There was a problem hiding this comment.
I guess my lack of experience with cmake is showing 😄 Certainly, we can do that. I guess one advantage the current approach has is that it explains why perl is needed.
There was a problem hiding this comment.
i guess anyone who want to understand why it is needed can look into cmake files, cmake show the line that triggered the error AFAIR
There was a problem hiding this comment.
Changed to find_package(Perl REQUIRED).
dusxmt
force-pushed
the
fix-arm-libvpx-asm-files
branch
from
9825220 to
4a1004c
Compare
cmake/libvpx.cmake
Outdated
| @@ -59,6 +93,8 @@ else() | |||
| list(APPEND include_directories | |||
| ${libvpx_loc}/source/config/linux/arm-neon | |||
| ) | |||
| set(ads2gas ${libvpx_loc}/source/libvpx/build/make/ads2gas.pl) | |||
There was a problem hiding this comment.
why is this needed? why just don't specify the path inside target_ads2gas_sources?
There was a problem hiding this comment.
That's because there's actually more variants of the script, like e.g. for 32-bit Windows or Apple Devices; we don't currently use any of those.
Another reason is to allow the check for Perl to be conditional
There was a problem hiding this comment.
It feels too hackish to me, why don't do the check for the system inside the function as well?
You can also move find_package inside the function, look here: https://github.com/telegramdesktop/tdesktop/blob/dev/Telegram/cmake/generate_scheme.cmake
There was a problem hiding this comment.
Thanks, that's a good idea, will do!
dusxmt
force-pushed
the
fix-arm-libvpx-asm-files
branch
from
4a1004c to
4082fa9
Compare
cmake/libvpx.cmake
Outdated
| OUTPUT | ||
| ${native_name} | ||
| COMMAND | ||
| sh -c \"cat \'${ads_name}\' | ${PERL_EXECUTABLE} \'${ads2native}\' > \'${native_name}\'\" |
There was a problem hiding this comment.
Probably you should use VERBATIM
There was a problem hiding this comment.
Also, why cat is used instead of <?
There was a problem hiding this comment.
Is there a way to pipe standard input and standard output through an application in cmake? That's the reason why I used the shell for it, since the perl script operates on stdin/stdout.
There was a problem hiding this comment.
I thought cmake passes the command to the shell, doesn't it?
There was a problem hiding this comment.
I'm not sure, I guess I can try it and see if it works
There was a problem hiding this comment.
It did indeed work, thanks! With your help, I might even learn cmake 😄
dusxmt
force-pushed
the
fix-arm-libvpx-asm-files
branch
from
4082fa9 to
859ce85
Compare
|
Looks beautiful for me now! I hope @john-preston will be able to look at this soon. |
|
Thanks! |
|
Tried to re-enable NEON on snapcraft builds, armhf still doesn' build with NEON :( |
|
Thanks for letting me know! Could you try it with #60? |
|
Well, it's not that simple. Snapcraft infrastructure builds only source that will be pushed to end users via edge channel. I.e. that would require to change tg_owt URL in snapcraft.yaml in tdesktop repo. This probably is not a good idea... |
|
I see. Well, the patch should fix it, there were two .c files which seemed like they were mutually exclusive with .asm files, but it turned out that they weren't. The remaining files in the list were indeed mutually exclusive, I checked them each with grep and saw that the functions they exported had the same names as functions in the .asm files - would result in a redefined symbol error if they were included in the 32-bit build. |
Bugfix for #48
Note: I wasn't able to test the full build because of the protobuf incompatibility in #43, but libvpx does indeed build now.