Skip to content

Add Apple M4 host detection #117530

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

Merged
merged 1 commit into from
Jan 16, 2025
Merged

Conversation

madsmtm
Copy link
Contributor

@madsmtm madsmtm commented Nov 25, 2024

Add Apple M4 host detection, which fixes rust-lang/rust#133414.

Also add support for older ARM families (this is likely never going to get used, since only macOS is officially supported as host OS, but nice to have for completeness sake). Error handling (checking CPUFAMILY_UNKNOWN) is also included here.

Finally, add links to extra documentation to make it easier for others to update this in the future.

NOTE: These values are taken from mach/machine.h the Xcode 16.2 SDK, and has been confirmed on an M4 Max in rust-lang/rust#133414 (comment).

@madsmtm
Copy link
Contributor Author

madsmtm commented Nov 25, 2024

CC @jroelofs who reviewed #82100 (I don't know the procedures for who to ask, sorry if you're the wrong person to tag).

Copy link

github-actions bot commented Nov 25, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@madsmtm madsmtm force-pushed the apple-m4-host-detection branch from 7944594 to 82bd606 Compare November 25, 2024 09:11
@zero9178 zero9178 requested a review from jroelofs November 25, 2024 12:42
@madsmtm madsmtm force-pushed the apple-m4-host-detection branch 2 times, most recently from 6074a26 to 7b5d723 Compare November 25, 2024 22:33
@madsmtm madsmtm force-pushed the apple-m4-host-detection branch 2 times, most recently from 78fd5d4 to a9966d9 Compare November 29, 2024 07:58
case CPUFAMILY_ARM_EVEREST_SAWTOOTH: // A16
case CPUFAMILY_ARM_IBIZA: // M3
case CPUFAMILY_ARM_PALMA: // M3 Max
case CPUFAMILY_ARM_COLL: // A17
Copy link
Contributor

Choose a reason for hiding this comment

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

apple-a17 is it's own thing, this shouldn't get merged with apple-m3. See the ProcessorAlias<> entries in AArch64Processors.td

Copy link
Contributor Author

@madsmtm madsmtm Dec 2, 2024

Choose a reason for hiding this comment

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

Hmm, why is apple-m3 aliased to apple-a16 and not apple-a17 in the first place?

Copy link
Contributor

Choose a reason for hiding this comment

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

apple-m3 shouldn't be (and isn't) aliased with apple-a17

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But why not? The Wikipedia article (which of course isn't a reliable resource) lists Apple M3 as a variant of Apple A17 Pro?

In any case, apple-a16 and apple-a17 don't seem to have any functional differences in the source code (yet), so the point is perhaps a bit moot anyways?

Copy link
Contributor

Choose a reason for hiding this comment

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

Wikipedia isn't a reliable source for this at all. Those pages often quote llvm's tablegen files as a source of truth on things, which sometimes aren't precisely correct for various reasons, both upstream-visible, and sometimes not-upstream-visible ones that I'm not going to explain here. We have them as separate processor definitions in AArch64Processors.td for reasons, and as lame of a justification that is, this code should respect that.... please.

Copy link
Contributor Author

@madsmtm madsmtm Dec 2, 2024

Choose a reason for hiding this comment

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

Fair, apologies if it came out wrong, my intention wasn't to contest your knowledge, I was mostly just curious ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, sorry, these things sometimes tread close to topics we can't really discuss externally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries!

I've updated the PR, please verify that you think it's correct?

@madsmtm madsmtm force-pushed the apple-m4-host-detection branch from a9966d9 to 6aa2846 Compare December 2, 2024 18:43
Also add support for older ARM families (this is likely never going to
get used, since only macOS is officially supported as host OS, but nice
to have for completeness sake). Error handling (checking
CPUFAMILY_UNKNOWN) is also included here.

Finally, add links to extra documentation to make it easier for others
to update this in the future.

These values should be up to date with Xcode 16.2 beta 3.
@madsmtm madsmtm force-pushed the apple-m4-host-detection branch from 6aa2846 to b4efe4b Compare December 6, 2024 13:19
Copy link
Contributor

@jroelofs jroelofs left a comment

Choose a reason for hiding this comment

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

Forgot about this... sorry. LGTM

@madsmtm
Copy link
Contributor Author

madsmtm commented Jan 16, 2025

No problem!

(I can't merge it myself, btw, if you're expecting that ;) ).

@jroelofs jroelofs merged commit a082cc1 into llvm:main Jan 16, 2025
8 checks passed
@madsmtm madsmtm deleted the apple-m4-host-detection branch January 16, 2025 17:03
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.

apple-m3 detected as the native CPU for nightly rustc on apple-m4
3 participants