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 warning when running scripts/build_ios.sh #49457

Closed
wants to merge 6 commits into from
Closed

Fix warning when running scripts/build_ios.sh #49457

wants to merge 6 commits into from

Conversation

skyline75489
Copy link
Contributor

@skyline75489 skyline75489 commented Dec 16, 2020

  • Fixes cmake implicitly converting 'string' to 'STRING' type
  • Fixes clang: warning: argument unused during compilation: '-mfpu=neon-fp16' [-Wunused-command-line-argument]

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Dec 16, 2020

💊 CI failures summary and remediations

As of commit 6f87223 (more details on the Dr. CI page):


💚 💚 Looks good so far! There are no failures yet. 💚 💚


This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions to the (internal) Dr. CI Users group.

This comment has been revised 37 times.

@codecov
Copy link

codecov bot commented Dec 17, 2020

Codecov Report

Merging #49457 (6f87223) into master (aa18d17) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #49457      +/-   ##
==========================================
- Coverage   80.68%   80.68%   -0.01%     
==========================================
  Files        1904     1904              
  Lines      206555   206555              
==========================================
- Hits       166667   166658       -9     
- Misses      39888    39897       +9     

@skyline75489 skyline75489 changed the title Fix cmake warning about type conversion in iOS.cmake Fix warning when running scripts/build_ios.sh Dec 17, 2020
cmake/MiscCheck.cmake Outdated Show resolved Hide resolved
add_definitions("-mfpu=neon-fp16")
add_definitions("-arch" ${IOS_ARCH})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found that XNNPACK has more comprehensive flag configuration https://github.com/google/XNNPACK/blob/02bb42942798fdd0cf444b0fc90b6e20b847688f/CMakeLists.txt#L2729. Not sure if we want to use this, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To explain this change. -mfpu=neon-fp16 is only supported on armv7 devices (iPhone 4, 4s, iPhone 5, 5c, 5s). Nowadays we would normally target arm64 which -march=armv8.2-a+fp16 and related should be used.

@mrshenli mrshenli added module: build Build system issues oncall: mobile Related to mobile support, including iOS and Android triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module labels Dec 18, 2020
cmake/iOS.cmake Outdated Show resolved Hide resolved
skyline75489 and others added 2 commits January 9, 2021 06:39
Co-authored-by: Nikita Shulga <nikita.shulga@gmail.com>
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@malfet has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@malfet merged this pull request in bee6b0b.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla signed Merged module: build Build system issues oncall: mobile Related to mobile support, including iOS and Android open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants