Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
66 changes: 55 additions & 11 deletions src/jit/assertionprop.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2995,6 +2995,23 @@ AssertionIndex Compiler::optGlobalAssertionIsEqualOrNotEqual(ASSERT_VALARG_TP as
{
return assertionIndex;
}

// Look for matching exact type assertions based on vtable accesses
if ((curAssertion->assertionKind == OAK_EQUAL) && (curAssertion->op1.kind == O1K_EXACT_TYPE) &&
op1->OperIs(GT_IND))

Choose a reason for hiding this comment

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

You don't allow OAK_NOT_EQUAL here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right -- let me update the comment.

{
GenTree* indirAddr = op1->AsIndir()->Addr();

if (indirAddr->OperIs(GT_LCL_VAR) && (indirAddr->TypeGet() == TYP_REF))
{
// op1 is accessing vtable of a ref type local var
if ((curAssertion->op1.vn == vnStore->VNConservativeNormalValue(indirAddr->gtVNPair)) &&
(curAssertion->op2.vn == vnStore->VNConservativeNormalValue(op2->gtVNPair)))
{
return assertionIndex;
}
}
}
}
return NO_ASSERTION_INDEX;
}
Expand Down Expand Up @@ -3119,13 +3136,19 @@ GenTree* Compiler::optAssertionPropGlobal_RelOp(ASSERT_VALARG_TP assertions, Gen
return optAssertionProp_Update(newTree, tree, stmt);
}

// Else check if we have an equality check involving a local
// Else check if we have an equality check involving a local or an indir
if (!tree->OperIs(GT_EQ, GT_NE))
{
return nullptr;
}

if (op1->gtOper != GT_LCL_VAR)
// Bail out if tree is not side effect free.
if ((tree->gtFlags & GTF_SIDE_EFFECT) != 0)

Choose a reason for hiding this comment

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

Can you describe the problem that you saw here when GTF_SIDE_EFFECT was true.
Did this change cause any current Asm diffs?

Copy link
Member Author

@AndyAyersMS AndyAyersMS Oct 30, 2019

Choose a reason for hiding this comment

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

No diffs from the previous version. I added this as a precaution, to parallel what we do above for the =/!= 0 case, since the simplification below will not preserve side effects.

{
return nullptr;
}

