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
Feature: Add ARM64 build for Windows #8583
Conversation
src/cpu.cpp
Outdated
} | ||
#define RDTSC_AVAILABLE | ||
#endif | ||
|
||
/* rdtsc for macOS, or cross-platform with modern clang */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment confuses me, as there is no macOS mentioned lower. This will do poorly over time, I think. Maybe just say: Cross-platform support via clang
or something?
e9c7fb1
to
754acb7
Compare
In general, there is a reason why we have this PR template :P You try to bypass it, but you get the question right back at you :D What you now have in "Motivation" is a "Description". You do not mention WHY we should have this. This would be a motivation .. what motivated you to add this, besides the: because I could? :) So the question that comes to mind: what devices does this service? We can just add any target to our release, but there is also the expectation that we support it if we do. So it is good to know for us what devices are targeted here :) Tnx :D |
There are an increasing number of Windows ARM laptops on the market - Surface Pro X, Snapdragon 8cx, an assortment of Lenovo laptops, etc. I suspect with the performance demonstrated by the new Apple chips, plus Microsoft improving their x86/x64 emulation on ARM, there might be increasing uptake of ARM Windows laptops in the near future. At least this way we're prepared. :) I am happy to support this as much as I can (I am looking into things like blitters using Neon intrinsics etc for improved performance). |
Tnx! I am fine with the GitHub Actions changes; the RDTSC I would like another devs opinion about, as I am not familiar enough with that code to spot any mistakes there :) |
I've taken another look at that actually and amended it - although the code does compile on M1, it faults due to the necessary counter apparently not being user-accessible. Funnily enough it does work on an M1 in a Windows VM... If I find a solution for M1 in the future I'll amend that; the code as it is however should be fine on non-Mac platforms (tested on Windows by me). |
Clang docs says: BTW, do we really need very low-overhead timers on all platforms? Wouldn't edit: Ah, there is TICC/TOCC already. |
"The high_resolution_clock is not implemented consistently across different standard library implementations, and its use should be avoided." https://en.cppreference.com/w/cpp/chrono/high_resolution_clock |
Perhaps I will implement this for MSVC only in this PR and we can split off clang/other platforms to another case for further discussion? |
I'm fine with the RDTSC changes. |
Reading up a bit more, there are suggestions that PMCCNTR may not be accessible on a Raspberry Pi on Linux either. As such I'll remove that change for the moment, and just add support for ARM/ARM64 MSVC. I believe we may be able to read PMUSERENR to determine if user access to the performance counters is available, but that would be a task for a different PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I realised the binary name for the web site was still coming out as "win64". Unfortunately, because we use the Ninja generator, detecting the platform isn't as straightforward as we might hope. The 'Platform' environment variable (which contains the MSVC toolset platform) needs to be checked as CMake itself has no idea what platform it is building for! |
Motivation / Problem
This adds a nightly (and release) ARM64 build for Windows. (While we could technically also support ARM32, I'm not sure there's enough of a user base for it to be worth supporting.)
This also enables support for NSIS arm64 builds (NSIS itself remains x86 but Windows for ARM does have x86 emulation support, and the installer works fine in my testing).