Avoid considering types boxed in the scanner#118529
Avoid considering types boxed in the scanner#118529MichalStrehovsky wants to merge 1 commit intodotnet:mainfrom
Conversation
I'm not completely sure this is a fine assumption to make (we're assuming something that RyuJIT should do). Wanted to collect numbers if this is worth pursuing. Change salvaged out of dotnet#118479, rt-sz will decide if we want to pursue it.
There was a problem hiding this comment.
Pull Request Overview
This change modifies the IL scanner's box instruction handling to avoid considering certain types as boxed, relying on RyuJIT to eliminate trivial box operations. The change expands the scope of box operation validation from only byref-like types to all non-runtime-determined types, while maintaining the existing validation logic for byref-like types.
Key changes:
- Expands box operation analysis to all non-runtime-determined types
- Restructures the validation logic to check for trivial elimination patterns first, then validate byref-like type restrictions
| if (type.IsByRefLike) | ||
| ThrowHelper.ThrowInvalidProgramException(); | ||
| } | ||
|
|
There was a problem hiding this comment.
The ThrowInvalidProgramException call is now only reachable when type.IsByRefLike is true, but it's placed outside the if block. This will cause the exception to be thrown for all byref-like types, even those that should be valid according to the preceding validation logic. The exception should be moved inside the if block or the logic should be restructured.
| } | |
| if (type.IsByRefLike) | |
| ThrowHelper.ThrowInvalidProgramException(); |
|
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas |
| } | ||
|
|
||
| ThrowHelper.ThrowInvalidProgramException(); | ||
| // If this is a byref-like type, only the above operations are permitted. |
There was a problem hiding this comment.
This works only if we recognize all patterns that are recognized by the JIT. I see RyuJIT recognizing more patterns than what we do here - e.g. something with LDNULL:
runtime/src/coreclr/jit/importer.cpp
Line 3216 in f23d779
|
Rt-sz says this is not meaningful so it doesn't look worth pursuing. |
I'm not completely sure this is a fine assumption to make (we're assuming something that RyuJIT should do). Wanted to collect numbers if this is worth pursuing.
Change salvaged out of #118479, rt-sz will decide if we want to pursue it.