From 35bac19631652d7b0b2718c64ad11f1ab10f0dff Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Thu, 30 Sep 2021 18:30:08 +0300 Subject: [PATCH 1/4] Refactor how casts are VNed, part 1 Remove duplication between VNPairForCast and VNForCast, route the former through the latter. Also change the debug output for casts. No diffs for this commit. --- src/coreclr/jit/valuenum.cpp | 149 +++++++++++++++-------------------- src/coreclr/jit/valuenum.h | 14 +++- 2 files changed, 75 insertions(+), 88 deletions(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 71c68cd7d78e7..0f7a29c6eb17e 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -1731,8 +1731,7 @@ ValueNum ValueNumStore::VNForByrefCon(target_size_t cnsVal) return VnForConst(cnsVal, GetByrefCnsMap(), TYP_BYREF); } -ValueNum ValueNumStore::VNForCastOper(var_types castToType, - bool srcIsUnsigned /*=false*/ DEBUGARG(bool printResult /*=true*/)) +ValueNum ValueNumStore::VNForCastOper(var_types castToType, bool srcIsUnsigned) { assert(castToType != TYP_STRUCT); INT32 cnsVal = INT32(castToType) << INT32(VCA_BitCount); @@ -1745,14 +1744,6 @@ ValueNum ValueNumStore::VNForCastOper(var_types castToType, } ValueNum result = VNForIntCon(cnsVal); -#ifdef DEBUG - if (printResult && m_pComp->verbose) - { - printf(" VNForCastOper(%s%s) is " FMT_VN "\n", varTypeName(castToType), srcIsUnsigned ? ", unsignedSrc" : "", - result); - } -#endif - return result; } @@ -1768,7 +1759,7 @@ void ValueNumStore::GetCastOperFromVN(ValueNum vn, var_types* pCastToType, bool* *pSrcIsUnsigned = (value & INT32(VCA_UnsignedSrc)) != 0; *pCastToType = var_types(value >> INT32(VCA_BitCount)); - assert(VNForCastOper(*pCastToType, *pSrcIsUnsigned DEBUGARG(/*printResult*/ false)) == vn); + assert(VNForCastOper(*pCastToType, *pSrcIsUnsigned) == vn); } ValueNum ValueNumStore::VNForHandle(ssize_t cnsVal, GenTreeFlags handleFlags) @@ -5608,6 +5599,10 @@ void ValueNumStore::vnDump(Compiler* comp, ValueNum vn, bool isPtr) vnDumpSimdType(comp, &funcApp); break; #endif // FEATURE_SIMD + case VNF_Cast: + case VNF_CastOvf: + vnDumpCast(comp, vn); + break; default: printf("%s(", VNFuncName(funcApp.m_func)); @@ -5799,6 +5794,36 @@ void ValueNumStore::vnDumpSimdType(Compiler* comp, VNFuncApp* simdType) } #endif // FEATURE_SIMD +void ValueNumStore::vnDumpCast(Compiler* comp, ValueNum castVN) +{ + VNFuncApp cast; + bool castFound = GetVNFunc(castVN, &cast); + assert(castFound && ((cast.m_func == VNF_Cast) || (cast.m_func == VNF_CastOvf))); + + var_types castToType; + bool srcIsUnsigned; + GetCastOperFromVN(cast.m_args[1], &castToType, &srcIsUnsigned); + + var_types castFromType = TypeOfVN(cast.m_args[0]); + var_types resultType = TypeOfVN(castVN); + if (srcIsUnsigned) + { + castFromType = varTypeToUnsigned(castFromType); + } + + comp->vnPrint(cast.m_args[0], 0); + + printf(", "); + if ((resultType != castToType) && (castToType != castFromType)) + { + printf("%s <- %s <- %s", varTypeName(resultType), varTypeName(castToType), varTypeName(castFromType)); + } + else + { + printf("%s <- %s", varTypeName(resultType), varTypeName(castFromType)); + } +} + #endif // DEBUG // Static fields, methods. @@ -9418,38 +9443,41 @@ void Compiler::fgValueNumberCastTree(GenTree* tree) tree->gtVNPair = vnStore->VNPairForCast(srcVNPair, castToType, castFromType, srcIsUnsigned, hasOverflowCheck); } -// Compute the normal ValueNumber for a cast operation with no exceptions +// Compute the ValueNumber for a cast operation ValueNum ValueNumStore::VNForCast(ValueNum srcVN, var_types castToType, var_types castFromType, - bool srcIsUnsigned /* = false */) + bool srcIsUnsigned, /* = false */ + bool hasOverflowCheck) /* = false */ { - // The resulting type after performingthe cast is always widened to a supported IL stack size + // The resulting type after performing the cast is always widened to a supported IL stack size var_types resultType = genActualType(castToType); - // When we're considering actual value returned by a non-checking cast whether or not the source is - // unsigned does *not* matter for non-widening casts. That is, if we cast an int or a uint to short, - // we just extract the first two bytes from the source bit pattern, not worrying about the interpretation. - // The same is true in casting between signed/unsigned types of the same width. Only when we're doing - // a widening cast do we care about whether the source was unsigned,so we know whether to sign or zero extend it. - // - bool srcIsUnsignedNorm = srcIsUnsigned; - if (genTypeSize(castToType) <= genTypeSize(castFromType)) + // For integral unchecked casts, only the "int -> long" upcasts use + // "srcIsUnsigned", to decide whether to use sign or zero extension. + if (!hasOverflowCheck && !varTypeIsFloating(castToType) && (genTypeSize(castToType) <= genTypeSize(castFromType))) { - srcIsUnsignedNorm = false; + srcIsUnsigned = false; } - ValueNum castTypeVN = VNForCastOper(castToType, srcIsUnsigned); - ValueNum resultVN = VNForFunc(resultType, VNF_Cast, srcVN, castTypeVN); + ValueNum srcExcVN; + ValueNum srcNormVN; + VNUnpackExc(srcVN, &srcNormVN, &srcExcVN); -#ifdef DEBUG - if (m_pComp->verbose) + VNFunc castFunc = hasOverflowCheck ? VNF_CastOvf : VNF_Cast; + ValueNum castTypeVN = VNForCastOper(castToType, srcIsUnsigned); + ValueNum resultNormVN = VNForFunc(resultType, castFunc, srcNormVN, castTypeVN); + ValueNum resultExcVN = srcExcVN; + + // Add an exception, except if folding took place. + // We only fold checked casts that do not overflow. + if (hasOverflowCheck && !IsVNConstant(resultNormVN)) { - printf(" VNForCast(" FMT_VN ", " FMT_VN ") returns ", srcVN, castTypeVN); - m_pComp->vnPrint(resultVN, 1); - printf("\n"); + ValueNum ovfChk = VNForFunc(TYP_REF, VNF_ConvOverflowExc, srcNormVN, castTypeVN); + resultExcVN = VNExcSetUnion(VNExcSetSingleton(ovfChk), srcExcVN); } -#endif + + ValueNum resultVN = VNWithExc(resultNormVN, resultExcVN); return resultVN; } @@ -9461,60 +9489,12 @@ ValueNumPair ValueNumStore::VNPairForCast(ValueNumPair srcVNPair, bool srcIsUnsigned, /* = false */ bool hasOverflowCheck) /* = false */ { - // The resulting type after performingthe cast is always widened to a supported IL stack size - var_types resultType = genActualType(castToType); - - ValueNumPair castArgVNP; - ValueNumPair castArgxVNP; - VNPUnpackExc(srcVNPair, &castArgVNP, &castArgxVNP); - - // When we're considering actual value returned by a non-checking cast, (hasOverflowCheck is false) - // whether or not the source is unsigned does *not* matter for non-widening casts. - // That is, if we cast an int or a uint to short, we just extract the first two bytes from the source - // bit pattern, not worrying about the interpretation. The same is true in casting between signed/unsigned - // types of the same width. Only when we're doing a widening cast do we care about whether the source - // was unsigned, so we know whether to sign or zero extend it. - // - // Important: Casts to floating point cannot be optimized in this fashion. (bug 946768) - // - bool srcIsUnsignedNorm = srcIsUnsigned; - if (!hasOverflowCheck && !varTypeIsFloating(castToType) && (genTypeSize(castToType) <= genTypeSize(castFromType))) - { - srcIsUnsignedNorm = false; - } - - VNFunc vnFunc = hasOverflowCheck ? VNF_CastOvf : VNF_Cast; - ValueNum castTypeVN = VNForCastOper(castToType, srcIsUnsignedNorm); - ValueNumPair castTypeVNPair(castTypeVN, castTypeVN); - ValueNumPair castNormRes = VNPairForFunc(resultType, vnFunc, castArgVNP, castTypeVNPair); + ValueNum srcLibVN = srcVNPair.GetLiberal(); + ValueNum srcConVN = srcVNPair.GetConservative(); + ValueNum castLibVN = VNForCast(srcLibVN, castToType, castFromType, srcIsUnsigned, hasOverflowCheck); + ValueNum castConVN = VNForCast(srcConVN, castToType, castFromType, srcIsUnsigned, hasOverflowCheck); - ValueNumPair resultVNP = VNPWithExc(castNormRes, castArgxVNP); - - // If we have a check for overflow, add the exception information. - if (hasOverflowCheck) - { - ValueNumPair excSet = ValueNumStore::VNPForEmptyExcSet(); - - ValueNumKind vnKinds[2] = {VNK_Liberal, VNK_Conservative}; - for (ValueNumKind vnKind : vnKinds) - { - // Do not add exceptions for folded casts. - // We only fold checked casts that do not overflow. - if (IsVNConstant(castNormRes.Get(vnKind))) - { - continue; - } - - ValueNum ovfChk = - VNForFunc(TYP_REF, VNF_ConvOverflowExc, castArgVNP.Get(vnKind), castTypeVNPair.Get(vnKind)); - excSet.Set(vnKind, VNExcSetSingleton(ovfChk)); - } - - excSet = VNPExcSetUnion(excSet, castArgxVNP); - resultVNP = VNPWithExc(castNormRes, excSet); - } - - return resultVNP; + return {castLibVN, castConVN}; } void Compiler::fgValueNumberHelperCallFunc(GenTreeCall* call, VNFunc vnf, ValueNumPair vnpExc) @@ -10843,7 +10823,6 @@ void Compiler::vnpPrint(ValueNumPair vnp, unsigned level) void Compiler::vnPrint(ValueNum vn, unsigned level) { - if (ValueNumStore::isReservedVN(vn)) { printf(ValueNumStore::reservedName(vn)); diff --git a/src/coreclr/jit/valuenum.h b/src/coreclr/jit/valuenum.h index b64d19993fa72..d09bd3197e91c 100644 --- a/src/coreclr/jit/valuenum.h +++ b/src/coreclr/jit/valuenum.h @@ -291,7 +291,7 @@ class ValueNumStore // Packs information about the cast into an integer constant represented by the returned value number, // to be used as the second operand of VNF_Cast & VNF_CastOvf. - ValueNum VNForCastOper(var_types castToType, bool srcIsUnsigned = false DEBUGARG(bool printResult = true)); + ValueNum VNForCastOper(var_types castToType, bool srcIsUnsigned); // Unpacks the information stored by VNForCastOper in the constant represented by the value number. void GetCastOperFromVN(ValueNum vn, var_types* pCastToType, bool* pSrcIsUnsigned); @@ -556,8 +556,12 @@ class ValueNumStore rhs.GetConservative(), indType, block)); } - // Compute the normal ValueNumber for a cast with no exceptions - ValueNum VNForCast(ValueNum srcVN, var_types castToType, var_types castFromType, bool srcIsUnsigned = false); + // Compute the ValueNumber for a cast + ValueNum VNForCast(ValueNum srcVN, + var_types castToType, + var_types castFromType, + bool srcIsUnsigned = false, + bool hasOverflowCheck = false); // Compute the ValueNumberPair for a cast ValueNumPair VNPairForCast(ValueNumPair srcVNPair, @@ -898,6 +902,10 @@ class ValueNumStore void vnDumpSimdType(Compiler* comp, VNFuncApp* simdType); #endif // FEATURE_SIMD + // Requires "castVN" to represent VNF_Cast or VNF_CastOvf. + // Prints the cast's representation mirroring GT_CAST's dump format. + void vnDumpCast(Compiler* comp, ValueNum castVN); + // Returns the string name of "vnf". static const char* VNFuncName(VNFunc vnf); // Used in the implementation of the above. From 6b5d55813fbd3c4daf80fd149538038cccabdfc2 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Thu, 30 Sep 2021 20:54:40 +0300 Subject: [PATCH 2/4] Refactor how casts are VNed, part 2 VN cast helpers just like casts. This allows them to be folded. In the past, this was dangerous because some of the pessimization protected us from undefined behavior in cases of out-of-bounds conversions, but now we do not fold such cases anymore. --- src/coreclr/jit/compiler.h | 3 + src/coreclr/jit/valuenum.cpp | 138 +++++++++++++++++++++++--------- src/coreclr/jit/valuenumfuncs.h | 10 --- 3 files changed, 104 insertions(+), 47 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 3758c56da45f5..9fe811025a1db 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5494,6 +5494,9 @@ class Compiler // Does value-numbering for a call. We interpret some helper calls. void fgValueNumberCall(GenTreeCall* call); + // Does value-numbering for a helper representing a cast operation. + void fgValueNumberCastHelper(GenTreeCall* call); + // Does value-numbering for a helper "call" that has a VN function symbol "vnf". void fgValueNumberHelperCallFunc(GenTreeCall* call, VNFunc vnf, ValueNumPair vnpExc); diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 0f7a29c6eb17e..bf2f3c82b2b18 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -9769,6 +9769,81 @@ void Compiler::fgValueNumberCall(GenTreeCall* call) } } +void Compiler::fgValueNumberCastHelper(GenTreeCall* call) +{ + CorInfoHelpFunc helpFunc = eeGetHelperNum(call->gtCallMethHnd); + var_types castToType = TYP_UNDEF; + var_types castFromType = TYP_UNDEF; + bool srcIsUnsigned = false; + bool hasOverflowCheck = false; + + switch (helpFunc) + { + case CORINFO_HELP_LNG2DBL: + castToType = TYP_DOUBLE; + castFromType = TYP_LONG; + break; + + case CORINFO_HELP_ULNG2DBL: + castToType = TYP_DOUBLE; + castFromType = TYP_LONG; + srcIsUnsigned = true; + break; + + case CORINFO_HELP_DBL2INT: + castToType = TYP_INT; + castFromType = TYP_DOUBLE; + break; + + case CORINFO_HELP_DBL2INT_OVF: + castToType = TYP_INT; + castFromType = TYP_DOUBLE; + hasOverflowCheck = true; + break; + + case CORINFO_HELP_DBL2LNG: + castToType = TYP_LONG; + castFromType = TYP_DOUBLE; + break; + + case CORINFO_HELP_DBL2LNG_OVF: + castToType = TYP_LONG; + castFromType = TYP_DOUBLE; + hasOverflowCheck = true; + break; + + case CORINFO_HELP_DBL2UINT: + castToType = TYP_UINT; + castFromType = TYP_DOUBLE; + break; + + case CORINFO_HELP_DBL2UINT_OVF: + castToType = TYP_UINT; + castFromType = TYP_DOUBLE; + hasOverflowCheck = true; + break; + + case CORINFO_HELP_DBL2ULNG: + castToType = TYP_ULONG; + castFromType = TYP_DOUBLE; + break; + + case CORINFO_HELP_DBL2ULNG_OVF: + castToType = TYP_ULONG; + castFromType = TYP_DOUBLE; + hasOverflowCheck = true; + break; + + default: + unreached(); + } + + ValueNumPair argVNP = call->gtCallArgs->GetNode()->gtVNPair; + ValueNumPair castVNP = vnStore->VNPairForCast(argVNP, castToType, castFromType, srcIsUnsigned, hasOverflowCheck); + + call->SetVNs(castVNP); +} + VNFunc Compiler::fgValueNumberJitHelperMethodVNFunc(CorInfoHelpFunc helpFunc) { assert(s_helperCallProperties.IsPure(helpFunc) || s_helperCallProperties.IsAllocator(helpFunc)); @@ -9817,37 +9892,6 @@ VNFunc Compiler::fgValueNumberJitHelperMethodVNFunc(CorInfoHelpFunc helpFunc) case CORINFO_HELP_ULMOD: vnf = VNFunc(GT_UMOD); break; - - case CORINFO_HELP_LNG2DBL: - vnf = VNF_Lng2Dbl; - break; - case CORINFO_HELP_ULNG2DBL: - vnf = VNF_ULng2Dbl; - break; - case CORINFO_HELP_DBL2INT: - vnf = VNF_Dbl2Int; - break; - case CORINFO_HELP_DBL2INT_OVF: - vnf = VNF_Dbl2IntOvf; - break; - case CORINFO_HELP_DBL2LNG: - vnf = VNF_Dbl2Lng; - break; - case CORINFO_HELP_DBL2LNG_OVF: - vnf = VNF_Dbl2LngOvf; - break; - case CORINFO_HELP_DBL2UINT: - vnf = VNF_Dbl2UInt; - break; - case CORINFO_HELP_DBL2UINT_OVF: - vnf = VNF_Dbl2UIntOvf; - break; - case CORINFO_HELP_DBL2ULNG: - vnf = VNF_Dbl2ULng; - break; - case CORINFO_HELP_DBL2ULNG_OVF: - vnf = VNF_Dbl2ULngOvf; - break; case CORINFO_HELP_FLTREM: vnf = VNFunc(GT_MOD); break; @@ -10052,12 +10096,32 @@ VNFunc Compiler::fgValueNumberJitHelperMethodVNFunc(CorInfoHelpFunc helpFunc) bool Compiler::fgValueNumberHelperCall(GenTreeCall* call) { - CorInfoHelpFunc helpFunc = eeGetHelperNum(call->gtCallMethHnd); - bool pure = s_helperCallProperties.IsPure(helpFunc); - bool isAlloc = s_helperCallProperties.IsAllocator(helpFunc); - bool modHeap = s_helperCallProperties.MutatesHeap(helpFunc); - bool mayRunCctor = s_helperCallProperties.MayRunCctor(helpFunc); - bool noThrow = s_helperCallProperties.NoThrow(helpFunc); + CorInfoHelpFunc helpFunc = eeGetHelperNum(call->gtCallMethHnd); + + switch (helpFunc) + { + case CORINFO_HELP_LNG2DBL: + case CORINFO_HELP_ULNG2DBL: + case CORINFO_HELP_DBL2INT: + case CORINFO_HELP_DBL2INT_OVF: + case CORINFO_HELP_DBL2LNG: + case CORINFO_HELP_DBL2LNG_OVF: + case CORINFO_HELP_DBL2UINT: + case CORINFO_HELP_DBL2UINT_OVF: + case CORINFO_HELP_DBL2ULNG: + case CORINFO_HELP_DBL2ULNG_OVF: + fgValueNumberCastHelper(call); + return false; + + default: + break; + } + + bool pure = s_helperCallProperties.IsPure(helpFunc); + bool isAlloc = s_helperCallProperties.IsAllocator(helpFunc); + bool modHeap = s_helperCallProperties.MutatesHeap(helpFunc); + bool mayRunCctor = s_helperCallProperties.MayRunCctor(helpFunc); + bool noThrow = s_helperCallProperties.NoThrow(helpFunc); ValueNumPair vnpExc = ValueNumStore::VNPForEmptyExcSet(); diff --git a/src/coreclr/jit/valuenumfuncs.h b/src/coreclr/jit/valuenumfuncs.h index 448f40e382146..7665770881bcd 100644 --- a/src/coreclr/jit/valuenumfuncs.h +++ b/src/coreclr/jit/valuenumfuncs.h @@ -69,16 +69,6 @@ ValueNumFuncDef(InvalidCastExc, 2, false, false, false) // CastClass check, ValueNumFuncDef(NewArrOverflowExc, 1, false, false, false) // Raises Integer overflow when Arg 0 is negative ValueNumFuncDef(HelperMultipleExc, 0, false, false, false) // Represents one or more different exceptions that could be thrown by a Jit Helper method -ValueNumFuncDef(Lng2Dbl, 1, false, false, false) -ValueNumFuncDef(ULng2Dbl, 1, false, false, false) -ValueNumFuncDef(Dbl2Int, 1, false, false, false) -ValueNumFuncDef(Dbl2UInt, 1, false, false, false) -ValueNumFuncDef(Dbl2Lng, 1, false, false, false) -ValueNumFuncDef(Dbl2ULng, 1, false, false, false) -ValueNumFuncDef(Dbl2IntOvf, 1, false, false, false) -ValueNumFuncDef(Dbl2UIntOvf, 1, false, false, false) -ValueNumFuncDef(Dbl2LngOvf, 1, false, false, false) -ValueNumFuncDef(Dbl2ULngOvf, 1, false, false, false) ValueNumFuncDef(FltRound, 1, false, false, false) ValueNumFuncDef(DblRound, 1, false, false, false) From 548a6ce8ff5af65792a9b2b74756618a7bf61356 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Thu, 30 Sep 2021 21:38:15 +0300 Subject: [PATCH 3/4] Enable VN-based constant propagation for calls This solves some of the regressions seen due to now-missing CSEs of these helper calls evaluated as constants by VN. --- src/coreclr/jit/assertionprop.cpp | 16 +++++++++++++++- src/coreclr/jit/gentree.cpp | 18 ++++++++++++++---- 2 files changed, 29 insertions(+), 5 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index e6334311c6aab..6f35ec48038dd 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -5425,7 +5425,14 @@ GenTree* Compiler::optExtractSideEffListFromConst(GenTree* tree) { // Do a sanity check to ensure persistent side effects aren't discarded and // tell gtExtractSideEffList to ignore the root of the tree. - assert(!gtNodeHasSideEffects(tree, GTF_PERSISTENT_SIDE_EFFECTS)); + // We are relying here on an invariant that VN will only fold non-throwing expressions. + const bool ignoreExceptions = true; + const bool ignoreCctors = false; + // We have to check "AsCall()->HasSideEffects()" here separately because "gtNodeHasSideEffects" + // also checks for side effects that arguments introduce (incosistently so, it otherwise only + // checks for the side effects the node itself has). TODO-Cleanup: change it to not do that? + assert(!gtNodeHasSideEffects(tree, GTF_PERSISTENT_SIDE_EFFECTS) || + (tree->IsCall() && !tree->AsCall()->HasSideEffects(this, ignoreExceptions, ignoreCctors))); // Exception side effects may be ignored because the root is known to be a constant // (e.g. VN may evaluate a DIV/MOD node to a constant and the node may still @@ -5625,6 +5632,13 @@ Compiler::fgWalkResult Compiler::optVNConstantPropCurStmt(BasicBlock* block, Sta } break; + case GT_CALL: + if (!tree->AsCall()->IsPure(this)) + { + return WALK_CONTINUE; + } + break; + default: // Unknown node, continue to walk. return WALK_CONTINUE; diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index a9f5402b50852..09fc05d141fdb 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -15924,7 +15924,7 @@ void Compiler::gtExtractSideEffList(GenTree* expr, { if (m_compiler->gtNodeHasSideEffects(node, m_flags)) { - m_sideEffects.Push(node); + PushSideEffects(node); if (node->OperIsBlk() && !node->OperIsStoreBlk()) { JITDUMP("Replace an unused OBJ/BLK node [%06d] with a NULLCHECK\n", dspTreeID(node)); @@ -15941,7 +15941,7 @@ void Compiler::gtExtractSideEffList(GenTree* expr, // gtNodeHasSideEffects and make this check unconditionally. if (node->OperIsAtomicOp()) { - m_sideEffects.Push(node); + PushSideEffects(node); return Compiler::WALK_SKIP_SUBTREES; } @@ -15952,7 +15952,7 @@ void Compiler::gtExtractSideEffList(GenTree* expr, (node->gtGetOp1()->TypeGet() == TYP_STRUCT)) { JITDUMP("Keep the GT_ADDR and GT_IND together:\n"); - m_sideEffects.Push(node); + PushSideEffects(node); return Compiler::WALK_SKIP_SUBTREES; } } @@ -15969,7 +15969,7 @@ void Compiler::gtExtractSideEffList(GenTree* expr, // those need to be extracted as if they're side effects. if (!UnmarkCSE(node)) { - m_sideEffects.Push(node); + PushSideEffects(node); return Compiler::WALK_SKIP_SUBTREES; } @@ -16005,6 +16005,16 @@ void Compiler::gtExtractSideEffList(GenTree* expr, return false; } } + + void PushSideEffects(GenTree* node) + { + // The extracted side effect will no longer be an argument, so unmark it. + // This is safe to do because the side effects will be visited in pre-order, + // aborting as soon as any tree is extracted. Thus if an argument for a call + // is being extracted, it is guaranteed that the call itself will not be. + node->gtFlags &= ~GTF_LATE_ARG; + m_sideEffects.Push(node); + } }; SideEffectExtractor extractor(this, flags); From 3bcf1a91c6c0c62993ca78d1bc5bf78bf24ef299 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Fri, 1 Oct 2021 18:55:30 +0300 Subject: [PATCH 4/4] Fix VNFuncs for MUL_OVF helpers This fixes the test failures revealed by the more aggressive constant propagation. This is very similar to a bug that was recently fixed where checked casts were treated as non-checked and could be observed without the assertion propagation changes as well, with the redundant branches optimization. --- src/coreclr/jit/valuenum.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index bf2f3c82b2b18..8143582285386 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -9874,12 +9874,14 @@ VNFunc Compiler::fgValueNumberJitHelperMethodVNFunc(CorInfoHelpFunc helpFunc) vnf = VNFunc(GT_RSZ); break; case CORINFO_HELP_LMUL: - case CORINFO_HELP_LMUL_OVF: vnf = VNFunc(GT_MUL); break; + case CORINFO_HELP_LMUL_OVF: + vnf = VNF_MUL_OVF; + break; case CORINFO_HELP_ULMUL_OVF: - vnf = VNFunc(GT_MUL); - break; // Is this the right thing? + vnf = VNF_MUL_UN_OVF; + break; case CORINFO_HELP_LDIV: vnf = VNFunc(GT_DIV); break;