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

platforms: use native arch parameters for apple-m1 #166535

Closed
wants to merge 2 commits into from

Conversation

Atemu
Copy link
Member

@Atemu Atemu commented Mar 31, 2022

This should allow clang to better optimise for the only aarch64-darwin CPU µarch

Closes #166401

Description of changes
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

This should allow clang to better optimise for the only aarch64-darwin CPU µarch

Closes NixOS#166401
@Atemu Atemu requested a review from thefloweringash March 31, 2022 06:44
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Mar 31, 2022
@lovesegfault
Copy link
Member

@Atemu Can you undraft this so reviewers get pinged?

It'd be great to land this :)

@Atemu
Copy link
Member Author

Atemu commented Jul 14, 2022

This is untested but sure

@Atemu Atemu marked this pull request as ready for review July 14, 2022 20:02
@lovesegfault
Copy link
Member

Why does ofborg think that this isn't a mass rebuild?

@Atemu
Copy link
Member Author

Atemu commented Jul 14, 2022

Perhaps it only checks x86_64-darwin?

@Atemu
Copy link
Member Author

Atemu commented Jul 14, 2022

nixpkgs-review reports 28506 packages updated on aarch64-darwin.

@lovesegfault
Copy link
Member

IMO we should wait for this staging cycle to end, and then just let this hit staging and see what happens.

I don't think there's another practical way of testing it

@Atemu
Copy link
Member Author

Atemu commented Jul 15, 2022

We should also do some benchmarks as µarch optimisations don't necessarily result in a performance improvement.

@vcunat
Copy link
Member

vcunat commented Jul 20, 2022

We're about to start another staging-next cycle, but I gather that you don't want to merge into this one yet? (I mean, why would you wait otherwise?)

@Atemu
Copy link
Member Author

Atemu commented Jul 20, 2022

It's not clear whether this is an improvement yet. We'd have to actually run some benchmarks to get a picture. It might very well cause huge regressions rather than improvements.

I also haven't found the time to do any builds yet, so I don't even know whether anything builds with this applied.

@vcunat vcunat marked this pull request as draft July 20, 2022 19:03
@Atemu
Copy link
Member Author

Atemu commented Jul 23, 2022

Yeah, this doesn't even build anything. Clang doesn't know about +fp-armv8+neon+zcm+zcz+fullfp16 and yet it is able to use these as target-flags when called differently? I don't understand it.

I'm not sure this is worthwhile pursuing but I'll try to build a few benchmarks with the improved but reduced set.

I'm also not quite sure whether it makes sense to set arch and cpu differently. Shouldn't these flags go in CPU only?

@Atemu
Copy link
Member Author

Atemu commented Sep 2, 2022

I don't plan on pursuing this any further.

@Atemu Atemu closed this Sep 2, 2022
@Atemu Atemu deleted the optimise-apple-m1-arch branch September 2, 2022 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants