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

Enable Arm64 RDM on Windows #109493

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

saucecontrol
Copy link
Member

@saucecontrol saucecontrol commented Nov 3, 2024

Fixes #39094

On Windows, we currently never enable RDM intrinsics because IsProcessorFeaturePresent does not have a flag for the Armv8.1 RDM optional extension. This results in Rdm.IsSupported returning different values on Windows vs Linux on the same hardware (realistically, anything newer than 2016).

I finally found enough documentation to support enabling RDM by implication. The docs for Armv8.1 extensions include the following:

FEAT_RDM is OPTIONAL from Armv8.0.
In an Armv8.1 implementation, if FEAT_AdvSIMD is implemented, FEAT_RDM is implemented.

Since we require a baseline of AdvSIMD support, we may assume that if we have an Armv8.1 processor, it has RDM support. This leads to a second problem, which is that there is also no IsProcessorFeaturePresent flag for Armv8.1 baseline. However, from the generic docs for Armv8 extensions:

An Armv8.x-A processor can implement any features from the next .x extension. However, it cannot implement features from
any .x extension after that.
For example, a processor described as implementing Armv8.1-A:
• Must implement all the mandatory features of Armv8.0-A and Armv8.1-A.
• Is permitted to implement some features from Armv8.2-A.
• Is not permitted to implement features from Armv8.3-A, Armv8.4-A, and so on

We may assume that the presence of any Armv8.2 optional feature means we have at least Armv8.1. And because IsProcessorFeaturePresent does have a flag for the Armv8.2 DP optional extension, we may safely assume that its presence also implies RDM.

@saucecontrol
Copy link
Member Author

CC @dotnet/jit-contrib

Doesn't look like the bot notified anyone on this one. I couldn't get a clean CI run, but none of the failures are related.

Copy link
Member

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

Sounds reasonable to me.

I finally found enough documentation to support enabling RDM by implication

@a74nh - can you double check this?

@a74nh
Copy link
Contributor

a74nh commented Dec 9, 2024

We may assume that the presence of any Armv8.2 optional feature means we have at least Armv8.1. And because IsProcessorFeaturePresent does have a flag for the Armv8.2 DP optional extension, we may safely assume that its presence also implies RDM.

This is a reasonable assumption to make. However, I think you can go a bit further. This change will miss some cases - you'll disable RDM for any 8.2 or 8.3 without DP and all 8.1.

For that reason, I'd recommend you also enable RDM for PF_ARM_V81_ATOMIC_INSTRUCTIONS_AVAILABLE, PF_ARM_V83_JSCVT_INSTRUCTIONS_AVAILABLE, PF_ARM_V83_LRCPC_INSTRUCTIONS_AVAILABLE and PF_ARM_SVE_INSTRUCTIONS_AVAILABLE. All of these are post 8.1 features, and if available must mean that RDM is present.

You'll still miss some cases, but I think that's the best you can do given the available outputs in windows.

@a74nh
Copy link
Contributor

a74nh commented Dec 9, 2024

Actually, scratch that. For any given feature, it's possible that either now or in the future it is backported to an earlier architecture revision due to a particular customer requirement.

DP (or DotProd) is currently earliest in 8.2. However, it's possible this this could be backported to 8.1 or 8.0 in the future. This feels very unlikely, but it's possible. If that happened, then that's going to create all sorts of issues here.

For that reason, I'd recommend you also enable RDM for PF_ARM_V81_ATOMIC_INSTRUCTIONS_AVAILABLE, PF_ARM_V83_JSCVT_INSTRUCTIONS_AVAILABLE, PF_ARM_V83_LRCPC_INSTRUCTIONS_AVAILABLE and PF_ARM_SVE_INSTRUCTIONS_AVAILABLE. All of these are post 8.1 features, and if available must mean that RDM is present.

Definitely do not this this!

The correct and safe solution here is that the windows API needs expanding to include more features. The full list is much bigger and this issue is only going to occur again for other features.

@saucecontrol
Copy link
Member Author

saucecontrol commented Dec 9, 2024

DP (or DotProd) is currently earliest in 8.2. However, it's possible this this could be backported to 8.1 or 8.0 in the future. This feels very unlikely, but it's possible. If that happened, then that's going to create all sorts of issues here.

In the unlikely event this actually happened and such a device happened to run Windows and someone tried to run a .NET app using RDM on that device, we do still have the DOTNET_EnableArm64Rdm config knob as an escape hatch.

