Help JIT elide redundant CharToHexLookup range check#124702
Help JIT elide redundant CharToHexLookup range check#124702xtqqczze wants to merge 1 commit intodotnet:mainfrom
Conversation
|
Tagging subscribers to this area: @dotnet/area-meta |
|
@xtqqczze I'm not convinced those diffs are meaningful. They're just PMI artifacts where we prejit all methods, even with AggressiveInlining. So most of your diffs are just [MethodImpl(MethodImplOptions.AggressiveInlining)]
public static int FromChar(int c)
{
return (c >= CharToHexLookup.Length) ? 0xFF : CharToHexLookup[c];
}
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static int FromUpperChar(int c)
{
return (c > 71) ? 0xFF : CharToHexLookup[c];
}here JIT does emit a bound check because noone checks Also, you can add a Debug.Assert if you want |
|
@EgorBo I was already thinking about |
|
The problem that you effectively change the behavior here. The invalid code that used to throw Invalid range exception now silently uses 0xFF |
|
When these methods are used as intended, with values that are clearly non-negative, there is no change in behavior, which explains the absence of meaningful diffs. As such, this change is only expected to improve code generation for projects targeting .NET Framework. @EgorBo As you suspected, more meaningful diffs can be seen by adding |
Diffs: MihuBot/runtime-utils#1776