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

Improve branch optimizer around implied relops #95234

Merged
merged 32 commits into from
Jan 2, 2024
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
2447227
Optimize "implied relops" in assertprop
EgorBo Nov 25, 2023
a6e0867
Improve impl
EgorBo Nov 25, 2023
579c05e
Clean up
EgorBo Nov 25, 2023
7171a5d
Clean up
EgorBo Nov 25, 2023
62a5f35
Add GT_EQ
EgorBo Nov 25, 2023
7489fad
add GT_NE as well
EgorBo Nov 26, 2023
a104102
clean up
EgorBo Nov 26, 2023
ffa7d1a
Test commit
EgorBo Nov 26, 2023
3f82e12
Implement in redundant-branch phase
EgorBo Nov 26, 2023
eff4f9d
jit-format
EgorBo Nov 26, 2023
cbe3461
Fix build
EgorBo Nov 26, 2023
069279c
Fix build
EgorBo Nov 26, 2023
2c18ec4
test
EgorBo Nov 26, 2023
541ada4
jit-format
EgorBo Nov 26, 2023
5245290
Test commit
EgorBo Nov 26, 2023
dacc1ed
Merge branch 'main' of github.com:dotnet/runtime into implied-relop-a…
EgorBo Nov 26, 2023
4c18b0b
Merge branch 'main' of github.com:dotnet/runtime into implied-relop-a…
EgorBo Nov 26, 2023
a3b15c9
Merge branch 'main' of github.com:dotnet/runtime into implied-relop-a…
EgorBo Nov 30, 2023
9418be5
test fix
EgorBo Nov 30, 2023
efbb400
Better fix
EgorBo Nov 30, 2023
714c001
Final clean up
EgorBo Nov 30, 2023
d796e79
Rename function
EgorBo Nov 30, 2023
5f66730
Revert previous changes
EgorBo Nov 30, 2023
94776af
Clean up
EgorBo Nov 30, 2023
6bdd596
Handle GT_NE
EgorBo Nov 30, 2023
a23517e
Merge branch 'implied-relop-assertprop' of github.com:EgorBo/runtime-…
EgorBo Dec 3, 2023
bb3f8cb
Merge branch 'main' of github.com:dotnet/runtime into implied-relop-a…
EgorBo Dec 3, 2023
b1c69f0
test
EgorBo Dec 3, 2023
c8136b0
Check type of domApp.m_args[0] (same as treeApp.m_args[0])
EgorBo Dec 3, 2023
302430e
Update redundantbranchopts.cpp
EgorBo Dec 4, 2023
ce5e346
Merge branch 'main' of github.com:dotnet/runtime into implied-relop-a…
EgorBo Dec 27, 2023
2d8a6df
Replace ssize_t with target_ssize_t
EgorBo Dec 27, 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
228 changes: 226 additions & 2 deletions src/coreclr/jit/redundantbranchopts.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,171 @@ struct RelopImplicationRule
bool reverse;
};

enum RelopResult
{
Unknown,
AlwaysFalse,
AlwaysTrue
};

