From 14b08c8c1194fd509d0bc5c7d11025e2e9150667 Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Sun, 4 Feb 2024 09:39:31 -0800 Subject: [PATCH] Try to fold basic arithmetic operations as part of import (#97901) * Try to fold basic arithmetic operations as part of import * Don't try to constant fold addition of two field sequences * Add braces * Instead, update the assert in FieldSeqStore::Append * Qualify FieldSeq::FieldKind * Continue allowing STOREIND for ICON_OBJ_HDL to skip the GC write barrier * Ensure we don't infinitely recurse * Respond to PR feedback --- src/coreclr/jit/assertionprop.cpp | 115 +++++++++++++++++++++++++++--- src/coreclr/jit/compiler.h | 3 + src/coreclr/jit/gcinfo.cpp | 3 +- src/coreclr/jit/gentree.cpp | 11 ++- src/coreclr/jit/gentree.h | 2 +- src/coreclr/jit/importer.cpp | 21 +++++- 6 files changed, 141 insertions(+), 14 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index 9dcdc79fd25af..ff5387f2c5678 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -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); } @@ -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: @@ -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, @@ -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 @@ -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 diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index ed6eb8787f8de..649de34c7c218 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -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); @@ -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); diff --git a/src/coreclr/jit/gcinfo.cpp b/src/coreclr/jit/gcinfo.cpp index 86c40ca8b1ea0..2364ce374e494 100644 --- a/src/coreclr/jit/gcinfo.cpp +++ b/src/coreclr/jit/gcinfo.cpp @@ -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; } diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index b23eb5d0eb432..2ef6833701ae5 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -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); + return nullptr; } diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 2d352c262f27d..6eb9a1858ae78 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -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 diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 69d623787fa35..3ba9e845fc8ab 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -7203,6 +7203,9 @@ void Compiler::impImportBlockCode(BasicBlock* block) } } + // Fold result, if possible. + op1 = gtFoldExpr(op1); + impPushOnStack(op1, tiRetVal); break; @@ -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); + 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); + + impPushOnStack(op1, tiRetVal); break; case CEE_CKFINITE: @@ -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); + + impPushOnStack(op1, tiRetVal); break; case CEE_POP: