Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

JIT: Remove bound checks when index is Byte and array.Length >= 256 #25912

Closed
wants to merge 7 commits into from
Closed

JIT: Remove bound checks when index is Byte and array.Length >= 256 #25912

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b8c9f62
Remove range check if array is known to have 256 or more elements and…
EgorBo Jul 27, 2019
034d97e
Handle GT_CAST in ComputeRange
EgorBo Jul 27, 2019
1f15a4b
optimize mscorlib
EgorBo Jul 27, 2019
cb0e27b
use CastToType
EgorBo Jul 27, 2019
ceb75d4
formatting
EgorBo Jul 27, 2019
b9b88cb
remove padding from AsciiCharInfo
EgorBo Jul 31, 2019
1d68ae6
Handle TYP_USHORT
EgorBo Jul 31, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/System.Private.CoreLib/shared/System/Char.cs
View file
Open in desktop
Expand Up @@ -38,7 +38,7 @@ namespace System
public const char MinValue = (char)0x00;

// Unicode category values from Unicode U+0000 ~ U+00FF. Store them in byte[] array to save space.
private static ReadOnlySpan<byte> CategoryForLatin1 => new byte[] { // uses C# compiler's optimization for static byte[] data
private static ReadOnlySpan<byte> CategoryForLatin1 => new byte[256] { // uses C# compiler's optimization for static byte[] data
(byte)UnicodeCategory.Control, (byte)UnicodeCategory.Control, (byte)UnicodeCategory.Control, (byte)UnicodeCategory.Control, (byte)UnicodeCategory.Control, (byte)UnicodeCategory.Control, (byte)UnicodeCategory.Control, (byte)UnicodeCategory.Control, // 0000 - 0007
(byte)UnicodeCategory.Control, (byte)UnicodeCategory.Control, (byte)UnicodeCategory.Control, (byte)UnicodeCategory.Control, (byte)UnicodeCategory.Control, (byte)UnicodeCategory.Control, (byte)UnicodeCategory.Control, (byte)UnicodeCategory.Control, // 0008 - 000F
(byte)UnicodeCategory.Control, (byte)UnicodeCategory.Control, (byte)UnicodeCategory.Control, (byte)UnicodeCategory.Control, (byte)UnicodeCategory.Control, (byte)UnicodeCategory.Control, (byte)UnicodeCategory.Control, (byte)UnicodeCategory.Control, // 0010 - 0017
Expand Down Expand Up @@ -89,7 +89,7 @@ private static bool IsAscii(char ch)
private static UnicodeCategory GetLatin1UnicodeCategory(char ch)
{
Debug.Assert(IsLatin1(ch), "char.GetLatin1UnicodeCategory(): ch should be <= 007f");
Copy link
Member

Choose a reason for hiding this comment

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

This comment is wrong

return (UnicodeCategory)CategoryForLatin1[(int)ch];
return (UnicodeCategory)CategoryForLatin1[(byte)ch];
EgorBo marked this conversation as resolved.
Show resolved Hide resolved
}

//
Expand Down
15 changes: 13 additions & 2 deletions src/jit/rangecheck.cpp
View file
Open in desktop
Expand Up @@ -1050,6 +1050,15 @@ bool RangeCheck::ComputeDoesOverflow(BasicBlock* block, GenTree* expr)
{
overflows = DoesPhiOverflow(block, expr);
}
else if (expr->OperGet() == GT_CAST)
Copy link

Choose a reason for hiding this comment

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

It sounds like RangeCheck::ComputeDoesOverflow is very conservative, it returns true by default and supports only GT_ADD as binary operation. It doesn't pay attention to gtFlags & GTF_OVERFLOW flag that I expected here.

@dotnet/jit-contrib do we need an issue/task to rewrite this function to make it work for all opcodes and clarify its relationships with GTF_OVERFLOW?

{
GenTreeCast* castExpr = expr->AsCast();
if (castExpr->CastToType() == TYP_UBYTE && castExpr->CastFromType() == TYP_INT)
EgorBo marked this conversation as resolved.
Show resolved Hide resolved
{
overflows = false;
}
}

GetOverflowMap()->Set(expr, overflows, OverflowMap::Overwrite);
m_pSearchPath->Remove(expr);
return overflows;
Expand Down Expand Up @@ -1144,9 +1153,11 @@ Range RangeCheck::ComputeRange(BasicBlock* block, GenTree* expr, bool monotonic
JITDUMP("%s\n", range.ToString(m_pCompiler->getAllocatorDebugOnly()));
}
}
else if (varTypeIsSmallInt(expr->TypeGet()))
else if (expr->OperIs(GT_CAST) || varTypeIsSmallInt(expr->TypeGet()))
Copy link

Choose a reason for hiding this comment

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

It looks unnatural for me to have a special case for the cast here, could you please extract the code below into Range ComputeRageForSmallInt(var_types type) and call it here and from a new else if (expr->OperIs(GT_CAST))?

Note: right now we hit this case only for IND and COMMA that are based on these indir nodes, e.g.:

N002 (  4,  3) [000725] *--X---N----              *  IND       ushort <l:$4c1, c:$4c0>
N001 (  1,  1) [000037] ------------              \--*  LCL_VAR   byref  V23 tmp12        u:1 <l:$140, c:$180>

N007 (  7,  7) [000248] ---XG--N----              *  COMMA     ubyte  <l:$41b, c:$41a>
N001 (  0,  0) [000505] ------------              +--*  NOP       void  
N006 (  7,  7) [000433] *--XG-------              \--*  IND       ubyte  <l:$419, c:$418>
N005 (  3,  4) [000247] ----G--N----                 \--*  ADD       byref  <l:$1cc, c:$1cb>
N002 (  1,  1) [000246] ------------                    +--*  LCL_VAR   byref  V13 tmp3         u:1 <l:$100, c:$140>
N004 (  2,  3) [000243] ------------                    \--*  CAST      long <- int $448
N003 (  1,  1) [000238] ------------                       \--*  LCL_VAR   int    V07 loc4         u:2 $206

@dotnet/jit-contrib does anybody know any other nodes that could have varTypeIsSmallInt(expr->TypeGet())?
I expected GT_CAST to have them, but as I was told for GT_CAST TypeGet() returns castFrom type and it can't be less then INT, does it sound right?

{
switch (expr->TypeGet())
var_types exprType = expr->OperIs(GT_CAST) ? expr->AsCast()->CastToType() : expr->TypeGet();

switch (exprType)
{
case TYP_UBYTE:
range = Range(Limit(Limit::keConstant, 0), Limit(Limit::keConstant, 255));
Expand Down