//------------------------------------------------------------------------
// IsCmp2ImpliedByCmp1: given two constant range checks:
//
// if (X oper1 bound1)
// {
// if (X oper2 bound2)
// {
//
// determine if the second range check is implied by the dominating first one.
//
// Arguments:
// oper1 - the first comparison operator
// bound1 - the first constant bound
// oper2 - the second comparison operator
// bound2 - the second constant bound
//
// Returns:
// Unknown - the second check is not implied by the first one
// AlwaysFalse - the second check is implied by the first one and is always false
// AlwaysTrue - the second check is implied by the first one and is always true
//
RelopResult IsCmp2ImpliedByCmp1(genTreeOps oper1, ssize_t bound1, genTreeOps oper2, ssize_t bound2)
{
struct IntegralRange
{
ssize_t startIncl; // inclusive
ssize_t endIncl; // inclusive

bool Intersects(const IntegralRange other) const
{
return (startIncl <= other.endIncl) && (other.startIncl <= endIncl);
}

bool Contains(const IntegralRange other) const
{
return (startIncl <= other.startIncl) && (other.endIncl <= endIncl);
}
};

// Start with the widest possible ranges
IntegralRange range1 = {SSIZE_T_MIN, SSIZE_T_MAX};
IntegralRange range2 = {SSIZE_T_MIN, SSIZE_T_MAX};

// Update ranges based on inputs
auto setRange = [](genTreeOps oper, ssize_t bound, IntegralRange* range) -> bool {
switch (oper)
{
case GT_LT:
// x < cns -> [SSIZE_T_MIN, cns - 1]
if (bound == SSIZE_T_MIN)
{
// overflows
return false;
}
range->endIncl = bound - 1;
return true;

case GT_LE:
// x <= cns -> [SSIZE_T_MIN, cns]
range->endIncl = bound;
return true;

case GT_GT:
// x > cns -> [cns + 1, SSIZE_T_MAX]
if (bound == SSIZE_T_MAX)
{
// overflows
return false;
}
range->startIncl = bound + 1;
return true;

case GT_GE:
// x >= cns -> [cns, SSIZE_T_MAX]
range->startIncl = bound;
return true;

case GT_EQ:
case GT_NE:
// x == cns -> [cns, cns]
// NE is special-cased below
range->startIncl = bound;
range->endIncl = bound;
return true;

default:
// unsupported operator
return false;
}
};

if (setRange(oper1, bound1, &range1) && setRange(oper2, bound2, &range2))
{
// Special handling of GT_NE:
if ((oper1 == GT_NE) || (oper2 == GT_NE))
{
// if (x != 100)
// if (x != 100) // always true
if (oper1 == oper2)
{
return bound1 == bound2 ? RelopResult::AlwaysTrue : RelopResult::Unknown;
}

// if (x == 100)
// if (x != 100) // always false
//
// if (x == 100)
// if (x != 101) // always true
if (oper1 == GT_EQ)
{
return bound1 == bound2 ? RelopResult::AlwaysFalse : RelopResult::AlwaysTrue;
}

// if (x > 100)
// if (x != 10) // always true
if ((oper2 == GT_NE) && !range1.Intersects(range2))
{
return AlwaysTrue;
}

return RelopResult::Unknown;
}

// If ranges never intersect, then the 2nd range is never "true"
if (!range1.Intersects(range2))
{
// E.g.:
//
// range1: [100 .. SSIZE_T_MAX]
// range2: [SSIZE_T_MIN .. 10]
//
// or in other words:
//
// if (x >= 100)
// if (x <= 10) // always false
//
return RelopResult::AlwaysFalse;
}

// If range1 is a subset of range2, then the 2nd range is always "true"
if (range2.Contains(range1))
{
// E.g.:
//
// range1: [100 .. SSIZE_T_MAX]
// range2: [10 .. SSIZE_T_MAX]
//
// or in other words:
//
// if (x >= 100)
// if (x >= 10) // always true
//
return RelopResult::AlwaysTrue;
}
}
return RelopResult::Unknown;
}

//------------------------------------------------------------------------
// s_implicationRules: rule table for unrelated relops
//
Expand Down Expand Up @@ -257,8 +422,6 @@ static const RelopImplicationRule s_implicationRules[] =
// We don't get all the cases here we could. Still to do:
// * two unsigned compares, same operands
// * mixture of signed/unsigned compares, same operands
// * mixture of compares, one operand same, other operands different constants
// x > 1 ==> x >= 0
//
void Compiler::optRelopImpliesRelop(RelopImplicationInfo* rii)
{
Expand Down Expand Up @@ -338,6 +501,67 @@ void Compiler::optRelopImpliesRelop(RelopImplicationInfo* rii)
}
}
}

