[X86][APX] Update the CPUID check logics for APX.#124624
[X86][APX] Update the CPUID check logics for APX.#124624Ruihan-Yin wants to merge 5 commits intodotnet:mainfrom
Conversation
|
Tagging subscribers to this area: @dotnet/runtime-infrastructure |
There was a problem hiding this comment.
Pull request overview
Updates x86/x64 APX feature detection in minipal_getcpufeatures to incorporate the new APX-related CPUID bit from the updated Intel APX documentation.
Changes:
- Renames the existing APX CPUID bit comment to
APX_Fand adds a follow-up CPUID query (leaf0x29) forAPX_NCI_NDD_NF. - Requires both
APX_FandCPUID.(EAX=29H,ECX=0):EBX[0]to be present before settingXArchIntrinsicConstants_Apx. - Re-issues
CPUID.(EAX=07H,ECX=1H)after the new leaf0x29query to restore state for subsequent checks.
|
Build Analysis is green, PR is ready for review. |
|
Failures look irrelevant Hi @kg @tannergooding , could you please take a look at the PR :) |
resolve comments Co-authored-by: Tanner Gooding <tagoo@outlook.com>
| if (maxCpuId >= 0x29) | ||
| { | ||
| __cpuidex(cpuidInfo, 0x00000029, 0x00000000); | ||
| if (((cpuidInfo[CPUID_EBX] & (1 << 0)) != 0) && hasApxDependencies) // AMX-TILE | ||
| { | ||
| result |= XArchIntrinsicConstants_Apx; | ||
| } |
There was a problem hiding this comment.
The PR description says the new APX_NCI_NDD_NF bit is informational and should not change APX enablement (APX features should be available as long as APX_F is set). However, the current logic only sets XArchIntrinsicConstants_Apx when both APX_F and CPUID.(EAX=29h,ECX=0):EBX[0] are present, which can change behavior under CPUID filtering/virtualization or on non-Intel CPUs. Either update the implementation so APX availability remains gated only by APX_F + OS state support (and treat the new bit as supplemental), or update the PR description/expectations to reflect the new requirement.
| if (maxCpuId >= 0x29) | ||
| { | ||
| __cpuidex(cpuidInfo, 0x00000029, 0x00000000); | ||
| if (((cpuidInfo[CPUID_EBX] & (1 << 0)) != 0) && hasApxDependencies) // AMX-TILE |
There was a problem hiding this comment.
The inline comment on this condition says // AMX-TILE, but this block is checking CPUID leaf 0x29 for the APX_NCI_NDD_NF-related bit (EBX[0]) before enabling APX. Please correct the comment so it matches what is actually being checked; misleading comments here make future maintenance/debugging harder.
| if (((cpuidInfo[CPUID_EBX] & (1 << 0)) != 0) && hasApxDependencies) // AMX-TILE | |
| if (((cpuidInfo[CPUID_EBX] & (1 << 0)) != 0) && hasApxDependencies) // APX_NCI_NDD_NF |
| if (maxCpuId >= 0x29) | ||
| { | ||
| __cpuidex(cpuidInfo, 0x00000029, 0x00000000); | ||
| if (((cpuidInfo[CPUID_EBX] & (1 << 0)) != 0) && hasApxDependencies) // AMX-TILE |
There was a problem hiding this comment.
__cpuidex(..., 0x29, 0) runs whenever maxCpuId >= 0x29, even when hasApxDependencies is false. Since CPUID is serializing and this function runs at startup, consider guarding the entire leaf-0x29 query behind hasApxDependencies (or otherwise short-circuit) so we avoid an unnecessary CPUID call on machines where APX_F/OS state support isn't present.
| if (maxCpuId >= 0x29) | |
| { | |
| __cpuidex(cpuidInfo, 0x00000029, 0x00000000); | |
| if (((cpuidInfo[CPUID_EBX] & (1 << 0)) != 0) && hasApxDependencies) // AMX-TILE | |
| if (hasApxDependencies && (maxCpuId >= 0x29)) | |
| { | |
| __cpuidex(cpuidInfo, 0x00000029, 0x00000000); | |
| if ((cpuidInfo[CPUID_EBX] & (1 << 0)) != 0) // AMX-TILE |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This PR is to update the new APX CPUID bits as documented in the specification:
This PR is to introduce the new CPUID bit only, and it is not intended to change the JIT behavior based on the presence of the new bit. (
APX_Fbit will guarantee the newAPX_NDD_NCI_NFbit on Intel processors as documented above). All the features and optimizations will be available as long asAPX_Fbit is set.