-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Add support for --instruction-set:native
#87865
Add support for --instruction-set:native
#87865
Conversation
This allows compiling for the ISA extensions that the currently running CPU supports. Fixes dotnet#73246.
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas Issue DetailsThis allows compiling for the ISA extensions that the currently running CPU supports. This should work for both crossgen2 and ILC.
I didn't test anything but x64 Windows but since this will involve a ton of annoying manual testing, sending for review now. Fixes #73246. Cc @dotnet/ilc-contrib
|
} | ||
HardwareIntrinsicHelpers.AddRuntimeRequiredIsaFlagsToBuilder(instructionSetSupportBuilder, cpuFeatures); | ||
} | ||
else if (instructionSet != null) |
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.
Open issue: should we allow computing optimistic instruction set or skip that when native
was specified.
(Or should we actually skip the optimistic one if any instruction-set
is specified?)
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.
Should we make it configurable? For example, we can introduce optimistic
instruction set that would enable the optimistic checks; or we can introduce ?
qualifier for instruction sets (in addition to the existing +
and '-` qualifiers) that would enable optimistic checks for given instruction set.
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.
That sounds like a good plan.
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.
Does that mean that you'd have to specify eg. native,foo?,bar?,baz?
(with foo
etc. being all opportunistic instruction sets you want guarded paths to be generated for) to get the resulting binary to use all the instruction sets available on the current machine, and guarded paths for all the other ones? I wonder if it would make sense to have a simplified way to express:
"Compile for
native
, but also add guarded paths for all existing other ISAs supported by the runtime".
Basically as a way to avoid having to manually look all of them up and add them with ?
as a suffix for each after native
. Just to get the idea across, something like native,all?
. Or is something like this already possible in some other way?
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.
Left some comments on cpufeatures.c
.
It would be nice to switch src/coreclr/vm+pal implementation of the detection over to the minipal one as well. It should be straightforward (delete what is there and switch to the minipal instead) and it would give us more confidence that the minipal implementation is right. |
Called out as stretch goal in #73246 (comment). Not looking for stretch goals right now. We can leave the issue open. |
Co-authored-by: Jan Kotas <[email protected]>
@dotnet/ilc-contrib this is ready for review now |
Have you done some of this annoying manual testing? LGTM modulo comments. |
I'll do this once this is ready to merge - I don't want to do that more than once. I wish I could test this in the CI but all of our ARM64 builds (where this is interesting) are crossbuilds. I'm still considering making a managed test (that runs on top of CoreCLR) that would p/invoke into jitinterface.dll that we'd package with the test and compare returned flags with what we detect in C#. But not sure if it's worth the hassle. |
It appears to be working to the extent I could test. I suggest we get this in and more people can give it a try. I don't have e.g. ARM64 mac so it's really not possible for me to test this to the extent that would be necessary. |
I think we would get most of the regression protection by sharing the code with CoreCLR. (It can be done as follow up.)
Sounds good. |
Mono interp failure is known according to build analysis and this PR doesn't affect Mono legs. Merging for a follow up. |
This allows compiling for the ISA extensions that the currently running CPU supports.
This should work for both crossgen2 and ILC.
I didn't test anything but x64 Windows but since this will involve a ton of annoying manual testing, sending for review now.
Fixes #73246.
Cc @dotnet/ilc-contrib