Skip to content

[r2r] Fix rule for skipping compilation of methods with CompExactlyDependsOn#125372

Open
BrzVlad wants to merge 1 commit intodotnet:mainfrom
BrzVlad:fix-r2r-comp-depends-on
Open

[r2r] Fix rule for skipping compilation of methods with CompExactlyDependsOn#125372
BrzVlad wants to merge 1 commit intodotnet:mainfrom
BrzVlad:fix-r2r-comp-depends-on

Conversation

@BrzVlad
Copy link
Member

@BrzVlad BrzVlad commented Mar 10, 2026

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. 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.

…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.
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 = true default variable and the return doBypass statement 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;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants