Skip to content

Try to fold basic arithmetic operations as part of import #97901

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

Merged
merged 8 commits into from
Feb 4, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
115 changes: 106 additions & 9 deletions src/coreclr/jit/assertionprop.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4631,7 +4631,11 @@ GenTree* Compiler::optAssertionProp_Comma(ASSERT_VALARG_TP assertions, GenTree*
//
GenTree* Compiler::optAssertionProp_Ind(ASSERT_VALARG_TP assertions, GenTree* tree, Statement* stmt)
{
if (optNonNullAssertionProp_Ind(assertions, tree))
assert(tree->OperIsIndir());

bool didNonNullProp = optNonNullAssertionProp_Ind(assertions, tree);
bool didNonHeapProp = optNonHeapAssertionProp_Ind(assertions, tree);
if (didNonNullProp || didNonHeapProp)
{
return optAssertionProp_Update(tree, tree, stmt);
}
Expand All @@ -4644,12 +4648,12 @@ GenTree* Compiler::optAssertionProp_Ind(ASSERT_VALARG_TP assertions, GenTree* tr
// based on assertions
//
// Arguments:
// op - tree to check
// 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
// pVnBased - [out] set to true if value numbers were used
// pIndex - [out] the assertion used in the proof
//
// Returns:
// Return Value:
// true if the tree's value will be non-null
//
// Notes:
Expand Down Expand Up @@ -4699,11 +4703,11 @@ bool Compiler::optAssertionIsNonNull(GenTree* op,
// 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
// op - tree to check
// assertions - set of live assertions
// pVnBased - [out] set to true if value numbers were used
//
// Returns:
// Return Value:
// index of assertion, or NO_ASSERTION_INDEX
//
AssertionIndex Compiler::optAssertionIsNonNullInternal(GenTree* op,
Expand Down Expand Up @@ -4809,6 +4813,7 @@ AssertionIndex Compiler::optAssertionIsNonNullInternal(GenTree* op,
}
return NO_ASSERTION_INDEX;
}

/*****************************************************************************
*
* Given a tree consisting of a call and a set of available assertions, we
Expand Down Expand Up @@ -4894,6 +4899,98 @@ bool Compiler::optNonNullAssertionProp_Ind(ASSERT_VALARG_TP assertions, GenTree*
return false;
}

//------------------------------------------------------------------------
// optAssertionIsNonHeap: see if we can prove a tree's value will be not GC-tracked
// 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
//
// Return Value:
// true if the tree's value will be not GC-tracked
//
// 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::optAssertionIsNonHeap(GenTree* op,
ASSERT_VALARG_TP assertions DEBUGARG(bool* pVnBased)
DEBUGARG(AssertionIndex* pIndex))
{
bool vnBased = (!optLocalAssertionProp && vnStore->IsVNObjHandle(op->gtVNPair.GetConservative()));
#ifdef DEBUG
*pIndex = NO_ASSERTION_INDEX;
*pVnBased = vnBased;
#endif

if (vnBased)
{
return true;
}

if (op->IsIconHandle(GTF_ICON_OBJ_HDL))
{
return true;
}

return false;
}

//------------------------------------------------------------------------
// optNonHeapAssertionProp_Ind: Possibly prove an indirection not GC-tracked.
//
// Arguments:
// assertions - Active assertions
// indir - The indirection
//
// Return Value:
// Whether the indirection was found to be not GC-tracked and marked as such.
//
bool Compiler::optNonHeapAssertionProp_Ind(ASSERT_VALARG_TP assertions, GenTree* indir)
{
assert(indir->OperIsIndir());

if (!indir->OperIs(GT_STOREIND) || (indir->gtFlags & GTF_IND_TGT_NOT_HEAP) != 0)
{
return false;
}

// We like CSE to happen for handles, as the codegen for loading a 64-bit constant can be pretty heavy
// and this is particularly true on platforms with a fixed-width instruction encoding. However, this
// pessimizes stores as we can no longer optimize around some object handles that would allow us to
// bypass the write barrier.
//
// In order to handle that, we'll propagate the IND_TGT_NOT_HEAP flag onto the store if the handle is
// directly or if the underlying value number is an applicable object handle.

#ifdef DEBUG
bool vnBased = false;
AssertionIndex index = NO_ASSERTION_INDEX;
#endif
if (optAssertionIsNonHeap(indir->AsIndir()->Data(), assertions DEBUGARG(&vnBased) DEBUGARG(&index)))
{
#ifdef DEBUG
if (verbose)
{
(vnBased) ? printf("\nVN based non-heap prop in " FMT_BB ":\n", compCurBB->bbNum)
: printf("\nNon-heap prop for index #%02u in " FMT_BB ":\n", index, compCurBB->bbNum);
gtDispTree(indir, nullptr, nullptr, true);
}
#endif
indir->gtFlags |= GTF_IND_TGT_NOT_HEAP;

return true;
}

return false;
}

/*****************************************************************************
*
* Given a tree consisting of a call and a set of available assertions, we
Expand Down
3 changes: 3 additions & 0 deletions src/coreclr/jit/compiler.h
Original file line number Diff line number Diff line change
Expand Up @@ -7771,6 +7771,8 @@ class Compiler
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));
bool optAssertionIsNonHeap(GenTree* op,
ASSERT_VALARG_TP assertions DEBUGARG(bool* pVnBased) DEBUGARG(AssertionIndex* pIndex));

AssertionIndex optGlobalAssertionIsEqualOrNotEqual(ASSERT_VALARG_TP assertions, GenTree* op1, GenTree* op2);
AssertionIndex optGlobalAssertionIsEqualOrNotEqualZero(ASSERT_VALARG_TP assertions, GenTree* op1);
Expand Down Expand Up @@ -7807,6 +7809,7 @@ class Compiler
GenTree* optAssertionProp_Update(GenTree* newTree, GenTree* tree, Statement* stmt);
GenTree* optNonNullAssertionProp_Call(ASSERT_VALARG_TP assertions, GenTreeCall* call);
bool optNonNullAssertionProp_Ind(ASSERT_VALARG_TP assertions, GenTree* indir);
bool optNonHeapAssertionProp_Ind(ASSERT_VALARG_TP assertions, GenTree* indir);

// Implied assertion functions.
void optImpliedAssertions(AssertionIndex assertionIndex, ASSERT_TP& activeAssertions);
Expand Down
3 changes: 2 additions & 1 deletion src/coreclr/jit/gcinfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,8 @@ GCInfo::WriteBarrierForm GCInfo::gcIsWriteBarrierCandidate(GenTreeStoreInd* stor
if ((store->gtFlags & GTF_IND_TGT_NOT_HEAP) != 0)
{
// This indirection is not storing to the heap.
// This case occurs for stack-allocated objects.
// This case occurs for stack-allocated objects
// and for some types of CSE'd frozen object handles.
return WBF_NoBarrier;
}

Expand Down
11 changes: 10 additions & 1 deletion src/coreclr/jit/gentree.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19326,7 +19326,16 @@ FieldSeq* FieldSeqStore::Append(FieldSeq* a, FieldSeq* b)
return a;
}

assert(!"Duplicate field sequences!");
// In UB-like code (such as manual IL) we can see an addition of two static field addresses.
// Treat that as cancelling out the sequence, since the result will point nowhere.
//
// It may be possible for the JIT to encounter other types of UB additions, such as due to
// complex optimizations, inlining, etc. In release we'll still do the right thing by returning
// nullptr here, but the more conservative assert can help avoid JIT bugs

assert(a->GetKind() == FieldSeq::FieldKind::SimpleStaticKnownAddress);
assert(b->GetKind() == FieldSeq::FieldKind::SimpleStaticKnownAddress);
Copy link
Member

Choose a reason for hiding this comment

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

Did you hit that assert?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, its what was discussed further up in #97901 (comment)

Basically, we have an IL test that explicitly adds two ldsflda. It does so after a conv.i4, but that is a nop on 32-bit and a user can write such IL without the conv.i4 as well, so the assert was already somewhat incorrect just not hit due to us not constant folding this early.

Copy link
Member

Choose a reason for hiding this comment

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

Ok. Yeah I think we discussed it with SingleAccretion at some point


return nullptr;
}

Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/jit/gentree.h
Original file line number Diff line number Diff line change
Expand Up @@ -480,7 +480,7 @@ enum GenTreeFlags : unsigned int
GTF_INX_RNGCHK = 0x80000000, // GT_INDEX_ADDR -- this array address should be range-checked
GTF_INX_ADDR_NONNULL = 0x40000000, // GT_INDEX_ADDR -- this array address is not null

GTF_IND_TGT_NOT_HEAP = 0x80000000, // GT_IND -- the target is not on the heap
GTF_IND_TGT_NOT_HEAP = 0x80000000, // GT_IND -- the target is not GC-tracked, such as an object on the nongc heap
GTF_IND_VOLATILE = 0x40000000, // OperIsIndir() -- the load or store must use volatile semantics (this is a nop on X86)
GTF_IND_NONFAULTING = 0x20000000, // OperIsIndir() -- An indir that cannot fault.
GTF_IND_TGT_HEAP = 0x10000000, // GT_IND -- the target is on the heap
Expand Down
21 changes: 19 additions & 2 deletions src/coreclr/jit/importer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7203,6 +7203,9 @@ void Compiler::impImportBlockCode(BasicBlock* block)
}
}

// Fold result, if possible.
op1 = gtFoldExpr(op1);
Comment on lines +7206 to +7207
Copy link
Member Author

Choose a reason for hiding this comment

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

This one is hit for MATH_OP2, so cases like ADD, SUB, MUL, DIV, REM, AND, OR, XOR


impPushOnStack(op1, tiRetVal);
break;

Expand All @@ -7225,14 +7228,23 @@ void Compiler::impImportBlockCode(BasicBlock* block)
type = genActualType(op1->TypeGet());
op1 = gtNewOperNode(oper, type, op1, op2);

// Fold result, if possible.
op1 = gtFoldExpr(op1);
Comment on lines +7231 to +7232
Copy link
Member Author

Choose a reason for hiding this comment

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

This one is hit for SH_OP2, so cases like SHL, SHR, SHR_UN


impPushOnStack(op1, tiRetVal);
break;

case CEE_NOT:
op1 = impPopStack().val;
impBashVarAddrsToI(op1, nullptr);

type = genActualType(op1->TypeGet());
impPushOnStack(gtNewOperNode(GT_NOT, type, op1), tiRetVal);
op1 = gtNewOperNode(GT_NOT, type, op1);

// Fold result, if possible.
op1 = gtFoldExpr(op1);
Comment on lines +7244 to +7245
Copy link
Member Author

Choose a reason for hiding this comment

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

This one is hit for NOT


impPushOnStack(op1, tiRetVal);
break;

case CEE_CKFINITE:
Expand Down Expand Up @@ -7960,7 +7972,12 @@ void Compiler::impImportBlockCode(BasicBlock* block)
case CEE_NEG:
op1 = impPopStack().val;
impBashVarAddrsToI(op1, nullptr);
impPushOnStack(gtNewOperNode(GT_NEG, genActualType(op1->gtType), op1), tiRetVal);
op1 = gtNewOperNode(GT_NEG, genActualType(op1->gtType), op1);

// Fold result, if possible.
op1 = gtFoldExpr(op1);
Comment on lines +7977 to +7978
Copy link
Member Author

Choose a reason for hiding this comment

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

This one is hit for NEG


impPushOnStack(op1, tiRetVal);
break;

case CEE_POP:
Expand Down