Ensure we don't print duplicate entries for the instruction-set help#119368
Ensure we don't print duplicate entries for the instruction-set help#119368tannergooding merged 1 commit intodotnet:mainfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes duplicate entries in command-line help output for instruction sets by ensuring each instruction set is only listed once per architecture. The changes prevent scenarios where the same instruction set name appears multiple times (like base, base, base, base) in help text.
Changes
- Add deduplication logic to remove duplicate instruction set names when generating help text
- Update example instruction sets in help text to use more modern examples
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/coreclr/tools/aot/crossgen2/Properties/Resources.resx | Update instruction set example from 'avx2,bmi,lzcnt' to 'avx,aes,apx' |
| src/coreclr/tools/aot/crossgen2/Crossgen2RootCommand.cs | Add System.Linq import and DistinctBy() to deduplicate instruction sets |
| src/coreclr/tools/aot/ILCompiler/ILCompilerRootCommand.cs | Add System.Linq import, DistinctBy() for deduplication, and update example |
jeffhandley
left a comment
There was a problem hiding this comment.
I'm good with this and I would support backporting it into 10.0 RC2 if the crew wants to.
|
CC. @MichalStrehovsky, this is pending your sign-off for the ilc/crossgen command line help fix. |
Why are we ending up with multiples? I understand what the fix does, but if I am to sign off, I would like to understand what those extra entries are and why the existing |
|
@MichalStrehovsky because we need to maintain individual entries for R2R for back-compat (as per @jkotas in a previous PR): https://github.com/dotnet/runtime/blob/main/src/coreclr/tools/Common/JitInterface/ThunkGenerator/InstructionSetDesc.txt All while mapping relevant groups to a single Due to the relatively simplistic nature of the tooling, this generates runtime/src/coreclr/tools/Common/JitInterface/CorInfoInstructionSet.cs Lines 1022 to 1029 in fbf63ac We could choose to note specify "base" for one of them, but then that breaks R2R and other query checks for user code (as it assumes any managed ISA is "specifiable") resulting in a much more complex (and risky) change being required. |
MichalStrehovsky
left a comment
There was a problem hiding this comment.
Makes sense, thanks!
|
Thanks @MichalStrehovsky. Should we backport this to .NET 10 as well to ensure a clean command line output there? |
I don't have a strong opinion on this. IlcInstructionSet is not officially documented on the ILC side and is soft-supported. Not sure where it stands on the crossgen2 side. |
|
I believe it's the same for crossgen2. CC. @jkotas ? |
Right. |
|
I'll leave it as is for now, unless someone feels strongly, and wait for a customer report then. It's a bit odd that it prints the same name multiple times, but ultimately harmless and its unlikely many users will encounter the issue. It's been fixed in .NET 11 so will be correct moving forward. |
|
We did get an issue reported here: #125730 Seems like a minor enough change to consider backporting to 10. |
ilc is an internal implementation detail of the NativeAOT toolset. We do not document how to run it directly. I do not think that the backport would meet the servicing bar. |
This fixes the command line output to ensure we don't get
base, base, base, baseor similar listed as detailed in #119353