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.

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] - Fixed sub-optimal optimization for a % 1 to Zero #77760

Merged
merged 25 commits into from
Dec 8, 2022
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
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
1 change: 1 addition & 0 deletions src/coreclr/jit/compiler.h
Original file line number Diff line number Diff line change
Expand Up @@ -5797,6 +5797,7 @@ class Compiler
GenTree* fgOptimizeBitwiseXor(GenTreeOp* xorOp);
GenTree* fgPropagateCommaThrow(GenTree* parent, GenTreeOp* commaThrow, GenTreeFlags precedingSideEffects);
GenTree* fgMorphRetInd(GenTreeUnOp* tree);
GenTree* fgMorphModToZero(GenTreeOp* tree);
GenTree* fgMorphModToSubMulDiv(GenTreeOp* tree);
GenTree* fgMorphUModToAndSub(GenTreeOp* tree);
GenTree* fgMorphSmpOpOptional(GenTreeOp* tree, bool* optAssertionPropDone);
Expand Down
89 changes: 61 additions & 28 deletions src/coreclr/jit/morph.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9869,20 +9869,13 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac, bool* optA

ASSIGN_HELPER_FOR_MOD:

// For "val % 1", return 0 if op1 doesn't have any side effects
// and we are not in the CSE phase, we cannot discard 'tree'
// because it may contain CSE expressions that we haven't yet examined.
//
if (((op1->gtFlags & GTF_SIDE_EFFECT) == 0) && !optValnumCSE_phase)
if (!optValnumCSE_phase)
{
if (op2->IsIntegralConst(1))
if (tree->OperIs(GT_MOD, GT_UMOD) && (op2->IsIntegralConst(1) || op2->IsIntegralConst(-1)))
TIHan marked this conversation as resolved.
Show resolved Hide resolved
{
GenTree* zeroNode = gtNewZeroConNode(typ);
#ifdef DEBUG
zeroNode->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED;
#endif
DEBUG_DESTROY_NODE(tree);
return zeroNode;
// Transformation: a % 1 = 0
// Transformation: a % -1 = 0
return fgMorphModToZero(tree->AsOp());
}
}

Expand All @@ -9909,20 +9902,17 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac, bool* optA
}
#endif
#endif // !TARGET_64BIT

if (!optValnumCSE_phase)
{
if (tree->OperIs(GT_UMOD) && op2->IsIntegralConstUnsignedPow2())
{
// Transformation: a % b = a & (b - 1);
tree = fgMorphUModToAndSub(tree->AsOp());
op1 = tree->AsOp()->gtOp1;
op2 = tree->AsOp()->gtOp2;
return fgMorphUModToAndSub(tree->AsOp());
}
#ifdef TARGET_ARM64
// ARM64 architecture manual suggests this transformation
// for the mod operator.
else
else if (tree->OperIs(GT_MOD, GT_UMOD))
#else
// XARCH only applies this transformation if we know
// that magic division will be used - which is determined
Expand All @@ -9933,9 +9923,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac, bool* optA
#endif
{
// Transformation: a % b = a - (a / b) * b;
tree = fgMorphModToSubMulDiv(tree->AsOp());
op1 = tree->AsOp()->gtOp1;
op2 = tree->AsOp()->gtOp2;
return fgMorphModToSubMulDiv(tree->AsOp());
}
}
break;
Expand Down Expand Up @@ -13160,6 +13148,57 @@ GenTree* Compiler::fgMorphMultiOp(GenTreeMultiOp* multiOp)
}
#endif // defined(FEATURE_SIMD) || defined(FEATURE_HW_INTRINSICS)

//------------------------------------------------------------------------
// fgMorphModToZero: Transform 'a % 1' or 'a % -1' into the equivalent '0'.
//
// Arguments:
// tree - The GT_MOD/GT_UMOD tree to morph
//
// Returns:
// The morphed tree, will be a GT_COMMA or a zero constant node.
//
GenTree* Compiler::fgMorphModToZero(GenTreeOp* tree)
{
JITDUMP("\nMorphing MOD/UMOD [%06u] to Zero\n", dspTreeID(tree));

assert(tree->OperIs(GT_MOD, GT_UMOD));
assert(tree->gtOp2->IsIntegralConst(1) || tree->gtOp2->IsIntegralConst(-1));

GenTree* op1 = tree->gtGetOp1();
GenTree* op2 = tree->gtGetOp2();

const var_types type = tree->TypeGet();

GenTree* zero = gtNewZeroConNode(type);
TIHan marked this conversation as resolved.
Show resolved Hide resolved

GenTree* op1SideEffects = nullptr;
gtExtractSideEffList(op1, &op1SideEffects, GTF_ALL_EFFECT);
if (op1SideEffects != nullptr)
{
GenTree* comma = gtNewOperNode(GT_COMMA, type, op1SideEffects, zero);

#ifdef DEBUG
// op1 may already have been morphed, so unset this bit.
op1SideEffects->gtDebugFlags &= ~GTF_DEBUG_NODE_MORPHED;
TIHan marked this conversation as resolved.
Show resolved Hide resolved
#endif // DEBUG

DEBUG_DESTROY_NODE(tree->gtOp2);
DEBUG_DESTROY_NODE(tree);

return fgMorphTree(comma);
}
else
{
INDEBUG(zero->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED);

DEBUG_DESTROY_NODE(tree->gtOp2);
DEBUG_DESTROY_NODE(tree->gtOp1);
DEBUG_DESTROY_NODE(tree);

return zero;
}
}