// Given R(x, cns1) and R*(x, cns2) see if we can infer R* from R.
// We assume cns1 and cns2 are always on the RHS of the compare.
if ((treeApp.m_args[0] == domApp.m_args[0]) && vnStore->IsVNConstant(treeApp.m_args[1]) &&
vnStore->IsVNConstant(domApp.m_args[1]) && varTypeIsIntOrI(vnStore->TypeOfVN(treeApp.m_args[1])) &&
varTypeIsIntOrI(vnStore->TypeOfVN(domApp.m_args[1])))
{
// We currently don't handle VNF_relop_UN funcs here
if (ValueNumStore::VNFuncIsSignedComparison(domApp.m_func) &&
ValueNumStore::VNFuncIsSignedComparison(treeApp.m_func))
{
// Dominating "X relop CNS"
const genTreeOps domOper = static_cast<genTreeOps>(domApp.m_func);
const ssize_t domCns = vnStore->CoercedConstantValue<ssize_t>(domApp.m_args[1]);

// Dominated "X relop CNS"
const genTreeOps treeOper = static_cast<genTreeOps>(treeApp.m_func);
const ssize_t treeCns = vnStore->CoercedConstantValue<ssize_t>(treeApp.m_args[1]);

// Example:
//
// void Test(int x)
// {
// if (x > 100)
// if (x > 10)
// Console.WriteLine("Taken!");
// }
//

// Corresponding BB layout:
//
// BB1:
// if (x <= 100)
// goto BB4
//
// BB2:
// // x is known to be > 100 here
// if (x <= 10) // never true
// goto BB4
//
// BB3:
// Console.WriteLine("Taken!");
//
// BB4:
// return;

// Check whether the dominating compare being "false" implies the dominated compare is known
Copy link
Member

Choose a reason for hiding this comment

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

The reversal/negation here is a bit tricky, but I think I follow it.

Seems like there's a complementary case where you don't negate the upper relop and then may instead have canInferFromTrue abilities?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, basically there are two cases:

canInferFromFalse:

if (x > 100)
{
    if (x > 10)
    {
        ...
    }
}

canInferFromTrue:

if (x > 100)
{
    ...
}
if (x > 10)
{
    ...
}

I'll check separately the 2nd case (last I tried it added very little diffs)

// to be either "true" or "false".
RelopResult treeOperStatus =
IsCmp2ImpliedByCmp1(GenTree::ReverseRelop(domOper), domCns, treeOper, treeCns);
if (treeOperStatus != RelopResult::Unknown)
{
rii->canInfer = true;
rii->vnRelation = ValueNumStore::VN_RELATION_KIND::VRK_Inferred;
rii->canInferFromTrue = false;
rii->canInferFromFalse = true;
rii->reverseSense = treeOperStatus == RelopResult::AlwaysTrue;
return;
}
}
}
}

// See if dominating compare is a compound comparison that might
Expand Down
12 changes: 12 additions & 0 deletions src/coreclr/jit/valuenum.h
Original file line number Diff line number Diff line change
Expand Up @@ -1046,6 +1046,9 @@ class ValueNumStore
// Returns "true" iff "vnf" is a comparison (and thus binary) operator.
static bool VNFuncIsComparison(VNFunc vnf);

// Returns "true" iff "vnf" is a signed comparison (and thus binary) operator.
static bool VNFuncIsSignedComparison(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 Expand Up @@ -2095,6 +2098,15 @@ inline bool ValueNumStore::VNFuncIsComparison(VNFunc vnf)
return GenTree::OperIsCompare(gtOp) != 0;
}

inline bool ValueNumStore::VNFuncIsSignedComparison(VNFunc vnf)
{
if (vnf >= VNF_Boundary)
{
return false;
}
return GenTree::OperIsCompare(genTreeOps(vnf)) != 0;
}

template <>
inline size_t ValueNumStore::CoerceTypRefToT(Chunk* c, unsigned offset)
{
Expand Down
Loading