The correct and safe solution here is that the windows API needs expanding to include more features.

That would be ideal, but with no movement from the Windows side in over 4 years, I don't know what else we can do. This is broken on hardware (including Microsoft-branded) that exists and runs Windows today, so a workaround seems reasonable.

The main issue here is that the config knobs can disable an ISA but can't forcibly enable it, meaning there's no way to use the hardware we know is present.

@tannergooding
Copy link
Member

That would be ideal, but with no movement from the Windows side in over 4 years, I don't know what else we can do

@jkotas, @kunalspathak. Anyone we can reach out to and ensure such config knobs are exposed? Unlike x64, there is no user-mode instruction (like CPUID) which allows querying of said support so we're reliant on the OS exposing the relevant queries.

we do still have the DOTNET_EnableArm64Rdm config knob as an escape hatch.

This isn't a knob that's really meant for use in production scenarios and certainly not by typical end users of applications. Rather its something that is primarily meant for testing scenarios, so that developers can validate their fallback paths instead.

I don't think any scenario where we emit invalid instructions by default for regular code is going to be viable for the shipping product.

@jkotas
Copy link
Member

jkotas commented Dec 9, 2024

@jkotas, @kunalspathak. Anyone we can reach out to and ensure such config knobs are exposed?

Done (both of you are on cc line)

@a74nh
Copy link
Contributor

a74nh commented Dec 10, 2024

but with no movement from the Windows side in over 4 years

If it helps, we might be able to provide some code could be used by the kernel to do the detection of all the features.

@tannergooding
Copy link
Member

tannergooding commented Dec 13, 2024

Had an internal conversation that basically boils down to Windows doesn't have any plans to expose a FEAT_RDM flag at this point in time.

Rather instead we can encode this essentially as if (OperatingSystem.IsWindowsVersionAtLeast(10, 0, 22000, 0)) { RdmSupported = true; }, effectively encoding that Windows 11 requires Armv8.1 or later (where AdvSimd support implies that RDM is provided)

This will exclude Windows 10, but that is going out of support next year in October 2025; prior to when .NET 10 will ship.

CC. @jkotas

@jkotas
Copy link
Member

jkotas commented Dec 14, 2024

More context: Rdm is supported on all Arm hardware supported by Windows 11 today and it is not expected to change in future.

@@ -475,6 +503,12 @@ int minipal_getcpufeatures(void)
// FP and SIMD support are enabled by default
result |= ARM64IntrinsicConstants_AdvSimd;

// RDM does not have an IsProcessorFeaturePresent flag, but it is a requirement for Windows 11
if (IsWindows11OrGreater())
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

@MichalPetryka MichalPetryka Dec 18, 2024

Choose a reason for hiding this comment

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

IsWindowsVersionOrGreater also lies about Windows version and says 6.2 without a manifest. Using it will return invalid values with custom hosts.

(Do note that in some cases even RtlGetVersion lies about the Windows version and returns 6.2)

Copy link
Member

Choose a reason for hiding this comment

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

IsWindowsVersionOrGreater also lies about Windows version and says 6.2 without a manifest. Using it will return invalid values with custom hosts.

This is intentional on the part of Windows and is covered under the Remarks section of the deprecated GetVersion/GetVersionEx functions and the recommended VersionHelper replacements: https://learn.microsoft.com/en-us/windows/win32/api/versionhelpers/

Applications must be explicitly manifested for Windows 10+, this manifest is likewise required for functions to use newer behaviors, for some (but not all) newer exports to be available, etc.

This is also required to ensure that user-selected comapt settings are respected, among other considerations. Trying to manually determine the actual underlying OS version is never recommended and liable to cause undefined or other problematic behaviors.

Copy link
Member

Choose a reason for hiding this comment

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

@jkotas, notably the managed OperationgSystem.IsWindowsVersionAtLeast check looks to use Environment.OSVersion.Version which is currently populated via RtlGetVersion

This does not correctly respect the manifest file and will mismatch from the underlying GetVersionEx call and the corresponding IsWindowsVersionOrGreater calls that are currently recommended for use.

Is that known/intentional? It seems like it should be unintentional and like it could cause various bugs/issues, especially where application behaviors may not match up...

Copy link
Contributor

Choose a reason for hiding this comment

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

Is that known/intentional? It seems like it should be unintentional and like it could cause various bugs/issues, especially where application behaviors may not match up...

https://learn.microsoft.com/en-us/dotnet/core/compatibility/core-libraries/5.0/environment-osversion-returns-correct-version

