Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JIT] X64/ARM64 - Fold 'x & 255' and 'x & 65535' to a cast #79630

Merged
merged 9 commits into from
Jan 5, 2023
Merged

[JIT] X64/ARM64 - Fold 'x & 255' and 'x & 65535' to a cast #79630

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7d0eff7
Fold 'x & 256' and 'x & 65535'
TIHan Dec 14, 2022
d15e980
Remove comments
TIHan Dec 14, 2022
83a19b0
Handle more cases
TIHan Dec 14, 2022
aa443fc
Minor change
TIHan Dec 14, 2022
9340fdd
Put header back
TIHan Dec 14, 2022
ea1fbac
Some refactoring
TIHan Dec 14, 2022
4b65624
Minor change
TIHan Dec 14, 2022
c83518a
Minor change
TIHan Dec 14, 2022
ffcfb31
Feedback
TIHan Jan 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions src/coreclr/jit/gentree.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13589,6 +13589,30 @@ GenTree* Compiler::gtFoldExprSpecial(GenTree* tree)
return icon;
};

auto NewZeroExtendNode = [&](var_types type, GenTree* op1, var_types castToType) -> GenTree* {
assert(varTypeIsIntegral(type));
assert(!varTypeIsSmall(type));
assert(!varTypeIsUnsigned(type));
assert(varTypeIsUnsigned(castToType));

GenTreeCast* cast = gtNewCastNode(TYP_INT, op1, false, castToType);
if (fgGlobalMorph)
Copy link
Member

Choose a reason for hiding this comment

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

can you educate me on why fgMorphTreeDone is needed here (I don't see it in other morph cases) and does it decrease jit-diffs if you just do this opt in GlobalMorph case only?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm following the precedent that was set. Right above NewZeroExtendNode, you see:

    // Helper function that creates a new IntCon node and morphs it, if required
    auto NewMorphedIntConNode = [&](int value) -> GenTreeIntCon* {
        GenTreeIntCon* icon = gtNewIconNode(value);
        if (fgGlobalMorph)
        {
            fgMorphTreeDone(icon);
        }
        return icon;
    };

Also, gtFoldExprSpecial has a comment that says:

// Return value:
//   Tree (possibly modified at root or below), or a new tree
//   Any new tree is fully morphed, if necessary

{
fgMorphTreeDone(cast);
}

if (varTypeIsLong(type))
{
cast = gtNewCastNode(TYP_LONG, cast, true, TYP_ULONG);
TIHan marked this conversation as resolved.
Show resolved Hide resolved
if (fgGlobalMorph)
{
fgMorphTreeDone(cast);
}
}

return cast;
};

// Here `op` is the non-constant operand, `cons` is the constant operand
// and `val` is the constant value.

Expand Down Expand Up @@ -13745,6 +13769,21 @@ GenTree* Compiler::gtFoldExprSpecial(GenTree* tree)
goto DONE_FOLD;
}
}
else if (val == 0xFF)
{
op = NewZeroExtendNode(tree->TypeGet(), op, TYP_UBYTE);
goto DONE_FOLD;
}
else if (val == 0xFFFF)
{
op = NewZeroExtendNode(tree->TypeGet(), op, TYP_USHORT);
goto DONE_FOLD;
}
else if ((val == 0xFFFFFFFF) && varTypeIsLong(tree))
{
op = NewZeroExtendNode(tree->TypeGet(), op, TYP_UINT);
goto DONE_FOLD;
}
else
{
/* The GTF_BOOLEAN flag is set for nodes that are part
Expand Down
66 changes: 66 additions & 0 deletions src/tests/JIT/opt/Remainder/IntRemainder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ namespace CodeGenTests
static class IntRemainder
{
static int _fieldValue = 123;
static uint _fieldValueUnsigned = 123;

[MethodImpl(MethodImplOptions.NoInlining)]
static int Int32_RemainderByOne()
Expand All @@ -32,6 +33,56 @@ static int Int32_RemainderByOneWithValue(int value)
return value % 1;
}

[MethodImpl(MethodImplOptions.NoInlining)]
static byte Byte_RemainderByMaxValuePlusOne(uint value)
{
// X64-NOT: and {{[a-z]+}}

// X64: movzx {{[a-z]+}}, {{[a-z]+}}

return (byte)(value % (Byte.MaxValue + 1));
}

[MethodImpl(MethodImplOptions.NoInlining)]
static ushort UInt16_RemainderByMaxValuePlusOne(uint value)
{
// X64-NOT: and {{[a-z]+}}

// X64: movzx {{[a-z]+}}, {{[a-z]+}}

return (ushort)(value % (UInt16.MaxValue + 1));
}

[MethodImpl(MethodImplOptions.NoInlining)]
static uint Byte_RemainderByMaxValuePlusOne_Return_UInt32(uint value)
{
// X64-NOT: and {{[a-z]+}}

// X64: movzx {{[a-z]+}}, {{[a-z]+}}

return (value % (Byte.MaxValue + 1));
}

[MethodImpl(MethodImplOptions.NoInlining)]
static uint UInt16_RemainderByMaxValuePlusOne_Return_UInt32(uint value)
{
// X64-NOT: and {{[a-z]+}}

// X64: movzx {{[a-z]+}}, {{[a-z]+}}

return (value % (UInt16.MaxValue + 1));
}

[MethodImpl(MethodImplOptions.NoInlining)]
static byte Byte_RemainderByMaxValuePlusOne_WithField()
{
// X64-NOT: and {{[a-z]+}}

// X64: movzx {{[a-z]+}}, {{[a-z]+}}

return (byte)(_fieldValueUnsigned % (Byte.MaxValue + 1));
}

static int Main()
{
if (Int32_RemainderByOne() != 0)
Expand All @@ -40,6 +91,21 @@ static int Main()
if (Int32_RemainderByOneWithValue(-123) != 0)
return 0;

if (Byte_RemainderByMaxValuePlusOne(68000) != 160)
return 0;

if (UInt16_RemainderByMaxValuePlusOne(68000) != 2464)
return 0;

if (Byte_RemainderByMaxValuePlusOne_Return_UInt32(68000) != 160)
return 0;

if (UInt16_RemainderByMaxValuePlusOne_Return_UInt32(68000) != 2464)
return 0;

if (Byte_RemainderByMaxValuePlusOne_WithField() != 123)
return 0;

return 100;
}
}
Expand Down