//------------------------------------------------------------------------
// fgMorphModToSubMulDiv: Transform a % b into the equivalent a - (a / b) * b
// (see ECMA III 3.55 and III.3.56).
Expand Down Expand Up @@ -13282,13 +13321,9 @@ GenTree* Compiler::fgMorphModToSubMulDiv(GenTreeOp* tree)
result = gtNewOperNode(GT_COMMA, type, tempInfos[i].asg, result);
}

#ifdef DEBUG
result->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED;
#endif

div->CheckDivideByConstOptimized(this);

return result;
return fgMorphTree(result);
}

//------------------------------------------------------------------------
Expand Down Expand Up @@ -13318,12 +13353,10 @@ GenTree* Compiler::fgMorphUModToAndSub(GenTreeOp* tree)
const size_t cnsValue = (static_cast<size_t>(tree->gtOp2->AsIntConCommon()->IntegralValue())) - 1;
GenTree* const newTree = gtNewOperNode(GT_AND, type, tree->gtOp1, gtNewIconNode(cnsValue, type));

INDEBUG(newTree->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED);

DEBUG_DESTROY_NODE(tree->gtOp2);
DEBUG_DESTROY_NODE(tree);

return newTree;
return fgMorphTree(newTree);
}

//------------------------------------------------------------------------------
Expand Down
74 changes: 74 additions & 0 deletions src/tests/JIT/opt/Remainder/IntRemainder.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Runtime.CompilerServices;

namespace CodeGenTests
{
static class IntRemainder
{
static int _fieldValue = 123;

[MethodImpl(MethodImplOptions.NoInlining)]
static int Int32_RemainderByOne()
{
// X64-FULL-LINE: call CORINFO_HELP_GETSHARED_NONGCSTATIC_BASE
// X64-FULL-LINE-NEXT: xor [[REG0:[a-z]+]], [[REG0]]

// ARM64-FULL-LINE: bl CORINFO_HELP_GETSHARED_NONGCSTATIC_BASE
// ARM64-FULL-LINE-NEXT: mov [[REG0:[a-z0-9]+]], wzr

return _fieldValue % 1;
}

[MethodImpl(MethodImplOptions.NoInlining)]
static int Int32_RemainderByOneWithValue(int value)
{
// X64-FULL-LINE: xor [[REG0:[a-z]+]], [[REG0]]

// ARM64-FULL-LINE: mov [[REG0:[a-z0-9]+]], wzr

return value % 1;
}

[MethodImpl(MethodImplOptions.NoInlining)]
static int Int32_RemainderByNegativeOne()
{
// X64-FULL-LINE: call CORINFO_HELP_GETSHARED_NONGCSTATIC_BASE
// X64-FULL-LINE-NEXT: xor [[REG0:[a-z]+]], [[REG0]]

// ARM64-FULL-LINE: bl CORINFO_HELP_GETSHARED_NONGCSTATIC_BASE
// ARM64-FULL-LINE-NEXT: mov [[REG0:[a-z0-9]+]], wzr

return _fieldValue % -1;
}

[MethodImpl(MethodImplOptions.NoInlining)]
static int Int32_RemainderByNegativeOneWithValue(int value)
{
// X64-FULL-LINE: xor [[REG0:[a-z]+]], [[REG0]]

// ARM64-FULL-LINE: mov [[REG0:[a-z0-9]+]], wzr

return value % -1;
}

static int Main()
{
if (Int32_RemainderByOne() != 0)
return 0;

if (Int32_RemainderByOneWithValue(-123) != 0)
return 0;

if (Int32_RemainderByNegativeOne() != 0)
return 0;

if (Int32_RemainderByNegativeOneWithValue(-123) != 0)
return 0;

return 100;
}
}
}
17 changes: 17 additions & 0 deletions src/tests/JIT/opt/Remainder/IntRemainder.csproj
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<OutputType>Exe</OutputType>
</PropertyGroup>
<PropertyGroup>
<DebugType>None</DebugType>
<Optimize>True</Optimize>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).cs">
<HasDisasmCheck>true</HasDisasmCheck>
</Compile>

<CLRTestEnvironmentVariable Include="DOTNET_TieredCompilation" Value="0" />
<CLRTestEnvironmentVariable Include="DOTNET_JITMinOpts" Value="0" />
Comment on lines +14 to +15
Copy link
Member

Choose a reason for hiding this comment

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

One thing I don't remember:

If we have <HasDisasmCheck>true<...>, why do we need to clear TieredCompilation/JITMinOpts? Shouldn't the "HasDisasmCheck" logic do that for us?

Copy link
Contributor Author

@TIHan TIHan Dec 8, 2022

Choose a reason for hiding this comment

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

<HasDisasmCheck>true<...> does not force any environment variables that affect codegen - so it is up to the test itself to decide them.

</ItemGroup>
</Project>
19 changes: 19 additions & 0 deletions src/tests/JIT/opt/Remainder/Regressions/Regression1/Regression1.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

public class Program
{
public static ulong[,] s_1;
public static int Main()
{
// This should not assert.
try
{
ushort vr10 = default(ushort);
bool vr11 = 0 < ((s_1[0, 0] * (uint)(0 / vr10)) % 1);
}
catch {}

return 100;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<OutputType>Exe</OutputType>
</PropertyGroup>
<PropertyGroup>
<DebugType>None</DebugType>
<Optimize>True</Optimize>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).cs" />
</ItemGroup>
</Project>