Copy link
Member

Choose a reason for hiding this comment

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

I agree that we should fix the manifests for Windows. I was trying to point out that the Windows developers at large try to avoid the OS version shimming since it does not work well for them.

Once that manifest file exists; then the question of using RtlGetVersion vs IsWindowsVersionOrGreater essentially goes away

It does not go away completely since manifests do not work well for plugins.

Copy link
Member

Choose a reason for hiding this comment

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

It does not go away completely since manifests do not work well for plugins.

Not sure I understand this?

Plugins and libraries in general are at the mercy of the application as it decides the OS context under which code runs. If the app doesn't include a manifest then IsWindowsVersionOrGreater correctly reports that the OS Context under which it is running and things like RDM or other functionality detailed above may not be available. As far as the app/library/plugin is concerned, unless it intentionally digs deeper, it's running on 6.2 -- Windows 8.

If the app does include a manifest, then the same APIs reports the lower of the greatest listed OS that is supported and the actual underlying OS being run on. This is the general target that the code should consider that it is running against and so everything continues to work.

A plugin or library could "require" that they run on a particular version or later and there isn't a way for them to force an application to use such a context; but they can add a relevant DllMain (C/C++) or Module Initializer/Static Constructor (.NET) which validates this and fails without -- most often, however, its better/easier to just not light up on the optional functionality. -- .NET assemblies also have the option to use SupportedOSPlatform and/or TargetFramework (net8.0-windows10.0.22000.0 as an example) to further help enforce such usage.

Copy link
Member

Choose a reason for hiding this comment

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

The problem is that the OS compat shims only isolate you from OS-version specific compat problems that were decided to be important enough to be shimmed. Many real-world OS-version specific compat issues are not shimmed and you are on your own. If you have to know the actual OS version to implement OS version specific workaround, you have to be able to get around the version lie shims.

The case study close to home is .NET Framework. Some OS version checks in .NET Framework had to get the real OS version and avoid version lie shims. Things would be broken otherwise. You can see some of that code in the initial .NET Core payload that included .NET Framework specifics:

// GetOSVersion - Gets the real OS version bypassing the OS compatibility shim
// Mscoree.dll resides in System32 dir and is always excluded from compat shim.
// This function calls mscoree!shim function via mscoreei ICLRRuntimeHostInternal interface
// to get the OS version. We do not do this PAL or coreclr..we direclty call the OS
// in that case.

Copy link
Member Author

@saucecontrol saucecontrol Dec 19, 2024

Choose a reason for hiding this comment

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

I've reverted this back to the original implication and added a big comment.

Given the oldest processor supported on Windows 11 is based on the Arm Cortex-A76 core (and its small core sibling Cortex-A55), which implements both RDM and DP, I don't believe there is any chance of either false positive or false negative in any supported scenario.

I also believe this implication is, for all intents and purposes, identical to the implication that "we are running on Windows 11, therefore we have hardware that meets the minimum requirements for Windows 11" but without the surprising difficulty of answering the question "are we really running on Windows 11?"

Copy link
Member Author

Choose a reason for hiding this comment

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

I did one last round of reading and wanted to add one final context note:

There is a single processor model on the Windows 11 support list that might implement RDM but not DP: the Snapdragon 850. That model has Cortex-A75 large cores and Cortex-A55 small cores. I don't have one to test, but docs seem to indicate that the A75 does not implement DP, while the A55 does. More commonly, the A55 appears as the small core alongside the Cortex-A76, which also supports both RDM and DP. The A76/A55 combination is what's in the Surface X 2019 (Microsoft SQ1) model, Snapdragon 7c, and the Broadcom SoC used in Raspberry Pi 5.

Windows 11 does not actually require Armv8.1 until 24H2, according to https://www.neowin.net/news/first-generation-windows-on-arm-pcs-will-not-be-able-to-run-windows-11-24h2/. There are reports that people have been running Windows 11 on unsupported devices with older builds and that 24H2 actually fails to boot.

So the DP->RDM implication includes a possible false negative on one supported processor model, while the Windows11->RDM implication includes possible false positives in unsupported configurations on older builds.

False negatives being preferable to false positives, I believe the implementation here is still right, but I can update the comment if it's worth churning the CI to get the exception noted there.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch-arm64 area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[win-arm64] Update EEJitManager::SetCpuInfo in codeman.cpp to support ARMv8.1-RDMA and ARMv8.2-DotProd
6 participants