[r2r] Fix rule for skipping compilation of methods with CompExactlyDependsOn#125372
[r2r] Fix rule for skipping compilation of methods with CompExactlyDependsOn#125372BrzVlad wants to merge 2 commits intodotnet:mainfrom
Conversation
…pendsOn
If a method has `CompExactlyDependsOn` attribute, it means the correctness of its code depends on matching support for the instruction set between r2r compilation time and run time jit compilation.
For the example of ShuffleNativeModified:
```
[CompExactlyDependsOn(typeof(Ssse3))]
internal static Vector128<byte> ShuffleNativeModified(Vector128<byte> vector, Vector128<byte> indices)
{
if (Ssse3.IsSupported)
return Ssse3.Shuffle(vector, indices);
return Vector128.Shuffle(vector, indices);
}
```
Ssse3 is an intel specific instruction set that is never used on arm64. The check in code was returning an ILLEGAL instruction set. This case was treated as `continue` instead of setting `doBypass` to false. Because of this, this method was skipped from r2r compilation. The fix avoids interpreting this method on ios-arm64, making JSON serialization/deserialization 2x faster.
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in the ReadyToRun (R2R) compiler logic that incorrectly excluded methods from compilation when all of their CompExactlyDependsOn attributes referenced instruction sets not applicable to the target architecture (i.e., returning InstructionSet.ILLEGAL).
Changes:
- The
doBypass = truedefault variable and thereturn doBypassstatement are removed, eliminating the incorrect early-exit path - ILLEGAL instruction sets are now treated as a known compile-time state (never supported on the target), so they no longer trigger the bypass heuristic
- Only instruction sets in an undetermined/opportunistic state (neither explicitly supported nor explicitly unsupported) still cause the method to be skipped from R2R compilation
src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs
Show resolved
Hide resolved
|
@BrzVlad It took a bit of time for the rationale here to percolate to the top of my memory. The issue is that |
|
Thanks, this brings some new insight into this attribute. Seems to me that this new attribute argument would pretty much indicate whether the method in question has a fallback that is not dependent on some particular instruction set. It might seem a bit confusing to have this argument duplicated on every single Also, this raises a new confusion. So |
|
@BrzVlad the attribute primarily serves the purpose of supporting the custom model for intrinsics in System.Private.CoreLib. The normal rules are that R2R compilation will compile code for intrinsics which are specified as guaranteed to be supported, and code which refers on other intrinsics which may/may not be supported is skipped. Where this gets messy is where a function depends on say... SSE2, and AVX, and switches between code paths based on the IsSupported apis. What will happen is that the jit compiler will see... Ah, you want to use SSE2. We'll record that you need that. AND it will see that OH, you used AVX. Well, the default compilation rules say that AVX isn't available, so we'll record that AVX must not be available if you want to use this code. Then we end up in the situation where we run the code on an AVX machine (which is the common case), and then the R2R generated code can't be used. To work around that we have a special rule for System.Private.CoreLib, which is that we don't generate records that indicate that AVX isn't supported, and just say... use the code anyways since we know that all code in System.Private.CoreLib should behave the same regardless of which intrinsics are enabled BUT this gets us into trouble if we generate code for an architecture specific helper that doesn't get inlined into the code path which checks IsSupported. For instance if we generate code for an hypothetical GetIndexOfFirstNonLatin1Char_AVX in R2R, it will just be a bunch of logic which throws exceptions into the R2R image, and we don't want to bring that code into a process that actually supports AVX. So we have this weird attribute + an analyzer that does a very strange interprocedural analysis. The weird rule also pops up where there are subtle variations in behavior around certain intrinsics. For example this Shuffle intrinsic. I expect that the behavior of Ssse3.Shuffle is subtly different from Vector128.Shuffle, so we need to be super careful to pick the exactly correct one and use it consistently through the process. Obviously that doesn't matter much for Arm64, as it will always use the Vector128 shuffle, but we didn't really have a meaningful Arm64 test suite when I wrote all of this, and I probably missed some cases around the correct behavior for skipping compilation. |
|
All of that said, the basic treatment we need is to make your change, and investigate on various platforms (Windows X64, Linux Arm64, Apple Arm64) which methods are added/removed from System.Private.CoreLib, and make sure that the adjustment there is as expected. I'd prefer to have some data to see the effects. |
…r CompExactlyDependsOn instruction sets Unless they are decorated with the CompHasFallback attribute.
6444e3e to
a90d822
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
You can also share your feedback on Copilot code review. Take the survey.
...aries/System.Private.CoreLib/src/System/Runtime/CompilerServices/CompHasFallbackAttribute.cs
Show resolved
Hide resolved
| bool anySupported = false; | ||
|
|
||
| foreach (var intrinsicType in compExactlyDependsOnList) | ||
| { | ||
| InstructionSet instructionSet = InstructionSetParser.LookupPlatformIntrinsicInstructionSet(intrinsicType.Context.Target.Architecture, intrinsicType); | ||
| if (instructionSet == InstructionSet.ILLEGAL) | ||
| { | ||
| // This instruction set isn't supported on the current platform at all. | ||
| continue; | ||
| } | ||
| if (instructionSetSupport.IsInstructionSetSupported(instructionSet) || instructionSetSupport.IsInstructionSetExplicitlyUnsupported(instructionSet)) | ||
| { | ||
| doBypass = false; | ||
| } | ||
| else | ||
| // If the instruction set is ILLEGAL, it means it is never supported by the current architecture so the behavior at runtime is known | ||
| if (instructionSet != InstructionSet.ILLEGAL) | ||
| { | ||
| // If we reach here this is an instruction set generally supported on this platform, but we don't know what the behavior will be at runtime | ||
| return true; | ||
| if (instructionSetSupport.IsInstructionSetSupported(instructionSet)) | ||
| { | ||
| anySupported = true; | ||
| } | ||
| else if (!instructionSetSupport.IsInstructionSetExplicitlyUnsupported(instructionSet)) | ||
| { | ||
| // If we reach here this is an instruction set generally supported on this platform, but we don't know what the behavior will be at runtime | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return doBypass; | ||
| if (!anySupported && !hasCompHasFallback) | ||
| { | ||
| // If none of the instruction sets are supported (all are either illegal or explicitly unsupported), | ||
| // skip compilation unless the method has a functional fallback path | ||
| return true; | ||
| } |
...aries/System.Private.CoreLib/src/System/Runtime/CompilerServices/CompHasFallbackAttribute.cs
Show resolved
Hide resolved
|
@davidwrighton On osx-arm64, linux-arm64 and win-x64 I've built corerun and then used crossgen2 on SPC.dll by using the rsp file that is used for r2r compilation when you do The result was that there is no difference in the set of compiled methods, except for the methods that have the new attribute linux-arm64-fix.txt |
If a method has
CompExactlyDependsOnattribute, it means the correctness of its code depends on matching support for the instruction set between r2r compilation time and run time jit compilation.For the example of ShuffleNativeModified:
Ssse3 is an intel specific instruction set that is never used on arm64. The check in code was returning an ILLEGAL instruction set. This case was treated as
continueinstead of settingdoBypassto false. Because of this, this method was skipped from r2r compilation. We now treat the ILLEGAL case as known compile time support, so we don't skip the method from r2r compilation. The fix avoids interpreting this method on ios-arm64, making JSON serialization/deserialization 2x faster.