Add option to configure optimistic R2R instruction sets - #133514
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9b7647c5-94c1-479f-a751-a2fac60ecfb7
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9b7647c5-94c1-479f-a751-a2fac60ecfb7
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @dotnet/crossgen-contrib |
There was a problem hiding this comment.
🟡 Changes recommended
The new CLI option is added but the extended help text still documents optimistic ISA configuration only via --instruction-set, making --help --help misleading/incomplete.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a new crossgen2 command-line option to explicitly adjust the optimistic ReadyToRun instruction set list (add/remove sets) without changing the baseline ISA requirements, by plumbing an override string into the shared instruction-set configuration helper.
Changes:
- Extend
Helpers.ConfigureInstructionSetSupportto accept an optional optimistic-ISA override list and apply it when computing the optimistic set. - Add
--optimistic-instruction-setto crossgen2 and pass it through to instruction-set configuration. - Add a localized resource string for the new option’s description.
File summaries
| File | Description |
|---|---|
| src/coreclr/tools/Common/InstructionSetHelpers.cs | Adds optimistic ISA override support and refactors specifier parsing/apply logic into helpers. |
| src/coreclr/tools/aot/crossgen2/Properties/Resources.resx | Adds the resource string used as the --optimistic-instruction-set option description. |
| src/coreclr/tools/aot/crossgen2/Program.cs | Wires the new option value into ConfigureInstructionSetSupport. |
| src/coreclr/tools/aot/crossgen2/Crossgen2RootCommand.cs | Introduces the new --optimistic-instruction-set option and registers it with the root command. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
| public Option<string> InstructionSet { get; } = | ||
| new("--instruction-set") { Description = SR.InstructionSets }; | ||
| public Option<string> OptimisticInstructionSetOverrides { get; } = | ||
| new("--optimistic-instruction-set") { Description = SR.OptimisticInstructionSets }; |
|
One or more custom setup steps configured for this repository failed during this Copilot code review run: Setup steps run before each review. If the review above is missing context, or no review was posted at all, the failing step above may be the cause. See the workflow run for failure details, fix your setup steps configuration, and re-request a review. Note You can configure setup steps for Copilot code review separately from Copilot cloud agent with a |
|
We may want to extend the syntax for the existing command line option instead of adding a new command line option: #87865 (comment) Also, this was rejected before since the rules what's allowed as optimistic check are bespoke - see #89282 (comment) , so this option comes with sharp edges. |
|
Are we still rejecting entire modules instead of doing isa usage per method as well, or was that fixed? This is a bit of a complex space and Arm64 is notably one where the boundaries aren't as layered as on xarch. We could layer it and more "properly" as Arm only allows backporting an ISA so far, generally speaking. So many ISAs that are currently opportunistic are actually unreachable from "base" |
Yeah, this one feels like we'd expose sharp edges. In general, people shouldn't have to specify these instruction sets, the command line arguments are undocumented and we do make changes to it without issuing breaking change notices (#119819 (comment)). It would be better if Android repo didn't do this. Are our ARM64 optimistic instruction set defaults correct? Could we change them to extend compatibility (ideally not Android specific)? |
We reject the whole module if the required set is not satisfied. Optimistic requirements are tracked per method.
I think we should drop rcpc2 from the default optimistic set - dotnet/android#12722 (comment) . A lot of methods depend on it opportunistically, it provides relatively small perf benefit, and it is not broadly available. Android team run into it since they measure startup time on broad range of devices. I suspect that we would find number of non-Android machines are impacted as well if we have done the homework. |
R2R allowed expanding the baseline instruction set via
--instruction-set:+inssetor adding unsupported sets via--instruction-set:-insset. The optimistic set could only be disabled completely via--instruction-set:-optimistic.This PR adds the option to configure explicitly the optimistic set via a new flag: ex:
--optimistic-instruction-set:avx512,-waitpkg. This is done in the second commit of the PR, where we add/remove the flags from this override set over the default set.For context, dotnet/android#12722 noted that a lot of r2r code was excluded on older devices because some instruction set was included by default in the optimistic set, which was not available at runtime. This resulted in various r2r code being rejected. While we could hardcode the new default optimistic set for android inside crossgen2, it seems like a better approach is to allow custom configuration of this by android build (and even users themselvs via
PublishReadyToRunCrossgen2ExtraArgs)