Skip to content

Commit

Permalink
JIT: extend RBO partial inference to unsigned relops (#75804)
Browse files Browse the repository at this point in the history
RBO can now partially infer from a pair of unsigned relops or an unsigned
relop and an equality relop.

Fixes #65327.
  • Loading branch information
AndyAyersMS authored Sep 19, 2022
1 parent e8d32c1 commit 78528df
Show file tree
Hide file tree
Showing 5 changed files with 160 additions and 56 deletions.
75 changes: 51 additions & 24 deletions src/coreclr/jit/redundantbranchopts.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -143,45 +143,74 @@ struct RelopImplicationRule
//
// clang-format off
//
#define V(x) (VNFunc)GT_##x
#define V(x) (VNFunc)GT_ ## x
#define U(x) VNF_ ## x ## _UN

static const RelopImplicationRule s_implicationRules[] =
{
// EQ
{V(EQ), true, false, V(GE), false},
{V(EQ), true, false, V(LE), false},
{V(EQ), true, false, V(GT), true},
{V(EQ), true, false, U(GT), true},
{V(EQ), true, false, V(LT), true},
{V(EQ), true, false, U(LT), true},

// NE
{V(NE), false, true, V(GE), true},
{V(NE), false, true, V(LE), true},
{V(NE), false, true, V(GT), false},
{V(NE), false, true, U(GT), false},
{V(NE), false, true, V(LT), false},
{V(NE), false, true, U(LT), false},

// LE
{V(LE), false, true, V(EQ), false},
{V(LE), false, true, V(NE), true},
{V(LE), false, true, V(GE), true},
{V(LE), false, true, V(LT), false},

// LE_UN
{U(LE), false, true, V(EQ), false},
{U(LE), false, true, V(NE), true},
{U(LE), false, true, U(GE), true},
{U(LE), false, true, U(LT), false},

// GT
{V(GT), true, false, V(EQ), true},
{V(GT), true, false, V(NE), false},
{V(GT), true, false, V(GE), false},
{V(GT), true, false, V(LT), true},

// GT_UN
{U(GT), true, false, V(EQ), true},
{U(GT), true, false, V(NE), false},
{U(GT), true, false, U(GE), false},
{U(GT), true, false, U(LT), true},

// GE
{V(GE), false, true, V(EQ), false},
{V(GE), false, true, V(NE), true},
{V(GE), false, true, V(LE), true},
{V(GE), false, true, V(GT), false},

// GE_UN
{U(GE), false, true, V(EQ), false},
{U(GE), false, true, V(NE), true},
{U(GE), false, true, U(LE), true},
{U(GE), false, true, U(GT), false},

// LT
{V(LT), true, false, V(EQ), true},
{V(LT), true, false, V(NE), false},
{V(LT), true, false, V(LE), false},
{V(LT), true, false, V(GT), true},

// LT_UN
{U(LT), true, false, V(EQ), true},
{U(LT), true, false, V(NE), false},
{U(LT), true, false, U(LE), false},
{U(LT), true, false, U(GT), true},
};
// clang-format on

Expand Down Expand Up @@ -237,17 +266,15 @@ void Compiler::optRelopImpliesRelop(RelopImplicationInfo* rii)
// VNs are not directly related. See if dominating
// compare encompasses a related VN.
//
VNFuncApp funcApp;
if (!vnStore->GetVNFunc(rii->domCmpNormVN, &funcApp))
VNFuncApp domApp;
if (!vnStore->GetVNFunc(rii->domCmpNormVN, &domApp))
{
return;
}

genTreeOps const oper = genTreeOps(funcApp.m_func);

// Exclude floating point relops.
//
if (varTypeIsFloating(vnStore->TypeOfVN(funcApp.m_args[0])))
if (varTypeIsFloating(vnStore->TypeOfVN(domApp.m_args[0])))
{
return;
}
Expand All @@ -261,39 +288,38 @@ void Compiler::optRelopImpliesRelop(RelopImplicationInfo* rii)
const bool inRange = true;
#endif

// Dominating compare has the form R(x,y)
// See if tree compare has the form R*(x,y) or R*(y,x) where we can infer R* from R
//
// Could also extend to the unsigned VN relops.
// If the dominating compare has the form R(x,y), see if tree compare has the
// form R*(x,y) or R*(y,x) where we can infer R* from R.
//
VNFuncApp treeApp;
if (inRange && GenTree::OperIsCompare(oper) && vnStore->GetVNFunc(rii->treeNormVN, &treeApp))
VNFunc const domFunc = domApp.m_func;
VNFuncApp treeApp;
if (inRange && ValueNumStore::VNFuncIsComparison(domFunc) && vnStore->GetVNFunc(rii->treeNormVN, &treeApp))
{
genTreeOps const treeOper = genTreeOps(treeApp.m_func);
genTreeOps domOper = oper;

if (((treeApp.m_args[0] == funcApp.m_args[0]) && (treeApp.m_args[1] == funcApp.m_args[1])) ||
((treeApp.m_args[0] == funcApp.m_args[1]) && (treeApp.m_args[1] == funcApp.m_args[0])))
if (((treeApp.m_args[0] == domApp.m_args[0]) && (treeApp.m_args[1] == domApp.m_args[1])) ||
((treeApp.m_args[0] == domApp.m_args[1]) && (treeApp.m_args[1] == domApp.m_args[0])))
{
const bool swapped = (treeApp.m_args[0] == funcApp.m_args[1]);
const bool swapped = (treeApp.m_args[0] == domApp.m_args[1]);

VNFunc const treeFunc = treeApp.m_func;
VNFunc domFunc1 = domFunc;

if (swapped)
{
domOper = GenTree::SwapRelop(domOper);
domFunc1 = ValueNumStore::SwapRelop(domFunc);
}

for (const RelopImplicationRule& rule : s_implicationRules)
{
if ((rule.domRelop == (VNFunc)domOper) && (rule.treeRelop == (VNFunc)treeOper))
if ((rule.domRelop == domFunc1) && (rule.treeRelop == treeFunc))
{
rii->canInfer = true;
rii->vnRelation = ValueNumStore::VN_RELATION_KIND::VRK_Inferred;
rii->canInferFromTrue = rule.canInferFromTrue;
rii->canInferFromFalse = rule.canInferFromFalse;
rii->reverseSense = rule.reverse;

JITDUMP("Can infer %s from [%s] %s\n", GenTree::OpName(treeOper),
rii->canInferFromTrue ? "true" : "false", GenTree::OpName(oper));
JITDUMP("Can infer %s from [%s] dominating %s\n", ValueNumStore::VNFuncName(treeFunc),
rii->canInferFromTrue ? "true" : "false", ValueNumStore::VNFuncName(domFunc));
return;
}
}
Expand All @@ -305,17 +331,18 @@ void Compiler::optRelopImpliesRelop(RelopImplicationInfo* rii)
//
// Look for {EQ,NE}({AND,OR,NOT}, 0)
//
genTreeOps const oper = genTreeOps(domFunc);
if (!GenTree::StaticOperIs(oper, GT_EQ, GT_NE))
{
return;
}

if (funcApp.m_args[1] != vnStore->VNZeroForType(TYP_INT))
if (domApp.m_args[1] != vnStore->VNZeroForType(TYP_INT))
{
return;
}

const ValueNum predVN = funcApp.m_args[0];
const ValueNum predVN = domApp.m_args[0];
VNFuncApp predFuncApp;
if (!vnStore->GetVNFunc(predVN, &predFuncApp))
{
Expand Down
79 changes: 50 additions & 29 deletions src/coreclr/jit/valuenum.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5283,6 +5283,52 @@ bool ValueNumStore::IsVNHandle(ValueNum vn)
return c->m_attribs == CEA_Handle;
}

//------------------------------------------------------------------------
// SwapRelop: return VNFunc for swapped relop
//
// Arguments:
// vnf - vnf for original relop
//
// Returns:
// VNFunc for swapped relop, or VNF_MemOpaque if the original VNFunc
// was not a relop.
//
VNFunc ValueNumStore::SwapRelop(VNFunc vnf)
{
VNFunc swappedFunc = VNF_MemOpaque;
if (vnf >= VNF_Boundary)
{
switch (vnf)
{
case VNF_LT_UN:
swappedFunc = VNF_GT_UN;
break;
case VNF_LE_UN:
swappedFunc = VNF_GE_UN;
break;
case VNF_GE_UN:
swappedFunc = VNF_LE_UN;
break;
case VNF_GT_UN:
swappedFunc = VNF_LT_UN;
break;
default:
break;
}
}
else
{
const genTreeOps op = (genTreeOps)vnf;

if (GenTree::OperIsCompare(op))
{
swappedFunc = (VNFunc)GenTree::SwapRelop(op);
}
}

return swappedFunc;
}

//------------------------------------------------------------------------
// GetRelatedRelop: return value number for reversed/swapped comparison
//
Expand Down Expand Up @@ -5354,36 +5400,11 @@ ValueNum ValueNumStore::GetRelatedRelop(ValueNum vn, VN_RELATION_KIND vrk)
//
if (swap)
{
if (newFunc >= VNF_Boundary)
{
switch (newFunc)
{
case VNF_LT_UN:
newFunc = VNF_GT_UN;
break;
case VNF_LE_UN:
newFunc = VNF_GE_UN;
break;
case VNF_GE_UN:
newFunc = VNF_LE_UN;
break;
case VNF_GT_UN:
newFunc = VNF_LT_UN;
break;
default:
return NoVN;
}
}
else
{
const genTreeOps op = (genTreeOps)newFunc;
newFunc = SwapRelop(newFunc);

if (!GenTree::OperIsCompare(op))
{
return NoVN;
}

newFunc = (VNFunc)GenTree::SwapRelop(op);
if (newFunc == VNF_MemOpaque)
{
return NoVN;
}
}

Expand Down
10 changes: 7 additions & 3 deletions src/coreclr/jit/valuenum.h
Original file line number Diff line number Diff line change
Expand Up @@ -304,9 +304,6 @@ class ValueNumStore
// (Requires InitValueNumStoreStatics to have been run.)
static bool VNFuncIsCommutative(VNFunc vnf);

// Returns "true" iff "vnf" is a comparison (and thus binary) operator.
static bool VNFuncIsComparison(VNFunc vnf);

bool VNEvalCanFoldBinaryFunc(var_types type, VNFunc func, ValueNum arg0VN, ValueNum arg1VN);

bool VNEvalCanFoldUnaryFunc(var_types type, VNFunc func, ValueNum arg0VN);
Expand Down Expand Up @@ -982,6 +979,13 @@ class ValueNumStore
//
ValueNum GetRelatedRelop(ValueNum vn, VN_RELATION_KIND vrk);

// Return VNFunc for swapped relop, or VNF_MemOpaque if the function
// is not a relop.
static VNFunc SwapRelop(VNFunc vnf);

// Returns "true" iff "vnf" is a comparison (and thus binary) operator.
static bool VNFuncIsComparison(VNFunc vnf);

// Convert a vartype_t to the value number's storage type for that vartype_t.
// For example, ValueNum of type TYP_LONG are stored in a map of INT64 variables.
// Lang is the language (C++) type for the corresponding vartype_t.
Expand Down
42 changes: 42 additions & 0 deletions src/tests/JIT/opt/RedundantBranch/RedundantBranchUnsigned.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
// 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;

// Runtime 65327
// M1 and M2 should generate the same code

public class RedundantBranchUnsigned
{
[MethodImpl(MethodImplOptions.NoInlining)]
public ReadOnlySpan<char> M1(ReadOnlySpan<char> span, int i)
{
// Note `<` here instead of `<=`
//
if ((uint)i < (uint)span.Length)
{
return span.Slice(i);
}
return default;
}

[MethodImpl(MethodImplOptions.NoInlining)]
public ReadOnlySpan<char> M2(ReadOnlySpan<char> span, int i)
{
if ((uint)i <= (uint)span.Length)
{
return span.Slice(i);
}
return default;
}

public static int Main()
{
var rbu = new RedundantBranchUnsigned();
var m1 = rbu.M1("hello", 2);
var m2 = rbu.M2("hello", 3);

return m1.Length + m2.Length + 95;
}
}
10 changes: 10 additions & 0 deletions src/tests/JIT/opt/RedundantBranch/RedundantBranchUnsigned.csproj
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<OutputType>Exe</OutputType>
<DebugType />
<Optimize>True</Optimize>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).cs" />
</ItemGroup>
</Project>

0 comments on commit 78528df

Please sign in to comment.