-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Update the naot instruction set support for .NET 10+ #2828
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
Conversation
{ | ||
if (IsX86Avx512Supported) | ||
{ | ||
return "x86-64-v4"; |
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.
These are the official short names for the ISA groupings.
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.
While I am very happy about the fix and other improvements, I find myself hesitant about the new "display name":
- most engineers know what AVX is, I am not sure about the ISA groupings (at least I was not familiar with those names so far)
- the ISA groupings contain information that is already displayed on the screen: the architecture
BenchmarkDotNet v0.15.3-develop (2025-09-11), Windows 11 (10.0.26100.6584/24H2/2024Update/HudsonValley)
AMD Ryzen Threadripper PRO 3945WX 12-Cores 3.99GHz, 1 CPU, 24 logical and 12 physical cores
.NET SDK 10.0.100-preview.6.25358.103
- [Host] : .NET 8.0.20 (8.0.20, 8.0.2025.41914), X64 RyuJIT AVX2
+ [Host] : .NET 8.0.20 (8.0.20, 8.0.2025.41914), X64 RyuJIT x86-64-v3
@AndreyAkinshin thoughts?
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.
I have the same concern: AVX2 is much more convinient for the users unlike x86-64-v3. Since we have enough space in this line, I'm suggesting to print both options.
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.
I'm in a bit of a disagreement here.
The names were formalized several years ago by Intel, AMD, LLVM, and others explicitly because it was becoming too unwieldy. Both to list all the options and because of the rapidly exploding number of combinations that could be specified. The manufacturers largely do not want developers thinking in terms of individual ISAs so much anymore and want them to essentially target "profiles" instead, for simplicity.
This also follows the general model that Arm64 and other manufacturers use as well. You declare your base profile (i.e. armv8.0-a
) and then optionally list any key extensions on top (i.e. armv8.0-a + lse
). So the intent is that you squash the complex profiles together (i.e. x86-64-v4
) and then list key important extensions that aren't part of a defined profile on top (i.e. x86-64-v4 + APX
).
This is likewise how we've changed the JIT to largely support the feature sets, how the JIT Dumps and JIT Disasm now print the information, and how the configuration knobs for manual disablement work.
We explicitly want developers to stop thinking in terms of "AVX2" or "AVX512" in the common cases, but they still have the ability to print the full ISA list if desired (via GetFullInfo
)
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.
I have no objections to using the new notation. However, I want us to maintain backward compatibility with user habits.
We explicitly want developers to stop thinking in terms of "AVX2" or "AVX512" in the common cases
BenchmarkDotNet does not have this goal. In this project, we want to provide a convenient user experience so that people can quickly gain insights from the information presented based on their existing knowledge. If we decide that we want to promote the new notation in addition to a convenient user experience, it makes even more sense to display both versions. This approach will naturally help people create a mental map without needing to search for details when they encounter unfamiliar labels. We can use the new notation in the first place, but then explicitly specify instruction sets inside.
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.
Isn’t it fairly counterintuitive to go against what the hardware manufacturers, compiler developers, and underlying platform have all agreed is the best for long term UX?
This wasn’t some arbitrary decision, but the result of extensive discussion based around real world pain points and confusion caused by the rapidly expanding complexity of ISAs
———-
Part of the consideration is that typical users don’t understand what things like AVX2 are and those that roughly know often get it confused for meaning the wrong thing. It’s only the people that directly work with intrinsics that actually understand
This is very similar in concept to people understanding the brand name “Windows 8” or “Windows 11” but not understanding how that maps to version numbers (6.2 and 10.0.22000)
The profiles are meant to be explicitly clear and give a point that shows something as being meaningfully greater without getting into the technical nuance that is irrelevant to the typical case — I.e listing things that won’t actually impact codegen or make a meaningful difference; which is an actual “problem” with how BDN lists ISAs today, surfacing info that likely isn’t causing profiling differences by default to typical devs
Particularly with how .NET actually works, this also applies here and simplifies what devs should be considering without removing pertinent info. They see the different baselines we support and do codegen differentiation against, nothing more unless they ask for details
———-
I strongly expect that the distaste here is more an initial visceral reaction to the “cheese being moved” from devs that do have more technical knowledge of the space. Such a reaction would likely be less strong if you had less knowledge of the space or were more ingrained with the space and kept up to date with the best practices and recommendations for dealing with hardware profiles as they’ve evolved over the past 5-10 years (not just for x64 but also for Arm64 and other major cpu vendors who are all unifying on such approaches)
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.
OK, you convinced me; let's try your way. =)
I still expect that it will cause confusion for some of our users, but it's probably not a big problem.
if (IsX86PclmulqdqSupported) yield return "PCLMUL"; | ||
if (IsX86PopcntSupported) yield return "POPCNT"; | ||
{ | ||
if (IsX86Avx10v2Supported) yield return "AVX10v2"; |
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 prints the full set of ISAs supported based on the way we group them in RyuJIT
} | ||
|
||
#pragma warning disable CA2252 // Some APIs require opting into preview features | ||
internal static bool IsX86BaseSupported => |
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.
These are grouped based on the DOTNET_Enable*
flags we expose so that we're only querying what can actually be toggled on/off.
Co-authored-by: Tim Cassell <[email protected]>
Still getting compile errors. PTAL |
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.
Thank you for providing the fix @tannergooding !
{ | ||
if (IsX86Avx512Supported) | ||
{ | ||
return "x86-64-v4"; |
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.
While I am very happy about the fix and other improvements, I find myself hesitant about the new "display name":
- most engineers know what AVX is, I am not sure about the ISA groupings (at least I was not familiar with those names so far)
- the ISA groupings contain information that is already displayed on the screen: the architecture
BenchmarkDotNet v0.15.3-develop (2025-09-11), Windows 11 (10.0.26100.6584/24H2/2024Update/HudsonValley)
AMD Ryzen Threadripper PRO 3945WX 12-Cores 3.99GHz, 1 CPU, 24 logical and 12 physical cores
.NET SDK 10.0.100-preview.6.25358.103
- [Host] : .NET 8.0.20 (8.0.20, 8.0.2025.41914), X64 RyuJIT AVX2
+ [Host] : .NET 8.0.20 (8.0.20, 8.0.2025.41914), X64 RyuJIT x86-64-v3
@AndreyAkinshin thoughts?
) Updated [BenchmarkDotNet](https://github.com/dotnet/BenchmarkDotNet) from 0.15.3 to 0.15.4. <details> <summary>Release notes</summary> _Sourced from [BenchmarkDotNet's releases](https://github.com/dotnet/BenchmarkDotNet/releases)._ ## 0.15.4 Full changelog: https://benchmarkdotnet.org/changelog/v0.15.4.html ## Highlights Allow override method/property for ParamsSource ([#2832](dotnet/BenchmarkDotNet#2832)) Commits viewable in [compare view](dotnet/BenchmarkDotNet@v0.15.3...v0.15.4). </details> Updated [BenchmarkDotNet.Diagnostics.Windows](https://github.com/dotnet/BenchmarkDotNet) from 0.15.2 to 0.15.4. <details> <summary>Release notes</summary> _Sourced from [BenchmarkDotNet.Diagnostics.Windows's releases](https://github.com/dotnet/BenchmarkDotNet/releases)._ ## 0.15.4 Full changelog: https://benchmarkdotnet.org/changelog/v0.15.4.html ## Highlights Allow override method/property for ParamsSource ([#2832](dotnet/BenchmarkDotNet#2832)) ## 0.15.3 Full changelog: https://benchmarkdotnet.org/changelog/v0.15.3.html ## Highlights Improvements: - Naot instruction set support for .NET 10+, migrate to ISA groupings in CPU summary [#2828](dotnet/BenchmarkDotNet#2828) - Support benchmark filtering for TestAdapter [#2662](dotnet/BenchmarkDotNet#2662) [#2788](dotnet/BenchmarkDotNet#2788) - Support non-primitive external types in `ArgumentsSource` [#2820](dotnet/BenchmarkDotNet#2820) - Enable MSBuild parallel build via `--nodeReuse:false` [#2693](dotnet/BenchmarkDotNet#2693) [#2814](dotnet/BenchmarkDotNet#2814) - Improve CPU detection [#2747](dotnet/BenchmarkDotNet#2747) [#2749](dotnet/BenchmarkDotNet#2749) - Enable assembly signing for debug build [#2774](dotnet/BenchmarkDotNet#2774) Deprecations: - Deprecated `WithNuget` [#2812](dotnet/BenchmarkDotNet#2812) Bug fixes: - Fix `InvalidOperationException` in diagnosers [#2758](dotnet/BenchmarkDotNet#2758) [#2805](dotnet/BenchmarkDotNet#2805) - Fix file detection in `NativeMemoryProfiler` [#2794](dotnet/BenchmarkDotNet#2794) [#2795](dotnet/BenchmarkDotNet#2795) - Fix long file paths issue in `EtwProfiler` [#2807](dotnet/BenchmarkDotNet#2807) [#2808](dotnet/BenchmarkDotNet#2808) - Fix log duplications in TestAdapter [#2790](dotnet/BenchmarkDotNet#2790) - Fix x86 disassembler error for net462 [#2792](dotnet/BenchmarkDotNet#2792) - Fix `IsNetCore` and `IsNativeAOT` for single-file apps without AOT [#2799](dotnet/BenchmarkDotNet#2799) - Fix density plot generation in `RPlotExporter` for latest version of R [#2809](dotnet/BenchmarkDotNet#2809) Commits viewable in [compare view](dotnet/BenchmarkDotNet@v0.15.2...v0.15.4). </details> Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
The instruction set support was simplified in the JIT for .NET 10 and so this updates the support to better match said updates while still trying to remain backwards compatible with .NET 6-9.
This resolves dotnet/runtime#119353