From 9f755be357e66bb97fc1d34caa3b11375cf97600 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Sat, 11 Feb 2023 16:45:54 +0100 Subject: [PATCH 1/8] JIT: Handle some "field offset computation" patterns Both during import and during VN. Just want to see TP impact, not sure if this is worth it. --- src/coreclr/jit/compiler.h | 3 +- src/coreclr/jit/importer.cpp | 100 +++++++++++++++++++++++++----- src/coreclr/jit/importercalls.cpp | 2 +- src/coreclr/jit/valuenum.cpp | 10 +++ 4 files changed, 99 insertions(+), 16 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 55d4ae161e92e9..a04db97d49db54 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -4000,6 +4000,7 @@ class Compiler GenTree* impNormStructVal(GenTree* structVal, CORINFO_CLASS_HANDLE structHnd, unsigned curLevel); + GenTree* impFoldSubOfLocalOffsets(GenTree* op1, GenTree* op2); GenTree* impTokenToHandle(CORINFO_RESOLVED_TOKEN* pResolvedToken, bool* pRuntimeLookup = nullptr, bool mustRestoreHandle = false, @@ -4281,7 +4282,7 @@ class Compiler #endif // DEBUG static bool impIsInvariant(const GenTree* tree); - static bool impIsAddressInLocal(const GenTree* tree, GenTree** lclVarTreeOut = nullptr); + static bool impIsAddressInLocal(const GenTree* tree, unsigned* lclNum = nullptr, unsigned* lclOffs = nullptr); void impMakeDiscretionaryInlineObservations(InlineInfo* pInlineInfo, InlineResult* inlineResult); diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 5f671605558497..c5fd0d1fe7ce6e 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -1295,6 +1295,47 @@ var_types Compiler::impNormStructType(CORINFO_CLASS_HANDLE structHnd, CorInfoTyp return structType; } +//------------------------------------------------------------------------ +// Compiler::impFoldSubOfLocalOffsets: Recognize and fold an offset +// computation for a local's field. +// +// Arguments: +// op1 - The first op being subtracted +// op2 - The second op being subtracted +// +// Returns: +// Null if the pattern was not recognized; otherwise a constant +// representing the offset. +// +GenTree* Compiler::impFoldSubOfLocalOffsets(GenTree* op1, GenTree* op2) +{ + if (!op1->TypeIs(TYP_I_IMPL, TYP_BYREF) || !op2->TypeIs(TYP_I_IMPL, TYP_BYREF) || op1->TypeGet() != op2->TypeGet()) + { + return nullptr; + } + + unsigned op1Lcl; + unsigned op1Offs; + if (!impIsAddressInLocal(op1, &op1Lcl, &op1Offs)) + { + return nullptr; + } + + unsigned op2Lcl; + unsigned op2Offs; + if (!impIsAddressInLocal(op2, &op2Lcl, &op2Offs)) + { + return nullptr; + } + + if (op1Lcl != op2Lcl) + { + return nullptr; + } + + return gtNewIconNode((ssize_t)op1Offs - (ssize_t)op2Offs, TYP_I_IMPL); +} + //------------------------------------------------------------------------ // Compiler::impNormStructVal: Normalize a struct value // @@ -6956,7 +6997,7 @@ void Compiler::impImportBlockCode(BasicBlock* block) lclNum = impInlineFetchLocal(lclNum DEBUGARG("Inline ldloca(s) first use temp")); assert(!lvaGetDesc(lclNum)->lvNormalizeOnLoad()); - op1 = gtNewLclVarAddrNode(lclNum, TYP_BYREF); + op1 = gtNewLclVarAddrNode(lclNum, lvaIsImplicitByRefLocal(lclNum) ? TYP_BYREF : TYP_I_IMPL); goto _PUSH_ADRVAR; } @@ -7009,7 +7050,7 @@ void Compiler::impImportBlockCode(BasicBlock* block) // where it was not absolutely needed, but doing otherwise would // require careful rethinking of the importer routines which use // the IL validity model (e. g. "impGetByRefResultType"). - op1 = gtNewLclVarAddrNode(lclNum, TYP_BYREF); + op1 = gtNewLclVarAddrNode(lclNum, lvaIsImplicitByRefLocal(lclNum) ? TYP_BYREF : TYP_I_IMPL); _PUSH_ADRVAR: assert(op1->OperIs(GT_LCL_VAR_ADDR)); @@ -7409,8 +7450,31 @@ void Compiler::impImportBlockCode(BasicBlock* block) goto MATH_OP2_FLAGS; case CEE_SUB: - oper = GT_SUB; - goto MATH_OP2; + if (opts.OptimizationEnabled()) + { + op2 = impPopStack().val; + op1 = impPopStack().val; + + GenTree* folded = nullptr; // impFoldSubOfLocalOffsets(op1, op2); + + if (folded != nullptr) + { + impPushOnStack(folded, tiRetVal); + break; + } + else + { + oper = GT_SUB; + ovfl = false; + callNode = false; + goto MATH_OP2_WITH_OPS; + } + } + else + { + oper = GT_SUB; + goto MATH_OP2; + } case CEE_SUB_OVF: uns = false; @@ -7493,6 +7557,8 @@ void Compiler::impImportBlockCode(BasicBlock* block) op2 = impPopStack().val; op1 = impPopStack().val; + MATH_OP2_WITH_OPS: + /* Can't do arithmetic with references */ assertImp(genActualType(op1->TypeGet()) != TYP_REF && genActualType(op2->TypeGet()) != TYP_REF); @@ -12770,7 +12836,8 @@ bool Compiler::impIsInvariant(const GenTree* tree) // the address of a field in a local. // Arguments: // tree -- The tree -// lclVarTreeOut -- [out] the local that this points into +// lclNum -- [out, optional] the local that this points into +// lclOffs -- [out, optional] the offset into the local // // Returns: // true if it points into a local @@ -12779,21 +12846,27 @@ bool Compiler::impIsInvariant(const GenTree* tree) // This is a variant of GenTree::IsLocalAddrExpr that is more suitable for // use during import. Unlike that function, this one handles field nodes. // -bool Compiler::impIsAddressInLocal(const GenTree* tree, GenTree** lclVarTreeOut) +bool Compiler::impIsAddressInLocal(const GenTree* tree, unsigned* lclNum, unsigned* lclOffs) { - const GenTree* op = tree; + unsigned offs = 0; + const GenTree* op = tree; while (op->OperIs(GT_FIELD_ADDR) && op->AsField()->IsInstance()) { + offs += op->AsField()->gtFldOffset; op = op->AsField()->GetFldObj(); } - if (op->OperIs(GT_LCL_VAR_ADDR)) + if (op->OperIs(GT_LCL_VAR_ADDR, GT_LCL_FLD_ADDR)) { - if (lclVarTreeOut != nullptr) + if (lclNum != nullptr) { - *lclVarTreeOut = const_cast(op); + *lclNum = op->AsLclVarCommon()->GetLclNum(); } + if (lclOffs != nullptr) + { + *lclOffs = offs + op->AsLclVarCommon()->GetLclOffs(); + } return true; } @@ -13168,11 +13241,10 @@ void Compiler::impInlineRecordArgInfo(InlineInfo* pInlineInfo, return; } - GenTree* lclVarTree; - const bool isAddressInLocal = impIsAddressInLocal(curArgVal, &lclVarTree); - if (isAddressInLocal) + unsigned lclNum; + if (impIsAddressInLocal(curArgVal, &lclNum)) { - LclVarDsc* varDsc = lvaGetDesc(lclVarTree->AsLclVarCommon()); + LclVarDsc* varDsc = lvaGetDesc(lclNum); if (varTypeIsStruct(varDsc)) { diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index b2a2d46892aafa..db6605364954d3 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -2823,7 +2823,7 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis, GenTree* indexClone = nullptr; GenTree* ptrToSpanClone = nullptr; assert(genActualType(index) == TYP_INT); - assert(ptrToSpan->TypeGet() == TYP_BYREF); + assert(ptrToSpan->TypeIs(TYP_I_IMPL, TYP_BYREF)); #if defined(DEBUG) if (verbose) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 62c6735a5c4eec..f858ccd60ca31a 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -4239,6 +4239,7 @@ ValueNum ValueNumStore::EvalUsingMathIdentity(var_types typ, VNFunc func, ValueN // (x - 0) == x // (x - x) == 0 // This identity does not apply for floating point (when x == -0.0). + // (x + a) - x == a auto identityForSubtraction = [=]() -> ValueNum { if (!varTypeIsFloating(typ)) { @@ -4251,6 +4252,15 @@ ValueNum ValueNumStore::EvalUsingMathIdentity(var_types typ, VNFunc func, ValueN { return ZeroVN; } + + VNFuncApp add; + if (GetVNFunc(arg0VN, &add) && (add.m_func == VNFunc(GT_ADD))) + { + if (add.m_args[0] == arg1VN) + return add.m_args[1]; + if (add.m_args[1] == arg1VN) + return add.m_args[0]; + } } return NoVN; From df7350102aebd5bf6367418ccbcdfd5b17afee9c Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Sat, 11 Feb 2023 16:47:45 +0100 Subject: [PATCH 2/8] Oops --- src/coreclr/jit/importer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index c5fd0d1fe7ce6e..70961a7b424d02 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -7455,7 +7455,7 @@ void Compiler::impImportBlockCode(BasicBlock* block) op2 = impPopStack().val; op1 = impPopStack().val; - GenTree* folded = nullptr; // impFoldSubOfLocalOffsets(op1, op2); + GenTree* folded = impFoldSubOfLocalOffsets(op1, op2); if (folded != nullptr) { From 8f9c4f2998a39446b1b7268fefa51046f95035c4 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 31 Mar 2023 11:32:23 +0200 Subject: [PATCH 3/8] Fix wrong identity for overflowing SUBs --- src/coreclr/jit/importer.cpp | 5 +++-- src/coreclr/jit/valuenum.cpp | 23 +++++++++++++---------- 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index eacd12022adac9..db837aa9203e22 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -1308,7 +1308,8 @@ var_types Compiler::impNormStructType(CORINFO_CLASS_HANDLE structHnd, CorInfoTyp // GenTree* Compiler::impFoldSubOfLocalOffsets(GenTree* op1, GenTree* op2) { - if (!op1->TypeIs(TYP_I_IMPL, TYP_BYREF) || !op2->TypeIs(TYP_I_IMPL, TYP_BYREF) || op1->TypeGet() != op2->TypeGet()) + if (!op1->TypeIs(TYP_I_IMPL, TYP_BYREF) || !op2->TypeIs(TYP_I_IMPL, TYP_BYREF) || + (op1->TypeGet() != op2->TypeGet())) { return nullptr; } @@ -12846,7 +12847,7 @@ bool Compiler::impIsInvariant(const GenTree* tree) // Returns: // true if it points into a local // -bool Compiler::impIsAddressInLocal(const GenTree* tree, GenTree** lclVarTreeOut) +bool Compiler::impIsAddressInLocal(const GenTree* tree, unsigned* lclNum, unsigned* lclOffs) { unsigned offs = 0; const GenTree* op = tree; diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 1e79a89e7f65dd..a0fa6ba66784cb 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -4450,8 +4450,7 @@ ValueNum ValueNumStore::EvalUsingMathIdentity(var_types typ, VNFunc func, ValueN // (x - 0) == x // (x - x) == 0 // This identity does not apply for floating point (when x == -0.0). - // (x + a) - x == a - auto identityForSubtraction = [=]() -> ValueNum { + auto identityForSubtraction = [=](bool ovf) -> ValueNum { if (!varTypeIsFloating(typ)) { ValueNum ZeroVN = VNZeroForType(typ); @@ -4464,13 +4463,17 @@ ValueNum ValueNumStore::EvalUsingMathIdentity(var_types typ, VNFunc func, ValueN return ZeroVN; } - VNFuncApp add; - if (GetVNFunc(arg0VN, &add) && (add.m_func == VNFunc(GT_ADD))) + if (!ovf) { - if (add.m_args[0] == arg1VN) - return add.m_args[1]; - if (add.m_args[1] == arg1VN) - return add.m_args[0]; + // (x + a) - x == a + VNFuncApp add; + if ((resultVN == NoVN) && GetVNFunc(arg0VN, &add) && (add.m_func == (VNFunc)GT_ADD)) + { + if (add.m_args[0] == arg1VN) + return add.m_args[1]; + if (add.m_args[1] == arg1VN) + return add.m_args[0]; + } } } @@ -4525,7 +4528,7 @@ ValueNum ValueNumStore::EvalUsingMathIdentity(var_types typ, VNFunc func, ValueN break; case GT_SUB: - resultVN = identityForSubtraction(); + resultVN = identityForSubtraction(false); break; case GT_MUL: @@ -4800,7 +4803,7 @@ ValueNum ValueNumStore::EvalUsingMathIdentity(var_types typ, VNFunc func, ValueN case VNF_SUB_OVF: case VNF_SUB_UN_OVF: - resultVN = identityForSubtraction(); + resultVN = identityForSubtraction(true); break; case VNF_MUL_OVF: From b944901ed3e59098dc8f57e64256f4d90e6b00b5 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 31 Mar 2023 12:15:00 +0200 Subject: [PATCH 4/8] Handle in local morph instead; revert previous changes --- src/coreclr/jit/compiler.h | 3 +- src/coreclr/jit/importer.cpp | 102 ++++-------------------------- src/coreclr/jit/importercalls.cpp | 2 +- src/coreclr/jit/lclmorph.cpp | 27 ++++++++ src/coreclr/jit/valuenum.cpp | 2 +- 5 files changed, 43 insertions(+), 93 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 040d155744a7be..f92b51c542ca3d 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -4059,7 +4059,6 @@ class Compiler GenTree* impNormStructVal(GenTree* structVal, CORINFO_CLASS_HANDLE structHnd, unsigned curLevel); - GenTree* impFoldSubOfLocalOffsets(GenTree* op1, GenTree* op2); GenTree* impTokenToHandle(CORINFO_RESOLVED_TOKEN* pResolvedToken, bool* pRuntimeLookup = nullptr, bool mustRestoreHandle = false, @@ -4341,7 +4340,7 @@ class Compiler #endif // DEBUG static bool impIsInvariant(const GenTree* tree); - static bool impIsAddressInLocal(const GenTree* tree, unsigned* lclNum = nullptr, unsigned* lclOffs = nullptr); + static bool impIsAddressInLocal(const GenTree* tree, GenTree** lclVarTreeOut = nullptr); void impMakeDiscretionaryInlineObservations(InlineInfo* pInlineInfo, InlineResult* inlineResult); diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index db837aa9203e22..27169fc89b352a 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -1294,48 +1294,6 @@ var_types Compiler::impNormStructType(CORINFO_CLASS_HANDLE structHnd, CorInfoTyp return structType; } -//------------------------------------------------------------------------ -// Compiler::impFoldSubOfLocalOffsets: Recognize and fold an offset -// computation for a local's field. -// -// Arguments: -// op1 - The first op being subtracted -// op2 - The second op being subtracted -// -// Returns: -// Null if the pattern was not recognized; otherwise a constant -// representing the offset. -// -GenTree* Compiler::impFoldSubOfLocalOffsets(GenTree* op1, GenTree* op2) -{ - if (!op1->TypeIs(TYP_I_IMPL, TYP_BYREF) || !op2->TypeIs(TYP_I_IMPL, TYP_BYREF) || - (op1->TypeGet() != op2->TypeGet())) - { - return nullptr; - } - - unsigned op1Lcl; - unsigned op1Offs; - if (!impIsAddressInLocal(op1, &op1Lcl, &op1Offs)) - { - return nullptr; - } - - unsigned op2Lcl; - unsigned op2Offs; - if (!impIsAddressInLocal(op2, &op2Lcl, &op2Offs)) - { - return nullptr; - } - - if (op1Lcl != op2Lcl) - { - return nullptr; - } - - return gtNewIconNode((ssize_t)op1Offs - (ssize_t)op2Offs, TYP_I_IMPL); -} - //------------------------------------------------------------------------ // Compiler::impNormStructVal: Normalize a struct value // @@ -6927,7 +6885,7 @@ void Compiler::impImportBlockCode(BasicBlock* block) lclNum = impInlineFetchLocal(lclNum DEBUGARG("Inline ldloca(s) first use temp")); assert(!lvaGetDesc(lclNum)->lvNormalizeOnLoad()); - op1 = gtNewLclVarAddrNode(lclNum, lvaIsImplicitByRefLocal(lclNum) ? TYP_BYREF : TYP_I_IMPL); + op1 = gtNewLclVarAddrNode(lclNum, TYP_BYREF); goto _PUSH_ADRVAR; } @@ -6980,7 +6938,7 @@ void Compiler::impImportBlockCode(BasicBlock* block) // where it was not absolutely needed, but doing otherwise would // require careful rethinking of the importer routines which use // the IL validity model (e. g. "impGetByRefResultType"). - op1 = gtNewLclVarAddrNode(lclNum, lvaIsImplicitByRefLocal(lclNum) ? TYP_BYREF : TYP_I_IMPL); + op1 = gtNewLclVarAddrNode(lclNum, TYP_BYREF); _PUSH_ADRVAR: assert(op1->OperIs(GT_LCL_VAR_ADDR)); @@ -7380,31 +7338,8 @@ void Compiler::impImportBlockCode(BasicBlock* block) goto MATH_OP2_FLAGS; case CEE_SUB: - if (opts.OptimizationEnabled()) - { - op2 = impPopStack().val; - op1 = impPopStack().val; - - GenTree* folded = impFoldSubOfLocalOffsets(op1, op2); - - if (folded != nullptr) - { - impPushOnStack(folded, tiRetVal); - break; - } - else - { - oper = GT_SUB; - ovfl = false; - callNode = false; - goto MATH_OP2_WITH_OPS; - } - } - else - { - oper = GT_SUB; - goto MATH_OP2; - } + oper = GT_SUB; + goto MATH_OP2; case CEE_SUB_OVF: uns = false; @@ -7487,8 +7422,6 @@ void Compiler::impImportBlockCode(BasicBlock* block) op2 = impPopStack().val; op1 = impPopStack().val; - MATH_OP2_WITH_OPS: - /* Can't do arithmetic with references */ assertImp(genActualType(op1->TypeGet()) != TYP_REF && genActualType(op2->TypeGet()) != TYP_REF); @@ -12841,36 +12774,26 @@ bool Compiler::impIsInvariant(const GenTree* tree) // the address of a field in a local. // Arguments: // tree -- The tree -// lclNum -- [out, optional] the local that this points into -// lclOffs -- [out, optional] the offset into the local +// lclVarTreeOut -- [out] the local that this points into // // Returns: // true if it points into a local // -bool Compiler::impIsAddressInLocal(const GenTree* tree, unsigned* lclNum, unsigned* lclOffs) +bool Compiler::impIsAddressInLocal(const GenTree* tree, GenTree** lclVarTreeOut) { - unsigned offs = 0; - const GenTree* op = tree; + const GenTree* op = tree; while (op->OperIs(GT_FIELD_ADDR) && op->AsField()->IsInstance()) { - offs += op->AsField()->gtFldOffset; op = op->AsField()->GetFldObj(); } - // Unexpected to see this during import - assert(!op->OperIs(GT_LCL_FLD_ADDR)); - if (op->OperIs(GT_LCL_VAR_ADDR)) { - if (lclNum != nullptr) + if (lclVarTreeOut != nullptr) { - *lclNum = op->AsLclVarCommon()->GetLclNum(); + *lclVarTreeOut = const_cast(op); } - if (lclOffs != nullptr) - { - *lclOffs = offs + op->AsLclVarCommon()->GetLclOffs(); - } return true; } @@ -13245,10 +13168,11 @@ void Compiler::impInlineRecordArgInfo(InlineInfo* pInlineInfo, return; } - unsigned lclNum; - if (impIsAddressInLocal(curArgVal, &lclNum)) + GenTree* lclVarTree; + const bool isAddressInLocal = impIsAddressInLocal(curArgVal, &lclVarTree); + if (isAddressInLocal) { - LclVarDsc* varDsc = lvaGetDesc(lclNum); + LclVarDsc* varDsc = lvaGetDesc(lclVarTree->AsLclVarCommon()); if (varTypeIsStruct(varDsc)) { diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index b2ee241d6106a7..4c8d1da92ca595 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -2865,7 +2865,7 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis, GenTree* indexClone = nullptr; GenTree* ptrToSpanClone = nullptr; assert(genActualType(index) == TYP_INT); - assert(ptrToSpan->TypeIs(TYP_I_IMPL, TYP_BYREF)); + assert(ptrToSpan->TypeGet() == TYP_BYREF || ptrToSpan->TypeGet() == TYP_I_IMPL); #if defined(DEBUG) if (verbose) diff --git a/src/coreclr/jit/lclmorph.cpp b/src/coreclr/jit/lclmorph.cpp index 54402124209aec..8f8014d12d4a5a 100644 --- a/src/coreclr/jit/lclmorph.cpp +++ b/src/coreclr/jit/lclmorph.cpp @@ -677,6 +677,33 @@ class LocalAddressVisitor final : public GenTreeVisitor PopValue(); break; + case GT_SUB: + { + Value& rhs = TopValue(0); + Value& lhs = TopValue(1); + if (m_compiler->opts.OptimizationEnabled() && lhs.IsAddress() && rhs.IsAddress() && + (lhs.LclNum() == rhs.LclNum()) && (rhs.Offset() <= lhs.Offset()) && + FitsIn(lhs.Offset() - rhs.Offset())) + { + assert(node->TypeIs(TYP_I_IMPL)); + + ssize_t result = (ssize_t)(lhs.Offset() - rhs.Offset()); + *use = node = m_compiler->gtNewIconNode(result, TYP_I_IMPL); + INDEBUG(lhs.Consume()); + INDEBUG(rhs.Consume()); + PopValue(); + PopValue(); + m_stmtModified = true; + break; + } + + EscapeValue(TopValue(0), node); + PopValue(); + EscapeValue(TopValue(0), node); + PopValue(); + break; + } + case GT_FIELD_ADDR: if (node->AsField()->IsInstance()) { diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index a0fa6ba66784cb..88efb47a632cb4 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -4467,7 +4467,7 @@ ValueNum ValueNumStore::EvalUsingMathIdentity(var_types typ, VNFunc func, ValueN { // (x + a) - x == a VNFuncApp add; - if ((resultVN == NoVN) && GetVNFunc(arg0VN, &add) && (add.m_func == (VNFunc)GT_ADD)) + if (GetVNFunc(arg0VN, &add) && (add.m_func == (VNFunc)GT_ADD)) { if (add.m_args[0] == arg1VN) return add.m_args[1]; From e1f4777a1d6d4247302b140204719f3f1ee0f732 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 31 Mar 2023 12:33:19 +0200 Subject: [PATCH 5/8] Handle field-to-field offset --- src/coreclr/jit/valuenum.cpp | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 88efb47a632cb4..fac73f71eeefab 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -4473,6 +4473,16 @@ ValueNum ValueNumStore::EvalUsingMathIdentity(var_types typ, VNFunc func, ValueN return add.m_args[1]; if (add.m_args[1] == arg1VN) return add.m_args[0]; + + // (x + a) - (x + b) == a - b + VNFuncApp add2; + if (GetVNFunc(arg1VN, &add2) && (add2.m_func == (VNFunc)GT_ADD)) + { + if (add2.m_args[0] == add.m_args[0]) + { + return VNForFunc(typ, (VNFunc)GT_SUB, add.m_args[1], add2.m_args[1]); + } + } } } } From 423937732b4f82ad5ab0ea6bcb9896240a221bff Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 31 Mar 2023 12:46:03 +0200 Subject: [PATCH 6/8] Generalize --- src/coreclr/jit/valuenum.cpp | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index fac73f71eeefab..d5d5e08bffff14 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -4466,6 +4466,7 @@ ValueNum ValueNumStore::EvalUsingMathIdentity(var_types typ, VNFunc func, ValueN if (!ovf) { // (x + a) - x == a + // (a + x) - x == a VNFuncApp add; if (GetVNFunc(arg0VN, &add) && (add.m_func == (VNFunc)GT_ADD)) { @@ -4475,12 +4476,21 @@ ValueNum ValueNumStore::EvalUsingMathIdentity(var_types typ, VNFunc func, ValueN return add.m_args[0]; // (x + a) - (x + b) == a - b + // (a + x) - (x + b) == a - b + // (x + a) - (b + x) == a - b + // (a + x) - (b + x) == a - b VNFuncApp add2; if (GetVNFunc(arg1VN, &add2) && (add2.m_func == (VNFunc)GT_ADD)) { - if (add2.m_args[0] == add.m_args[0]) + for (int a = 0; a < 2; a++) { - return VNForFunc(typ, (VNFunc)GT_SUB, add.m_args[1], add2.m_args[1]); + for (int b = 0; b < 2; b++) + { + if (add.m_args[a] == add2.m_args[b]) + { + return VNForFunc(typ, (VNFunc)GT_SUB, add.m_args[1 - a], add2.m_args[1 - b]); + } + } } } } From adb3474bbec1a192951ce78e0dd0342b8a1577f6 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 4 Apr 2023 14:18:22 +0200 Subject: [PATCH 7/8] Expand assertion to cover TYP_BYREF --- src/coreclr/jit/lclmorph.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/lclmorph.cpp b/src/coreclr/jit/lclmorph.cpp index 8f8014d12d4a5a..cd89df9a937498 100644 --- a/src/coreclr/jit/lclmorph.cpp +++ b/src/coreclr/jit/lclmorph.cpp @@ -685,7 +685,8 @@ class LocalAddressVisitor final : public GenTreeVisitor (lhs.LclNum() == rhs.LclNum()) && (rhs.Offset() <= lhs.Offset()) && FitsIn(lhs.Offset() - rhs.Offset())) { - assert(node->TypeIs(TYP_I_IMPL)); + // TODO-Correctness: Due to inlining we may end up with incorrectly typed SUB trees here. + assert(node->TypeIs(TYP_I_IMPL, TYP_BYREF)); ssize_t result = (ssize_t)(lhs.Offset() - rhs.Offset()); *use = node = m_compiler->gtNewIconNode(result, TYP_I_IMPL); From 70627f4d0bf440674d637a2f35273656dd1bae74 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 4 Apr 2023 19:18:23 +0200 Subject: [PATCH 8/8] Apply suggestions from code review Co-authored-by: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com> --- src/coreclr/jit/lclmorph.cpp | 4 ++-- src/coreclr/jit/valuenum.cpp | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/coreclr/jit/lclmorph.cpp b/src/coreclr/jit/lclmorph.cpp index cd89df9a937498..bda5e9bac316bd 100644 --- a/src/coreclr/jit/lclmorph.cpp +++ b/src/coreclr/jit/lclmorph.cpp @@ -685,11 +685,11 @@ class LocalAddressVisitor final : public GenTreeVisitor (lhs.LclNum() == rhs.LclNum()) && (rhs.Offset() <= lhs.Offset()) && FitsIn(lhs.Offset() - rhs.Offset())) { - // TODO-Correctness: Due to inlining we may end up with incorrectly typed SUB trees here. + // TODO-Bug: Due to inlining we may end up with incorrectly typed SUB trees here. assert(node->TypeIs(TYP_I_IMPL, TYP_BYREF)); ssize_t result = (ssize_t)(lhs.Offset() - rhs.Offset()); - *use = node = m_compiler->gtNewIconNode(result, TYP_I_IMPL); + node->BashToConst(result, TYP_I_IMPL); INDEBUG(lhs.Consume()); INDEBUG(rhs.Consume()); PopValue(); diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index d5d5e08bffff14..ee150eb59822f6 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -4548,7 +4548,7 @@ ValueNum ValueNumStore::EvalUsingMathIdentity(var_types typ, VNFunc func, ValueN break; case GT_SUB: - resultVN = identityForSubtraction(false); + resultVN = identityForSubtraction(/* ovf */ false); break; case GT_MUL: @@ -4823,7 +4823,7 @@ ValueNum ValueNumStore::EvalUsingMathIdentity(var_types typ, VNFunc func, ValueN case VNF_SUB_OVF: case VNF_SUB_UN_OVF: - resultVN = identityForSubtraction(true); + resultVN = identityForSubtraction(/* ovf */ true); break; case VNF_MUL_OVF: