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: update non-null assertion prop to destructure VNs #49238

Merged
merged 1 commit into from
Mar 6, 2021
Merged

JIT: update non-null assertion prop to destructure VNs #49238

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
124 changes: 94 additions & 30 deletions src/coreclr/jit/assertionprop.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3685,16 +3685,21 @@ GenTree* Compiler::optAssertionProp_Comma(ASSERT_VALARG_TP assertions, GenTree*
return nullptr;
}

/*****************************************************************************
*
* Given a tree consisting of a Ind and a set of available assertions, we try
* to propagate an assertion and modify the Ind tree if we can. We pass in the
* root of the tree via 'stmt', for local copy prop 'stmt' will be nullptr.
*
* Returns the modified tree, or nullptr if no assertion prop took place.
*
*/

//------------------------------------------------------------------------
// optAssertionProp_Ind: see if we can prove the indirection can't cause
// and exception.
//
// Arguments:
// assertions - set of live assertions
// tree - tree to possibly optimize
// stmt - statement containing the tree
//
// Returns:
// The modified tree, or nullptr if no assertion prop took place.
//
// Notes:
// stmt may be nullptr during local assertion prop
//
GenTree* Compiler::optAssertionProp_Ind(ASSERT_VALARG_TP assertions, GenTree* tree, Statement* stmt)
{
assert(tree->OperIsIndir());
Expand Down Expand Up @@ -3742,16 +3747,26 @@ GenTree* Compiler::optAssertionProp_Ind(ASSERT_VALARG_TP assertions, GenTree* tr
return nullptr;
}

/*****************************************************************************
* Check if a non-null assertion can be made about the input operand "op"
* from the set of "assertions," or implicitly from the value number on "op."
*
* Sets "pVnBased" if the assertion is value number based. If no matching
* assertions are found from the table, then returns "NO_ASSERTION_INDEX."
*
* Note: If both VN and assertion table yield a matching assertion, "pVnBased"
* is only set and the return value is "NO_ASSERTION_INDEX."
*/
//------------------------------------------------------------------------
// optAssertionIsNonNull: see if we can prove a tree's value will be non-null
// based on assertions
//
// Arguments:
// op - tree to check
// assertions - set of live assertions
// pVnBased - [out] set to true if value numbers were used
// pIndex - [out] the assertion used in the proof
//
// Returns:
// true if the tree's value will be non-null
//
// Notes:
// Sets "pVnBased" if the assertion is value number based. If no matching
// assertions are found from the table, then returns "NO_ASSERTION_INDEX."
//
// If both VN and assertion table yield a matching assertion, "pVnBased"
// is only set and the return value is "NO_ASSERTION_INDEX."
//
bool Compiler::optAssertionIsNonNull(GenTree* op,
ASSERT_VALARG_TP assertions DEBUGARG(bool* pVnBased)
DEBUGARG(AssertionIndex* pIndex))
Expand All @@ -3769,20 +3784,35 @@ bool Compiler::optAssertionIsNonNull(GenTree* op,
return true;
}

AssertionIndex index = optAssertionIsNonNullInternal(op, assertions);
AssertionIndex index = optAssertionIsNonNullInternal(op, assertions DEBUGARG(pVnBased));
#ifdef DEBUG
*pIndex = index;
#endif
return index != NO_ASSERTION_INDEX;
}

/*****************************************************************************
* Check if a non-null assertion can be made about the input operand "op"
* from the set of "assertions."
*
*/
AssertionIndex Compiler::optAssertionIsNonNullInternal(GenTree* op, ASSERT_VALARG_TP assertions)
//------------------------------------------------------------------------
// optAssertionIsNonNullInternal: see if we can prove a tree's value will
// be non-null based on assertions
//
// Arguments:
// op - tree to check
// assertions - set of live assertions
// pVnBased - [out] set to true if value numbers were used
//
// Returns:
// index of assertion, or NO_ASSERTION_INDEX
//
AssertionIndex Compiler::optAssertionIsNonNullInternal(GenTree* op,
ASSERT_VALARG_TP assertions DEBUGARG(bool* pVnBased))
{

#ifdef DEBUG
// Initialize the out param
//
*pVnBased = false;
#endif

// If local assertion prop use lcl comparison, else use VN comparison.
if (!optLocalAssertionProp)
{
Expand All @@ -3791,9 +3821,32 @@ AssertionIndex Compiler::optAssertionIsNonNullInternal(GenTree* op, ASSERT_VALAR
return NO_ASSERTION_INDEX;
}

ValueNum vn = op->gtVNPair.GetConservative();
// Look at both the top-level vn, and
// the vn we get by stripping off any constant adds.
//
ValueNum vn = vnStore->VNConservativeNormalValue(op->gtVNPair);
ValueNum vnBase = vn;
VNFuncApp funcAttr;

// Check each assertion to find if we have a vn == or != null assertion.
while (vnStore->GetVNFunc(vnBase, &funcAttr) && (funcAttr.m_func == (VNFunc)GT_ADD))
Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally perhaps this while loop would be unnecessary, and VN creation would fold chains of constant adds into a single add.

Also note there is a similar loop on the gen side in optCreateAssertion which given a dereference through a byref, tries to walk the VN back to the inspiring gc ref.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also we should be able to get rid of the IR destructuring (see first part of optAssertionProp_Ind for example) for global prop as the VN destructuring should be equivalent, but when I did that I lost some of the diffs, and it creates more divergence between local prop and global prop.

{
if (vnStore->IsVNConstant(funcAttr.m_args[1]) && varTypeIsIntegral(vnStore->TypeOfVN(funcAttr.m_args[1])))
{
vnBase = funcAttr.m_args[0];
}
else if (vnStore->IsVNConstant(funcAttr.m_args[0]) &&
varTypeIsIntegral(vnStore->TypeOfVN(funcAttr.m_args[0])))
{
vnBase = funcAttr.m_args[1];
}
else
{
break;
}
}

// Check each assertion to find if we have a vn != null assertion.
//
BitVecOps::Iter iter(apTraits, assertions);
unsigned index = 0;
while (iter.NextElem(&index))
Expand All @@ -3808,10 +3861,21 @@ AssertionIndex Compiler::optAssertionIsNonNullInternal(GenTree* op, ASSERT_VALAR
{
continue;
}
if (curAssertion->op1.vn != vn || curAssertion->op2.vn != ValueNumStore::VNForNull())

if (curAssertion->op2.vn != ValueNumStore::VNForNull())
{
continue;
}

if ((curAssertion->op1.vn != vn) && (curAssertion->op1.vn != vnBase))
{
continue;
}

#ifdef DEBUG
*pVnBased = true;
#endif

return assertionIndex;
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/jit/compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7259,7 +7259,7 @@ class Compiler
var_types toType,
ASSERT_VALARG_TP assertions);
AssertionIndex optAssertionIsSubtype(GenTree* tree, GenTree* methodTableArg, ASSERT_VALARG_TP assertions);
AssertionIndex optAssertionIsNonNullInternal(GenTree* op, ASSERT_VALARG_TP assertions);
AssertionIndex optAssertionIsNonNullInternal(GenTree* op, ASSERT_VALARG_TP assertions DEBUGARG(bool* pVnBased));
bool optAssertionIsNonNull(GenTree* op,
ASSERT_VALARG_TP assertions DEBUGARG(bool* pVnBased) DEBUGARG(AssertionIndex* pIndex));

Expand Down