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.

Sign up for GitHub

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

Jump to bottom

Fix NATIVE_FP16 macro in our fork of half.h #7449

Merged
merged 1 commit into from
Jan 9, 2025
Merged

Fix NATIVE_FP16 macro in our fork of half.h #7449

merged 1 commit into from
Jan 9, 2025

Conversation

swolchok
Copy link
Contributor

Simple build unbreak for Raspberry Pi 5 -- fp16 is an optional aarch64 feature.

@pytorch-bot PyTorch Bot
Copy link

pytorch-bot bot commented Dec 27, 2024 •
edited
Loading

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/7449

Note: Links to docs will display an error until the docs builds have been completed.

⏳ No Failures, 8 Pending

As of commit fc44779 with merge base 39e8538 (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 27, 2024
@swolchok swolchok added the release notes: runtime Changes related to the core runtime which loads the program methods, initializes delegates, and runs label Jan 2, 2025
@swolchok swolchok requested review from dbort and kimishpatel January 2, 2025 23:46
@swolchok
Copy link
Contributor Author

swolchok commented Jan 7, 2025

ping

@swolchok swolchok requested a review from mergennachin January 7, 2025 23:55
mergennachin
mergennachin reviewed Jan 8, 2025
View reviewed changes
runtime/core/portable_type/half.h Outdated
@@ -15,7 +15,7 @@
#include <ostream>

#if defined(__GNUC__) || defined(__clang__)
#if defined(__aarch64__)
#if defined(__aarch64__) && defined(__ARM_FEATURE_FP16)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that the correct definition actually?

I can see __ARM_FEATURE_FP16_SCALAR_ARITHMETIC, __ARM_FEATURE_FP16_FML and __ARM_FEATURE_FP16_VECTOR_ARITHMETIC in https://developer.arm.com/documentation/101028/0012/5--Feature-test-macros

But I don't see a standalone __ARM_FEATURE_FP16 macros in the document.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's actually a typo, it would be good to have a regression test somewhere to catch these types of errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

regression test

I guess we could disassemble half_test.o and make sure it contains the relevant aarch64 instructions. Should probably just do that in upstream PyTorch though; after #7040 is finally ready to go, this file will be powered by c10/util/Half.h anyway.

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 probably wanted __ARM_FEATURE_FP16_SCALAR_ARITHMETIC, for the record.

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 manually disassembled half_test.cpp.o and confirmed that 1) it wasn't the same as baseline before I fixed the defined check to __ARM_FEATURE_FP16_SCALAR_ARITHMETIC 2) it is now the same as baseline

@swolchok
Fix NATIVE_FP16 macro in our fork of half.h
fc44779 
Simple build unbreak for Raspberry Pi 5 -- fp16 is an optional aarch64 feature.
@swolchok swolchok merged commit fb1cc93 into main Jan 9, 2025
44 checks passed
@swolchok swolchok deleted the native-fp16-fix branch January 9, 2025 01:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mergennachin mergennachin mergennachin left review comments

@kimishpatel kimishpatel kimishpatel approved these changes

@dbort dbort Awaiting requested review from dbort

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. release notes: runtime Changes related to the core runtime which loads the program methods, initializes delegates, and runs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@swolchok @mergennachin @kimishpatel @facebook-github-bot