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

Feature: Add ARM64 build for Windows #8583

Merged
merged 2 commits into from Jan 18, 2021
Merged

Conversation

orudge
Copy link
Contributor

@orudge orudge commented Jan 17, 2021

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).

src/cpu.cpp Outdated
}
#define RDTSC_AVAILABLE
#endif

/* rdtsc for macOS, or cross-platform with modern clang */
Copy link
Member

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?

src/cpu.cpp Show resolved Hide resolved
src/cpu.cpp Show resolved Hide resolved
@orudge orudge force-pushed the arm64-windows branch 2 times, most recently from e9c7fb1 to 754acb7 Compare January 17, 2021 13:14
@TrueBrain
Copy link
Member

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

@orudge
Copy link
Contributor Author

orudge commented Jan 17, 2021

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 :)

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).

@TrueBrain
Copy link
Member

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 :)

@TrueBrain TrueBrain added candidate: yes This Pull Request is a candidate for being merged needs review size: trivial This Pull Request is trivial labels Jan 17, 2021
@orudge
Copy link
Contributor Author

orudge commented Jan 17, 2021

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).

@Milek7
Copy link
Contributor

Milek7 commented Jan 17, 2021

Clang docs says: Query for this feature with __has_builtin(__builtin_readcyclecounter). Note that even if present, its use may depend on run-time privilege or other OS controlled state.
Maybe it would be better to whitelist it only where we know it works, instead of excluding APPLE?

BTW, do we really need very low-overhead timers on all platforms? Wouldn't std::chrono::high_resolution_clock be enough?

edit: Ah, there is TICC/TOCC already.

@michicc
Copy link
Member

michicc commented Jan 17, 2021

"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

@orudge
Copy link
Contributor Author

orudge commented Jan 17, 2021

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?

@michicc
Copy link
Member

michicc commented Jan 17, 2021

I'm fine with the RDTSC changes.

@orudge
Copy link
Contributor Author

orudge commented Jan 17, 2021

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.

LordAro
LordAro previously approved these changes Jan 17, 2021
Copy link
Member

@LordAro LordAro left a comment

Choose a reason for hiding this comment

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

LGTM

@orudge
Copy link
Contributor Author

orudge commented Jan 17, 2021

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!

@orudge orudge merged commit a2bd0a1 into OpenTTD:master Jan 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
candidate: yes This Pull Request is a candidate for being merged needs review size: trivial This Pull Request is trivial
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants