[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 1 commit 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
| @@ -611,30 +611,20 @@ public static bool ShouldCodeNotBeCompiledIntoFinalImage(InstructionSetSupport i | |||
|
|
|||
| if (compExactlyDependsOnList != null && compExactlyDependsOnList.Count > 0) | |||
| { | |||
| // Default to true, and set to false if at least one of the types is actually supported in the current environment, and none of the | |||
| // intrinsic types are in an opportunistic state. | |||
| bool doBypass = true; | |||
There was a problem hiding this comment.
This variable seems confusing to me and it was probably meant to serve a purpose that I don't really understand. Also the ILLEGAL case was deliberately skipped, while it feels it should have set doBypass to false. @davidwrighton Could you validate that I'm not missing something obvious here ?
|
@BrzVlad It took a bit of time for the rationale here to percolate to the top of my memory. The issue is that |
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.