From 4383fba9dd6e7c1fce6f56ec4aa08d11f34c5d6c Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 13 Feb 2024 04:21:04 +0100 Subject: [PATCH 01/12] VN version --- src/coreclr/jit/assertionprop.cpp | 95 +++++++++++++++++++++++-------- src/coreclr/jit/compiler.h | 6 ++ src/coreclr/jit/gentree.cpp | 32 +++++++++++ src/coreclr/jit/importer.cpp | 3 +- src/coreclr/jit/morph.cpp | 18 +----- 5 files changed, 113 insertions(+), 41 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index 13ddd67409606..c664b7330087c 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -2663,6 +2663,60 @@ AssertionIndex Compiler::optAssertionIsSubtype(GenTree* tree, GenTree* methodTab return NO_ASSERTION_INDEX; } +//------------------------------------------------------------------------------ +// optVNStrengthReductionOnTree: +// +// Arguments: +// block - The block containing the tree. +// parent - The parent node of the tree. +// tree - The tree node +// +// Return Value: +// Returns a new tree or nullptr if nothing is changed. +// +GenTree* Compiler::optVNStrengthReductionOnTree(BasicBlock* block, GenTree* parent, GenTree* tree) +{ + if (tree->IsHelperCall()) + { + GenTreeCall* call = tree->AsCall(); + switch (call->GetHelperNum()) + { + // Fold "cast(cast(obj, cls), cls)" to "cast(obj, cls)" + case CORINFO_HELP_CHKCASTARRAY: + case CORINFO_HELP_CHKCASTANY: + case CORINFO_HELP_CHKCASTINTERFACE: + case CORINFO_HELP_CHKCASTCLASS: + case CORINFO_HELP_ISINSTANCEOFARRAY: + case CORINFO_HELP_ISINSTANCEOFCLASS: + case CORINFO_HELP_ISINSTANCEOFANY: + case CORINFO_HELP_ISINSTANCEOFINTERFACE: + { + GenTree* clsArg = call->gtArgs.GetUserArgByIndex(0)->GetNode(); + GenTree* objArg = call->gtArgs.GetUserArgByIndex(1)->GetNode(); + ValueNum clsArgVN = clsArg->gtVNPair.GetConservative(); + ValueNum objArgVN = objArg->gtVNPair.GetConservative(); + + VNFuncApp funcApp; + if (vnStore->GetVNFunc(objArgVN, &funcApp) && + ((funcApp.m_func == VNF_CastClass) || (funcApp.m_func == VNF_IsInstanceOf)) && + (funcApp.m_args[0] == clsArgVN)) + { + // The outer cast is redundant, remove it and preserve its side effects + // We do ignoreRoot here because the actual cast node is proven to never any exceptions + // (namely, InvalidCastException). + return gtWrapWithSideEffects(objArg, call, GTF_ALL_EFFECT, true); + } + } + break; + + default: + break; + } + } + + return nullptr; +} + //------------------------------------------------------------------------------ // optVNConstantPropOnTree: Substitutes tree with an evaluated constant while // managing side-effects. @@ -5098,33 +5152,22 @@ GenTree* Compiler::optAssertionProp_Call(ASSERT_VALARG_TP assertions, GenTreeCal (helper == CORINFO_HELP_CHKCASTCLASS) || (helper == CORINFO_HELP_CHKCASTANY) || (helper == CORINFO_HELP_CHKCASTCLASS_SPECIAL)) { - GenTree* arg1 = call->gtArgs.GetArgByIndex(1)->GetNode(); - if (arg1->gtOper != GT_LCL_VAR) + GenTree* objArg = call->gtArgs.GetArgByIndex(1)->GetNode(); + GenTree* clsArg = call->gtArgs.GetArgByIndex(0)->GetNode(); + + if (objArg->gtOper != GT_LCL_VAR) { return nullptr; } - GenTree* arg2 = call->gtArgs.GetArgByIndex(0)->GetNode(); - - unsigned index = optAssertionIsSubtype(arg1, arg2, assertions); + unsigned index = optAssertionIsSubtype(objArg, clsArg, assertions); if (index != NO_ASSERTION_INDEX) { -#ifdef DEBUG - if (verbose) - { - printf("\nDid VN based subtype prop for index #%02u in " FMT_BB ":\n", index, compCurBB->bbNum); - gtDispTree(call, nullptr, nullptr, true); - } -#endif - GenTree* list = nullptr; - gtExtractSideEffList(call, &list, GTF_SIDE_EFFECT, true); - if (list != nullptr) - { - arg1 = gtNewOperNode(GT_COMMA, call->TypeGet(), list, arg1); - fgSetTreeSeq(arg1); - } + JITDUMP("\nDid VN based subtype prop for index #%02u in " FMT_BB ":\n", index, compCurBB->bbNum); + DISPTREE(call); - return optAssertionProp_Update(arg1, call, stmt); + objArg = gtWrapWithSideEffects(objArg, call, GTF_SIDE_EFFECT, true); + return optAssertionProp_Update(objArg, call, stmt); } // Leave a hint for fgLateCastExpansion that obj is never null. @@ -5132,7 +5175,7 @@ GenTree* Compiler::optAssertionProp_Call(ASSERT_VALARG_TP assertions, GenTreeCal INDEBUG(bool vnBased = false); // GTF_CALL_M_CAST_CAN_BE_EXPANDED check is to improve TP if (((call->gtCallMoreFlags & GTF_CALL_M_CAST_CAN_BE_EXPANDED) != 0) && - optAssertionIsNonNull(arg1, assertions DEBUGARG(&vnBased) DEBUGARG(&nonNullIdx))) + optAssertionIsNonNull(objArg, assertions DEBUGARG(&vnBased) DEBUGARG(&nonNullIdx))) { call->gtCallMoreFlags |= GTF_CALL_M_CAST_OBJ_NONNULL; return optAssertionProp_Update(call, call, stmt); @@ -6419,8 +6462,14 @@ Compiler::fgWalkResult Compiler::optVNConstantPropCurStmt(BasicBlock* block, if (newTree == nullptr) { - // Not propagated, keep going. - return WALK_CONTINUE; + // If it wasn't a constant, let's see if we can reduce the strength + // of the tree using VN. + newTree = optVNStrengthReductionOnTree(block, parent, tree); + if (newTree == nullptr) + { + // Not propagated, keep going. + return WALK_CONTINUE; + } } // TODO https://github.com/dotnet/runtime/issues/10450: diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 1d4dbcdb82aa8..63f77db923b7d 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -3515,6 +3515,11 @@ class Compiler GenTreeFlags GenTreeFlags = GTF_SIDE_EFFECT, bool ignoreRoot = false); + GenTree* gtWrapWithSideEffects(GenTree* tree, + GenTree* sideEffectsSource, + GenTreeFlags sideEffectsFlags = GTF_ALL_EFFECT, + bool ignoreRoot = false); + bool gtSplitTree( BasicBlock* block, Statement* stmt, GenTree* splitPoint, Statement** firstNewStmt, GenTree*** splitPointUse); @@ -7713,6 +7718,7 @@ class Compiler fgWalkResult optVNConstantPropCurStmt(BasicBlock* block, Statement* stmt, GenTree* parent, GenTree* tree); GenTree* optVNConstantPropOnJTrue(BasicBlock* block, GenTree* test); GenTree* optVNConstantPropOnTree(BasicBlock* block, GenTree* parent, GenTree* tree); + GenTree* optVNStrengthReductionOnTree(BasicBlock* block, GenTree* parent, GenTree* tree); GenTree* optExtractSideEffListFromConst(GenTree* tree); AssertionIndex GetAssertionCount() diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 58c28f322f375..fc6dac23b2e06 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -17063,6 +17063,38 @@ bool Compiler::gtSplitTree( return splitter.MadeChanges; } +//------------------------------------------------------------------------ +// gtExtractSideEffList: Extracts side effects from sideEffectSource (if any) +// and wraps the input tree with a COMMA node with them. +// +// Arguments: +// tree - the expression tree to wrap with side effects (if any) +// sideEffectSource - the expression tree to extract side effects from +// flags - side effect flags to be considered +// ignoreRoot - ignore side effects on the expression root node +// +// Return Value: +// The original tree wrapped with a COMMA node that contains the side effects +// or just the tree itself if sideEffectSource has no side effects. +// +GenTree* Compiler::gtWrapWithSideEffects(GenTree* tree, + GenTree* sideEffectSource, + GenTreeFlags sideEffectsFlags, + bool ignoreRoot) +{ + GenTree* sideEffects = nullptr; + gtExtractSideEffList(sideEffectSource, &sideEffects, sideEffectsFlags, ignoreRoot); + if (sideEffects != nullptr) + { + tree = gtNewOperNode(GT_COMMA, tree->TypeGet(), sideEffects, tree); + if (fgNodeThreading == NodeThreading::AllTrees) + { + fgSetTreeSeq(tree); + } + } + return tree; +} + //------------------------------------------------------------------------ // gtExtractSideEffList: Extracts side effects from the given expression. // diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 2e79a60439020..b3d035b26d96b 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -1677,8 +1677,7 @@ GenTree* Compiler::impRuntimeLookupToTree(CORINFO_RESOLVED_TOKEN* pResolvedToken return slotPtrTree; } - slotPtrTree = gtNewIndir(TYP_I_IMPL, slotPtrTree, GTF_IND_NONFAULTING); - slotPtrTree->gtFlags &= ~GTF_GLOB_REF; // TODO-Bug?: this is a quirk. Can we mark this indirection invariant? + slotPtrTree = gtNewIndir(TYP_I_IMPL, slotPtrTree, GTF_IND_NONFAULTING | GTF_IND_INVARIANT); return slotPtrTree; } diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 312fc8300966a..feb109f9b6877 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -9653,22 +9653,8 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac, bool* optA } else { - GenTree* op1SideEffects = nullptr; - gtExtractSideEffList(op1, &op1SideEffects, GTF_ALL_EFFECT); - if (op1SideEffects != nullptr) - { - DEBUG_DESTROY_NODE(tree); - // Keep side-effects of op1 - tree = gtNewOperNode(GT_COMMA, TYP_INT, op1SideEffects, gtNewIconNode(0)); - JITDUMP("false with side effects:\n") - DISPTREE(tree); - } - else - { - JITDUMP("false\n"); - DEBUG_DESTROY_NODE(tree, op1); - tree = gtNewIconNode(0); - } + JITDUMP("false\n"); + tree = gtWrapWithSideEffects(gtNewIconNode(0), op1); } INDEBUG(tree->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED); return tree; From 9dec5cca00190324c09a4b31d645ea01692ed8e1 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 13 Feb 2024 13:38:43 +0100 Subject: [PATCH 02/12] Clean up --- src/coreclr/jit/assertionprop.cpp | 5 ++--- src/coreclr/jit/compiler.h | 6 +++--- src/coreclr/jit/gentree.cpp | 12 ++++++------ src/coreclr/jit/morph.cpp | 2 +- 4 files changed, 12 insertions(+), 13 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index c664b7330087c..cea3858061dc0 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -2664,7 +2664,7 @@ AssertionIndex Compiler::optAssertionIsSubtype(GenTree* tree, GenTree* methodTab } //------------------------------------------------------------------------------ -// optVNStrengthReductionOnTree: +// optVNStrengthReductionOnTree: Substitutes tree with a less expensive tree using VN // // Arguments: // block - The block containing the tree. @@ -2702,8 +2702,7 @@ GenTree* Compiler::optVNStrengthReductionOnTree(BasicBlock* block, GenTree* pare (funcApp.m_args[0] == clsArgVN)) { // The outer cast is redundant, remove it and preserve its side effects - // We do ignoreRoot here because the actual cast node is proven to never any exceptions - // (namely, InvalidCastException). + // We do ignoreRoot here because the actual cast node is proven to never throw any exceptions. return gtWrapWithSideEffects(objArg, call, GTF_ALL_EFFECT, true); } } diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 63f77db923b7d..74ecf6c8fb710 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -3516,9 +3516,9 @@ class Compiler bool ignoreRoot = false); GenTree* gtWrapWithSideEffects(GenTree* tree, - GenTree* sideEffectsSource, - GenTreeFlags sideEffectsFlags = GTF_ALL_EFFECT, - bool ignoreRoot = false); + GenTree* sideEffectsSource, + GenTreeFlags sideEffectsFlags = GTF_SIDE_EFFECT, + bool ignoreRoot = false); bool gtSplitTree( BasicBlock* block, Statement* stmt, GenTree* splitPoint, Statement** firstNewStmt, GenTree*** splitPointUse); diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index fc6dac23b2e06..bbc4b61f64872 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -17068,22 +17068,22 @@ bool Compiler::gtSplitTree( // and wraps the input tree with a COMMA node with them. // // Arguments: -// tree - the expression tree to wrap with side effects (if any) -// sideEffectSource - the expression tree to extract side effects from -// flags - side effect flags to be considered -// ignoreRoot - ignore side effects on the expression root node +// tree - the expression tree to wrap with side effects (if any) +// sideEffectsSource - the expression tree to extract side effects from +// flags - side effect flags to be considered +// ignoreRoot - ignore side effects on the expression root node // // Return Value: // The original tree wrapped with a COMMA node that contains the side effects // or just the tree itself if sideEffectSource has no side effects. // GenTree* Compiler::gtWrapWithSideEffects(GenTree* tree, - GenTree* sideEffectSource, + GenTree* sideEffectsSource, GenTreeFlags sideEffectsFlags, bool ignoreRoot) { GenTree* sideEffects = nullptr; - gtExtractSideEffList(sideEffectSource, &sideEffects, sideEffectsFlags, ignoreRoot); + gtExtractSideEffList(sideEffectsSource, &sideEffects, sideEffectsFlags, ignoreRoot); if (sideEffects != nullptr) { tree = gtNewOperNode(GT_COMMA, tree->TypeGet(), sideEffects, tree); diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index feb109f9b6877..ebf5244dedc43 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -9654,7 +9654,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac, bool* optA else { JITDUMP("false\n"); - tree = gtWrapWithSideEffects(gtNewIconNode(0), op1); + tree = gtWrapWithSideEffects(gtNewIconNode(0), op1, GTF_ALL_EFFECT); } INDEBUG(tree->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED); return tree; From 04a82ee34e86da3c52f4d813438a84f1136de610 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 13 Feb 2024 14:19:59 +0100 Subject: [PATCH 03/12] Clean up --- src/coreclr/jit/assertionprop.cpp | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index cea3858061dc0..8f38fc33c222d 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -5151,22 +5151,23 @@ GenTree* Compiler::optAssertionProp_Call(ASSERT_VALARG_TP assertions, GenTreeCal (helper == CORINFO_HELP_CHKCASTCLASS) || (helper == CORINFO_HELP_CHKCASTANY) || (helper == CORINFO_HELP_CHKCASTCLASS_SPECIAL)) { - GenTree* objArg = call->gtArgs.GetArgByIndex(1)->GetNode(); - GenTree* clsArg = call->gtArgs.GetArgByIndex(0)->GetNode(); + GenTree* arg1 = call->gtArgs.GetArgByIndex(1)->GetNode(); - if (objArg->gtOper != GT_LCL_VAR) + if (arg1->gtOper != GT_LCL_VAR) { return nullptr; } - unsigned index = optAssertionIsSubtype(objArg, clsArg, assertions); + GenTree* arg2 = call->gtArgs.GetArgByIndex(0)->GetNode(); + + unsigned index = optAssertionIsSubtype(arg1, arg2, assertions); if (index != NO_ASSERTION_INDEX) { JITDUMP("\nDid VN based subtype prop for index #%02u in " FMT_BB ":\n", index, compCurBB->bbNum); DISPTREE(call); - objArg = gtWrapWithSideEffects(objArg, call, GTF_SIDE_EFFECT, true); - return optAssertionProp_Update(objArg, call, stmt); + arg1 = gtWrapWithSideEffects(arg1, call, GTF_SIDE_EFFECT, true); + return optAssertionProp_Update(arg1, call, stmt); } // Leave a hint for fgLateCastExpansion that obj is never null. @@ -5174,7 +5175,7 @@ GenTree* Compiler::optAssertionProp_Call(ASSERT_VALARG_TP assertions, GenTreeCal INDEBUG(bool vnBased = false); // GTF_CALL_M_CAST_CAN_BE_EXPANDED check is to improve TP if (((call->gtCallMoreFlags & GTF_CALL_M_CAST_CAN_BE_EXPANDED) != 0) && - optAssertionIsNonNull(objArg, assertions DEBUGARG(&vnBased) DEBUGARG(&nonNullIdx))) + optAssertionIsNonNull(arg1, assertions DEBUGARG(&vnBased) DEBUGARG(&nonNullIdx))) { call->gtCallMoreFlags |= GTF_CALL_M_CAST_OBJ_NONNULL; return optAssertionProp_Update(call, call, stmt); From fae76476c2a80f867173c4312712a98a823de6c8 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Tue, 13 Feb 2024 14:22:37 +0100 Subject: [PATCH 04/12] Update assertionprop.cpp --- src/coreclr/jit/assertionprop.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index 8f38fc33c222d..060130193eda4 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -5152,7 +5152,6 @@ GenTree* Compiler::optAssertionProp_Call(ASSERT_VALARG_TP assertions, GenTreeCal (helper == CORINFO_HELP_CHKCASTCLASS_SPECIAL)) { GenTree* arg1 = call->gtArgs.GetArgByIndex(1)->GetNode(); - if (arg1->gtOper != GT_LCL_VAR) { return nullptr; From 76081b3dacd7cfb476db0676c40846f0e156630f Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 13 Feb 2024 14:30:42 +0100 Subject: [PATCH 05/12] Clean up --- src/coreclr/jit/assertionprop.cpp | 23 +++++++++-------------- src/coreclr/jit/compiler.h | 2 +- 2 files changed, 10 insertions(+), 15 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index 060130193eda4..7946209a06b7a 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -6343,8 +6343,8 @@ GenTree* Compiler::optVNConstantPropOnJTrue(BasicBlock* block, GenTree* test) } //------------------------------------------------------------------------------ -// optVNConstantPropCurStmt -// Performs constant prop on the current statement's tree nodes. +// optVNStrengthReductionCurStmt: Performs strength reduction (including constant propagation) +// on the current statement's tree nodes using VN. // // Assumption: // This function is called as part of a post-order tree walk. @@ -6358,17 +6358,12 @@ GenTree* Compiler::optVNConstantPropOnJTrue(BasicBlock* block, GenTree* test) // Return Value: // Returns the standard visitor walk result. // -// Description: -// Checks if a node is an R-value and evaluates to a constant. If the node -// evaluates to constant, then the tree is replaced by its side effects and -// the constant node. -// -Compiler::fgWalkResult Compiler::optVNConstantPropCurStmt(BasicBlock* block, - Statement* stmt, - GenTree* parent, - GenTree* tree) +Compiler::fgWalkResult Compiler::optVNStrengthReductionCurStmt(BasicBlock* block, + Statement* stmt, + GenTree* parent, + GenTree* tree) { - // Don't perform const prop on expressions marked with GTF_DONT_CSE + // Don't perform strength reduction on expressions marked with GTF_DONT_CSE // TODO-ASG: delete. if (!tree->CanCSE()) { @@ -6456,7 +6451,7 @@ Compiler::fgWalkResult Compiler::optVNConstantPropCurStmt(BasicBlock* block, return WALK_CONTINUE; } - // Perform the constant propagation + // Perform the constant propagation first GenTree* newTree = optVNConstantPropOnTree(block, parent, tree); if (newTree == nullptr) @@ -6544,7 +6539,7 @@ Compiler::fgWalkResult Compiler::optVNAssertionPropCurStmtVisitor(GenTree** ppTr pThis->optVnNonNullPropCurStmt(pData->block, pData->stmt, *ppTree); - return pThis->optVNConstantPropCurStmt(pData->block, pData->stmt, data->parent, *ppTree); + return pThis->optVNStrengthReductionCurStmt(pData->block, pData->stmt, data->parent, *ppTree); } /***************************************************************************** diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 74ecf6c8fb710..e5da7f13cce5f 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -7715,7 +7715,7 @@ class Compiler public: void optVnNonNullPropCurStmt(BasicBlock* block, Statement* stmt, GenTree* tree); - fgWalkResult optVNConstantPropCurStmt(BasicBlock* block, Statement* stmt, GenTree* parent, GenTree* tree); + fgWalkResult optVNStrengthReductionCurStmt(BasicBlock* block, Statement* stmt, GenTree* parent, GenTree* tree); GenTree* optVNConstantPropOnJTrue(BasicBlock* block, GenTree* test); GenTree* optVNConstantPropOnTree(BasicBlock* block, GenTree* parent, GenTree* tree); GenTree* optVNStrengthReductionOnTree(BasicBlock* block, GenTree* parent, GenTree* tree); From d5a7088d59a4c6312f660f9e5c2f188839ceb07e Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 13 Feb 2024 15:47:25 +0100 Subject: [PATCH 06/12] Address feedback --- src/coreclr/jit/assertionprop.cpp | 129 ++++++++++++++++++------------ src/coreclr/jit/compiler.h | 7 +- 2 files changed, 83 insertions(+), 53 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index 7946209a06b7a..78f5c177152e0 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -2664,60 +2664,95 @@ AssertionIndex Compiler::optAssertionIsSubtype(GenTree* tree, GenTree* methodTab } //------------------------------------------------------------------------------ -// optVNStrengthReductionOnTree: Substitutes tree with a less expensive tree using VN +// optVNBasedFoldExpr_Call: Folds given call using VN to a simpler tree. // // Arguments: // block - The block containing the tree. // parent - The parent node of the tree. -// tree - The tree node +// call - The call to fold // // Return Value: // Returns a new tree or nullptr if nothing is changed. // -GenTree* Compiler::optVNStrengthReductionOnTree(BasicBlock* block, GenTree* parent, GenTree* tree) +GenTree* Compiler::optVNBasedFoldExpr_Call(BasicBlock* block, GenTree* parent, GenTreeCall* call) { - if (tree->IsHelperCall()) - { - GenTreeCall* call = tree->AsCall(); - switch (call->GetHelperNum()) - { - // Fold "cast(cast(obj, cls), cls)" to "cast(obj, cls)" - case CORINFO_HELP_CHKCASTARRAY: - case CORINFO_HELP_CHKCASTANY: - case CORINFO_HELP_CHKCASTINTERFACE: - case CORINFO_HELP_CHKCASTCLASS: - case CORINFO_HELP_ISINSTANCEOFARRAY: - case CORINFO_HELP_ISINSTANCEOFCLASS: - case CORINFO_HELP_ISINSTANCEOFANY: - case CORINFO_HELP_ISINSTANCEOFINTERFACE: - { - GenTree* clsArg = call->gtArgs.GetUserArgByIndex(0)->GetNode(); - GenTree* objArg = call->gtArgs.GetUserArgByIndex(1)->GetNode(); - ValueNum clsArgVN = clsArg->gtVNPair.GetConservative(); - ValueNum objArgVN = objArg->gtVNPair.GetConservative(); - - VNFuncApp funcApp; - if (vnStore->GetVNFunc(objArgVN, &funcApp) && - ((funcApp.m_func == VNF_CastClass) || (funcApp.m_func == VNF_IsInstanceOf)) && - (funcApp.m_args[0] == clsArgVN)) + switch (call->GetHelperNum()) + { + // + // Fold "CAST(CAST(obj, cls), cls)" to "CAST(obj, cls)" + // where CAST is either ISINST or CASTCLASS. + // + case CORINFO_HELP_CHKCASTARRAY: + case CORINFO_HELP_CHKCASTANY: + case CORINFO_HELP_CHKCASTINTERFACE: + case CORINFO_HELP_CHKCASTCLASS: + case CORINFO_HELP_ISINSTANCEOFARRAY: + case CORINFO_HELP_ISINSTANCEOFCLASS: + case CORINFO_HELP_ISINSTANCEOFANY: + case CORINFO_HELP_ISINSTANCEOFINTERFACE: + { + GenTree* castClsArg = call->gtArgs.GetUserArgByIndex(0)->GetNode(); + GenTree* castObjArg = call->gtArgs.GetUserArgByIndex(1)->GetNode(); + ValueNum castClsArgVN = castClsArg->gtVNPair.GetConservative(); + ValueNum castObjArgVN = castObjArg->gtVNPair.GetConservative(); + + VNFuncApp funcApp; + if (vnStore->GetVNFunc(castObjArgVN, &funcApp) && + ((funcApp.m_func == VNF_CastClass) || (funcApp.m_func == VNF_IsInstanceOf))) + { + ValueNum innerCastClsVN = funcApp.m_args[0]; + if (innerCastClsVN == castClsArgVN) { // The outer cast is redundant, remove it and preserve its side effects // We do ignoreRoot here because the actual cast node is proven to never throw any exceptions. - return gtWrapWithSideEffects(objArg, call, GTF_ALL_EFFECT, true); + return gtWrapWithSideEffects(castObjArg, call, GTF_ALL_EFFECT, true); } } + } + break; + + default: break; + } - default: - break; - } + return nullptr; +} + +//------------------------------------------------------------------------------ +// optVNBasedFoldExpr: Folds given tree using VN to a constant or a simpler tree. +// +// Arguments: +// block - The block containing the tree. +// parent - The parent node of the tree. +// tree - The tree to fold. +// +// Return Value: +// Returns a new tree or nullptr if nothing is changed. +// +GenTree* Compiler::optVNBasedFoldExpr(BasicBlock* block, GenTree* parent, GenTree* tree) +{ + // First, attempt to fold it to a constant if possible. + GenTree* foldedToCns = optVNBasedFoldConstExpr(block, parent, tree); + if (foldedToCns != nullptr) + { + return foldedToCns; } + switch (tree->OperGet()) + { + case GT_CALL: + return optVNBasedFoldExpr_Call(block, parent, tree->AsCall()); + + // We can add more VN-based foldings here. + + default: + break; + } return nullptr; } //------------------------------------------------------------------------------ -// optVNConstantPropOnTree: Substitutes tree with an evaluated constant while +// optVNBasedFoldConstExpr: Substitutes tree with an evaluated constant while // managing side-effects. // // Arguments: @@ -2744,7 +2779,7 @@ GenTree* Compiler::optVNStrengthReductionOnTree(BasicBlock* block, GenTree* pare // the relop will evaluate to "true" or "false" statically, then the side-effects // will be put into new statements, presuming the JTrue will be folded away. // -GenTree* Compiler::optVNConstantPropOnTree(BasicBlock* block, GenTree* parent, GenTree* tree) +GenTree* Compiler::optVNBasedFoldConstExpr(BasicBlock* block, GenTree* parent, GenTree* tree) { if (tree->OperGet() == GT_JTRUE) { @@ -6343,7 +6378,7 @@ GenTree* Compiler::optVNConstantPropOnJTrue(BasicBlock* block, GenTree* test) } //------------------------------------------------------------------------------ -// optVNStrengthReductionCurStmt: Performs strength reduction (including constant propagation) +// optVNBasedFoldCurStmt: Performs VN-based folding // on the current statement's tree nodes using VN. // // Assumption: @@ -6358,10 +6393,10 @@ GenTree* Compiler::optVNConstantPropOnJTrue(BasicBlock* block, GenTree* test) // Return Value: // Returns the standard visitor walk result. // -Compiler::fgWalkResult Compiler::optVNStrengthReductionCurStmt(BasicBlock* block, - Statement* stmt, - GenTree* parent, - GenTree* tree) +Compiler::fgWalkResult Compiler::optVNBasedFoldCurStmt(BasicBlock* block, + Statement* stmt, + GenTree* parent, + GenTree* tree) { // Don't perform strength reduction on expressions marked with GTF_DONT_CSE // TODO-ASG: delete. @@ -6451,19 +6486,13 @@ Compiler::fgWalkResult Compiler::optVNStrengthReductionCurStmt(BasicBlock* block return WALK_CONTINUE; } - // Perform the constant propagation first - GenTree* newTree = optVNConstantPropOnTree(block, parent, tree); + // Perform the VN-based folding: + GenTree* newTree = optVNBasedFoldExpr(block, parent, tree); if (newTree == nullptr) { - // If it wasn't a constant, let's see if we can reduce the strength - // of the tree using VN. - newTree = optVNStrengthReductionOnTree(block, parent, tree); - if (newTree == nullptr) - { - // Not propagated, keep going. - return WALK_CONTINUE; - } + // Not propagated, keep going. + return WALK_CONTINUE; } // TODO https://github.com/dotnet/runtime/issues/10450: @@ -6471,7 +6500,7 @@ Compiler::fgWalkResult Compiler::optVNStrengthReductionCurStmt(BasicBlock* block optAssertionProp_Update(newTree, tree, stmt); - JITDUMP("After constant propagation on [%06u]:\n", tree->gtTreeID); + JITDUMP("After VN-based fold of [%06u]:\n", tree->gtTreeID); DBEXEC(VERBOSE, gtDispStmt(stmt)); return WALK_CONTINUE; @@ -6539,7 +6568,7 @@ Compiler::fgWalkResult Compiler::optVNAssertionPropCurStmtVisitor(GenTree** ppTr pThis->optVnNonNullPropCurStmt(pData->block, pData->stmt, *ppTree); - return pThis->optVNStrengthReductionCurStmt(pData->block, pData->stmt, data->parent, *ppTree); + return pThis->optVNBasedFoldCurStmt(pData->block, pData->stmt, data->parent, *ppTree); } /***************************************************************************** diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index e5da7f13cce5f..a4efc3e0c61b6 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -7715,10 +7715,11 @@ class Compiler public: void optVnNonNullPropCurStmt(BasicBlock* block, Statement* stmt, GenTree* tree); - fgWalkResult optVNStrengthReductionCurStmt(BasicBlock* block, Statement* stmt, GenTree* parent, GenTree* tree); + fgWalkResult optVNBasedFoldCurStmt(BasicBlock* block, Statement* stmt, GenTree* parent, GenTree* tree); GenTree* optVNConstantPropOnJTrue(BasicBlock* block, GenTree* test); - GenTree* optVNConstantPropOnTree(BasicBlock* block, GenTree* parent, GenTree* tree); - GenTree* optVNStrengthReductionOnTree(BasicBlock* block, GenTree* parent, GenTree* tree); + GenTree* optVNBasedFoldConstExpr(BasicBlock* block, GenTree* parent, GenTree* tree); + GenTree* optVNBasedFoldExpr(BasicBlock* block, GenTree* parent, GenTree* tree); + GenTree* optVNBasedFoldExpr_Call(BasicBlock* block, GenTree* parent, GenTreeCall* call); GenTree* optExtractSideEffListFromConst(GenTree* tree); AssertionIndex GetAssertionCount() From 80a8e2c5dd23604d9d540b1468132eb823832d33 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Wed, 14 Feb 2024 10:42:43 +0100 Subject: [PATCH 07/12] Apply suggestions from code review Co-authored-by: Andy Ayers --- src/coreclr/jit/assertionprop.cpp | 2 +- src/coreclr/jit/gentree.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index 78f5c177152e0..b04a3a057776a 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -6398,7 +6398,7 @@ Compiler::fgWalkResult Compiler::optVNBasedFoldCurStmt(BasicBlock* block, GenTree* parent, GenTree* tree) { - // Don't perform strength reduction on expressions marked with GTF_DONT_CSE + // Don't try and fold expressions marked with GTF_DONT_CSE // TODO-ASG: delete. if (!tree->CanCSE()) { diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index bbc4b61f64872..65d0f5ef69b49 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -17064,7 +17064,7 @@ bool Compiler::gtSplitTree( } //------------------------------------------------------------------------ -// gtExtractSideEffList: Extracts side effects from sideEffectSource (if any) +// gWrapWithSicdeEffects: Extracts side effects from sideEffectSource (if any) // and wraps the input tree with a COMMA node with them. // // Arguments: From 395c103287b60b81951ad7bf8ee9027faff3c002 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Wed, 14 Feb 2024 12:11:29 +0100 Subject: [PATCH 08/12] Cleanup --- src/coreclr/jit/gentree.cpp | 4 ++-- src/coreclr/jit/valuenum.cpp | 7 +++++++ 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 65d0f5ef69b49..7fcd48f3df95b 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -17064,13 +17064,13 @@ bool Compiler::gtSplitTree( } //------------------------------------------------------------------------ -// gWrapWithSicdeEffects: Extracts side effects from sideEffectSource (if any) +// gtWrapWithSideEffects: Extracts side effects from sideEffectSource (if any) // and wraps the input tree with a COMMA node with them. // // Arguments: // tree - the expression tree to wrap with side effects (if any) // sideEffectsSource - the expression tree to extract side effects from -// flags - side effect flags to be considered +// sideEffectsFlags - side effect flags to be considered // ignoreRoot - ignore side effects on the expression root node // // Return Value: diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 8bb7c308207af..977e984865aac 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -13098,6 +13098,13 @@ bool Compiler::fgValueNumberHelperCall(GenTreeCall* call) vnStore->VNPairForFunc(TYP_REF, VNF_OverflowExc, vnStore->VNPForVoid())); break; + case CORINFO_HELP_CHKCASTINTERFACE: + case CORINFO_HELP_CHKCASTARRAY: + case CORINFO_HELP_CHKCASTCLASS: + case CORINFO_HELP_CHKCASTANY: + // InvalidCastExc for these is set in VNForCast + break; + default: // Setup vnpExc with the information that multiple different exceptions // could be generated by this helper From c9bc1f41807bca116da10e37e389a8557a771e07 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Wed, 14 Feb 2024 12:33:23 +0100 Subject: [PATCH 09/12] add a comment --- src/coreclr/jit/assertionprop.cpp | 8 ++++++++ src/coreclr/jit/gentree.cpp | 2 ++ 2 files changed, 10 insertions(+) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index b04a3a057776a..14d7f5df80092 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -2696,6 +2696,14 @@ GenTree* Compiler::optVNBasedFoldExpr_Call(BasicBlock* block, GenTree* parent, G ValueNum castClsArgVN = castClsArg->gtVNPair.GetConservative(); ValueNum castObjArgVN = castObjArg->gtVNPair.GetConservative(); + if ((castObjArg->gtFlags & GTF_ALL_EFFECT) != 0) + { + // It won't be trivial to properly extract side-effects from the call node. + // Ideally, we only need side effects from the castClsArg argument as the call itself + // won't throw any exceptions. But we should not forget about the EarlyNode (setup args) + return nullptr; + } + VNFuncApp funcApp; if (vnStore->GetVNFunc(castObjArgVN, &funcApp) && ((funcApp.m_func == VNF_CastClass) || (funcApp.m_func == VNF_IsInstanceOf))) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 7fcd48f3df95b..52b6c7486f40f 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -17069,6 +17069,8 @@ bool Compiler::gtSplitTree( // // Arguments: // tree - the expression tree to wrap with side effects (if any) +// it has to be either a side effect free node of sideEffectsSource +// or any tree outside sideEffectsSource's hierarchy // sideEffectsSource - the expression tree to extract side effects from // sideEffectsFlags - side effect flags to be considered // ignoreRoot - ignore side effects on the expression root node From 47a87d65972a7d8e8025f8e7d13c60d5659e0e09 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Wed, 14 Feb 2024 12:41:43 +0100 Subject: [PATCH 10/12] add a todo --- src/coreclr/jit/gentree.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 52b6c7486f40f..6ece1a2c90dd9 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -17069,7 +17069,7 @@ bool Compiler::gtSplitTree( // // Arguments: // tree - the expression tree to wrap with side effects (if any) -// it has to be either a side effect free node of sideEffectsSource +// it has to be either a side effect free subnode of sideEffectsSource // or any tree outside sideEffectsSource's hierarchy // sideEffectsSource - the expression tree to extract side effects from // sideEffectsFlags - side effect flags to be considered @@ -17088,6 +17088,10 @@ GenTree* Compiler::gtWrapWithSideEffects(GenTree* tree, gtExtractSideEffList(sideEffectsSource, &sideEffects, sideEffectsFlags, ignoreRoot); if (sideEffects != nullptr) { + // TODO: assert if tree is a subnode of sideEffectsSource and the tree has its own side effects + // otherwise the resulting COMMA might have some side effects to be duplicated + // For now, caller is responsible for ensuring that will not happen. + tree = gtNewOperNode(GT_COMMA, tree->TypeGet(), sideEffects, tree); if (fgNodeThreading == NodeThreading::AllTrees) { From a46c81159c233388cb2bdc57a70cc838a653273f Mon Sep 17 00:00:00 2001 From: EgorBo Date: Wed, 14 Feb 2024 12:45:22 +0100 Subject: [PATCH 11/12] improve comment --- src/coreclr/jit/gentree.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 6ece1a2c90dd9..9775395febbb0 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -17090,7 +17090,8 @@ GenTree* Compiler::gtWrapWithSideEffects(GenTree* tree, { // TODO: assert if tree is a subnode of sideEffectsSource and the tree has its own side effects // otherwise the resulting COMMA might have some side effects to be duplicated - // For now, caller is responsible for ensuring that will not happen. + // It should be possible to be smarter here and allow such cases by extracting the side effects + // properly for this particular case. For now, caller is responsible for avoiding such cases. tree = gtNewOperNode(GT_COMMA, tree->TypeGet(), sideEffects, tree); if (fgNodeThreading == NodeThreading::AllTrees) From b4ab4c5a0027e711ad2f1e3df41bbd6e66b38379 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Wed, 14 Feb 2024 13:12:53 +0100 Subject: [PATCH 12/12] Address feedback --- src/coreclr/jit/assertionprop.cpp | 11 ++++++----- src/coreclr/jit/gentree.cpp | 4 ---- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index 14d7f5df80092..fb317faea42c1 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -2679,7 +2679,7 @@ GenTree* Compiler::optVNBasedFoldExpr_Call(BasicBlock* block, GenTree* parent, G switch (call->GetHelperNum()) { // - // Fold "CAST(CAST(obj, cls), cls)" to "CAST(obj, cls)" + // Fold "CAST(IsInstanceOf(obj, cls), cls)" to "IsInstanceOf(obj, cls)" // where CAST is either ISINST or CASTCLASS. // case CORINFO_HELP_CHKCASTARRAY: @@ -2705,15 +2705,16 @@ GenTree* Compiler::optVNBasedFoldExpr_Call(BasicBlock* block, GenTree* parent, G } VNFuncApp funcApp; - if (vnStore->GetVNFunc(castObjArgVN, &funcApp) && - ((funcApp.m_func == VNF_CastClass) || (funcApp.m_func == VNF_IsInstanceOf))) + if (vnStore->GetVNFunc(castObjArgVN, &funcApp) && (funcApp.m_func == VNF_IsInstanceOf)) { ValueNum innerCastClsVN = funcApp.m_args[0]; if (innerCastClsVN == castClsArgVN) { // The outer cast is redundant, remove it and preserve its side effects - // We do ignoreRoot here because the actual cast node is proven to never throw any exceptions. - return gtWrapWithSideEffects(castObjArg, call, GTF_ALL_EFFECT, true); + // We do ignoreRoot here because the actual cast node never throws any exceptions. + GenTree* result = gtWrapWithSideEffects(castObjArg, call, GTF_ALL_EFFECT, true); + fgSetTreeSeq(result); + return result; } } } diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 9775395febbb0..22336da17625d 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -17094,10 +17094,6 @@ GenTree* Compiler::gtWrapWithSideEffects(GenTree* tree, // properly for this particular case. For now, caller is responsible for avoiding such cases. tree = gtNewOperNode(GT_COMMA, tree->TypeGet(), sideEffects, tree); - if (fgNodeThreading == NodeThreading::AllTrees) - { - fgSetTreeSeq(tree); - } } return tree; }