if (!op1->OperIs(GT_LCL_VAR, GT_IND))
{
return nullptr;
}
Expand Down Expand Up @@ -3188,11 +3211,21 @@ GenTree* Compiler::optAssertionPropGlobal_RelOp(ASSERT_VALARG_TP assertions, Gen
{
op1->ChangeOperConst(GT_CNS_INT);
op1->AsIntCon()->gtIconVal = vnStore->ConstantValue<int>(vnCns);

if (vnStore->IsVNHandle(vnCns))
Copy link

Choose a reason for hiding this comment

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

Presumably this case happens only on 32 bit targets?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was also considering not propagating handle constants into trees as they're generally not foldable and end up producing large immediates.

{
op1->gtFlags |= (vnStore->GetHandleFlags(vnCns) & GTF_ICON_HDL_MASK);
}
}
else if (op1->TypeGet() == TYP_LONG)
{
op1->ChangeOperConst(GT_CNS_NATIVELONG);
op1->AsIntConCommon()->SetLngValue(vnStore->ConstantValue<INT64>(vnCns));

if (vnStore->IsVNHandle(vnCns))
{
op1->gtFlags |= (vnStore->GetHandleFlags(vnCns) & GTF_ICON_HDL_MASK);
}
}
else if (op1->TypeGet() == TYP_DOUBLE)
{
Expand Down Expand Up @@ -3227,6 +3260,16 @@ GenTree* Compiler::optAssertionPropGlobal_RelOp(ASSERT_VALARG_TP assertions, Gen
}

op1->gtVNPair.SetBoth(vnCns); // Preserve the ValueNumPair, as ChangeOperConst/SetOper will clear it.

// Also set the value number on the relop.
if (curAssertion->assertionKind == OAK_EQUAL)
{
tree->gtVNPair.SetBoth(vnStore->VNOneForType(TYP_INT));
}
else
{
tree->gtVNPair.SetBoth(vnStore->VNZeroForType(TYP_INT));
}
}
// If the assertion involves "op2" and "op1" is also a local var, then just morph the tree.
else if (op2->gtOper == GT_LCL_VAR)
Expand Down Expand Up @@ -3957,7 +4000,7 @@ GenTree* Compiler::optAssertionProp_Update(GenTree* newTree, GenTree* tree, Stat
* Returns the modified tree, or nullptr if no assertion prop took place.
*/

GenTree* Compiler::optAssertionProp(ASSERT_VALARG_TP assertions, GenTree* tree, Statement* stmt)
GenTree* Compiler::optAssertionProp(ASSERT_VALARG_TP assertions, GenTree* tree, Statement* stmt, BasicBlock* block)
{
switch (tree->gtOper)
{
Expand Down Expand Up @@ -3992,6 +4035,14 @@ GenTree* Compiler::optAssertionProp(ASSERT_VALARG_TP assertions, GenTree* tree,

return optAssertionProp_RelOp(assertions, tree, stmt);

case GT_JTRUE:

if (block != nullptr)
{
return optVNConstantPropOnJTrue(block, tree);
}
return nullptr;

default:
return nullptr;
}
Expand Down Expand Up @@ -5153,18 +5204,11 @@ void Compiler::optAssertionPropMain()
// and thus we must morph, set order, re-link
for (GenTree* tree = stmt->GetTreeList(); tree != nullptr; tree = tree->gtNext)
{
if (tree->OperIs(GT_JTRUE))
{
// A GT_TRUE is always the last node in a tree, so we can break here
assert((tree->gtNext == nullptr) && (stmt->GetNextStmt() == nullptr));
break;
Copy link

Choose a reason for hiding this comment

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

Hmm, can't remember exactly why I added this early exit for JTRUE nodes. Most likely for consistency with the assertion gen loop that has special handling for JTRUE. Here the only potentially unwanted side effect of not exiting seems to be setting the wrong bit in assertions. But since it's the last node of the block it doesn't matter...

}

JITDUMP("Propagating %s assertions for " FMT_BB ", stmt " FMT_STMT ", tree [%06d], tree -> %d\n",
BitVecOps::ToString(apTraits, assertions), block->bbNum, stmt->GetID(), dspTreeID(tree),
tree->GetAssertionInfo().GetAssertionIndex());

GenTree* newTree = optAssertionProp(assertions, tree, stmt);
GenTree* newTree = optAssertionProp(assertions, tree, stmt, block);
if (newTree)
{
assert(optAssertionPropagatedCurrentStmt == true);
Expand Down
2 changes: 1 addition & 1 deletion src/jit/compiler.h
Original file line number Diff line number Diff line change
Expand Up @@ -6730,7 +6730,7 @@ class Compiler
Statement* stmt DEBUGARG(AssertionIndex index));

// Assertion propagation functions.
GenTree* optAssertionProp(ASSERT_VALARG_TP assertions, GenTree* tree, Statement* stmt);
GenTree* optAssertionProp(ASSERT_VALARG_TP assertions, GenTree* tree, Statement* stmt, BasicBlock* block);
GenTree* optAssertionProp_LclVar(ASSERT_VALARG_TP assertions, GenTree* tree, Statement* stmt);
GenTree* optAssertionProp_Ind(ASSERT_VALARG_TP assertions, GenTree* tree, Statement* stmt);
GenTree* optAssertionProp_Cast(ASSERT_VALARG_TP assertions, GenTree* tree, Statement* stmt);
Expand Down
2 changes: 1 addition & 1 deletion src/jit/morph.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14444,7 +14444,7 @@ GenTree* Compiler::fgMorphTree(GenTree* tree, MorphAddrContext* mac)
{
tree = newTree;
/* newTree is non-Null if we propagated an assertion */
newTree = optAssertionProp(apFull, tree, nullptr);
newTree = optAssertionProp(apFull, tree, nullptr, nullptr);
}
assert(tree != nullptr);
}
Expand Down