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

Feature: Add ARM64 build for Windows #8583

merged 2 commits into from Jan 18, 2021

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 */

This comment has been minimized.

@TrueBrain

TrueBrain Jan 17, 2021
Member

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 orudge:arm64-windows branch 2 times, most recently from e9c7fb1 to 754acb7 Jan 17, 2021
@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jan 17, 2021

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

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

@orudge
Copy link
Contributor Author

@orudge 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 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 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 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 michicc commented Jan 17, 2021

I'm fine with the RDTSC changes.

@orudge orudge force-pushed the orudge:arm64-windows branch from 2d61752 to 27e4b7d Jan 17, 2021
@orudge
Copy link
Contributor Author

@orudge 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.

Copy link
Member

@LordAro LordAro left a comment

LGTM

@orudge orudge force-pushed the orudge:arm64-windows branch from 27e4b7d to 0e19f6e Jan 17, 2021
@orudge
Copy link
Contributor Author

@orudge 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
8 checks passed
8 checks passed
Emscripten
Details
Commit checker
Details
Check preview needs update Check preview needs update
Details
Linux (clang, clang++)
Details
Linux (gcc, g++)
Details
Mac OS (x64, x86_64)
Details
Windows (x86)
Details
Windows (x64)
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants