From 4b9429af86c235bf0be5a07b95e1ad46229c6708 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 5 Dec 2021 23:08:41 +0300 Subject: [PATCH 1/8] Improvements for the inliner, part 3: 1) recognize pinvokes 2) remove "CALL(cns,...) -> CNS" heuristic 3) mark Span/ReadOnlySpan get_Length as intrinsic 4) recognize "static readonly" fields of primitive types as constants --- src/coreclr/jit/compiler.cpp | 1 + src/coreclr/jit/compiler.h | 1 + src/coreclr/jit/compiler.hpp | 1 - src/coreclr/jit/fgbasic.cpp | 27 ++++++++++++++----- src/coreclr/jit/gentree.h | 2 -- src/coreclr/jit/importer.cpp | 1 + src/coreclr/jit/inline.def | 1 + src/coreclr/jit/inlinepolicy.cpp | 26 ++++++++++++++++-- src/coreclr/jit/inlinepolicy.h | 2 ++ .../src/System/ReadOnlySpan.cs | 1 + .../System.Private.CoreLib/src/System/Span.cs | 1 + 11 files changed, 53 insertions(+), 11 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index e56c96b026c0b1..d481d832662d59 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -6281,6 +6281,7 @@ int Compiler::compCompileHelper(CORINFO_MODULE_HANDLE classPtr, compHasBackwardJump = false; compHasBackwardJumpInHandler = false; + compHasPinvokes = false; #ifdef DEBUG compCurBB = nullptr; diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 1a8d4040d5ecf4..46ddfb3c66eb3b 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -9423,6 +9423,7 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX bool compSwitchedToOptimized; // Codegen initially was Tier0 but jit switched to FullOpts bool compSwitchedToMinOpts; // Codegen initially was Tier1/FullOpts but jit switched to MinOpts bool compSuppressedZeroInit; // There are vars with lvSuppressedZeroInit set + bool compHasPinvokes; // Does the method have a pinvoke? // NOTE: These values are only reliable after // the importing is completely finished. diff --git a/src/coreclr/jit/compiler.hpp b/src/coreclr/jit/compiler.hpp index 8fad38af36f846..6328d6390ce96a 100644 --- a/src/coreclr/jit/compiler.hpp +++ b/src/coreclr/jit/compiler.hpp @@ -1149,7 +1149,6 @@ inline GenTree* Compiler::gtNewIndexRef(var_types typ, GenTree* arrayOp, GenTree inline GenTreeArrLen* Compiler::gtNewArrLen(var_types typ, GenTree* arrayOp, int lenOffset, BasicBlock* block) { GenTreeArrLen* arrLen = new (this, GT_ARR_LENGTH) GenTreeArrLen(typ, arrayOp, lenOffset); - static_assert_no_msg(GTF_ARRLEN_NONFAULTING == GTF_IND_NONFAULTING); arrLen->SetIndirExceptionFlags(this); if (block != nullptr) { diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index f20baa57a74ef1..bcccfcd0a35208 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -1101,6 +1101,7 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed CORINFO_METHOD_HANDLE methodHnd = nullptr; bool isJitIntrinsic = false; + bool isPinvoke = false; NamedIntrinsic ni = NI_Illegal; if (resolveTokens) @@ -1108,6 +1109,7 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed impResolveToken(codeAddr, &resolvedToken, CORINFO_TOKENKIND_Method); methodHnd = resolvedToken.hMethod; isJitIntrinsic = eeIsJitIntrinsic(methodHnd); + isPinvoke = info.compCompHnd->getMethodAttribs(methodHnd) & CORINFO_FLG_PINVOKE; } if (isJitIntrinsic) @@ -1238,6 +1240,10 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed } } } + else if (isPinvoke) + { + compInlineResult->NoteBool(InlineObservation::CALLEE_HAS_PINVOKE, true); + } if ((codeAddr < codeEndp - sz) && (OPCODE)getU1LittleEndian(codeAddr + sz) == CEE_RET) { @@ -1245,15 +1251,24 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed // it is a wrapper method. compInlineResult->Note(InlineObservation::CALLEE_LOOKS_LIKE_WRAPPER); } + } + break; - if (!isJitIntrinsic && !handled && FgStack::IsArgument(pushedStack.Top())) + case CEE_LDSFLD: + if (resolveTokens && !isPreJit) { - // Optimistically assume that "call(arg)" returns something arg-dependent. - // However, we don't know how many args it expects and its return type. - handled = true; + // Push a constant to the stack for static readonly fields (initialized) + // of primitive types. + impResolveToken(codeAddr, &resolvedToken, CORINFO_TOKENKIND_Field); + CORINFO_FIELD_HANDLE fldHnd = resolvedToken.hField; + bool inited = false; + if ((info.compCompHnd->getStaticFieldCurrentClass(fldHnd, &inited) == NO_CLASS_HANDLE) && inited) + { + pushedStack.PushConstant(); + handled = true; + } } - } - break; + break; case CEE_LDIND_I1: case CEE_LDIND_U1: diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 1a0865073982dc..ce863ebc168aa2 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -495,7 +495,6 @@ enum GenTreeFlags : unsigned int GTF_IND_TGT_NOT_HEAP = 0x80000000, // GT_IND -- the target is not on the heap GTF_IND_VOLATILE = 0x40000000, // GT_IND -- the load or store must use volatile sematics (this is a nop on X86) GTF_IND_NONFAULTING = 0x20000000, // Operations for which OperIsIndir() is true -- An indir that cannot fault. - // Same as GTF_ARRLEN_NONFAULTING. GTF_IND_TGTANYWHERE = 0x10000000, // GT_IND -- the target could be anywhere GTF_IND_TLS_REF = 0x08000000, // GT_IND -- the target is accessed via TLS GTF_IND_ASG_LHS = 0x04000000, // GT_IND -- this GT_IND node is (the effective val) of the LHS of an @@ -581,7 +580,6 @@ enum GenTreeFlags : unsigned int GTF_ARR_BOUND_INBND = 0x80000000, // GT_ARR_BOUNDS_CHECK -- have proved this check is always in-bounds GTF_ARRLEN_ARR_IDX = 0x80000000, // GT_ARR_LENGTH -- Length which feeds into an array index expression - GTF_ARRLEN_NONFAULTING = 0x20000000, // GT_ARR_LENGTH -- An array length operation that cannot fault. Same as GT_IND_NONFAULTING. GTF_SIMDASHW_OP = 0x80000000, // GT_HWINTRINSIC -- Indicates that the structHandle should be gotten from gtGetStructHandleForSIMD // rather than from gtGetStructHandleForHWSIMD. diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 70a5cf39b544a2..839b850ef2fd05 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -7735,6 +7735,7 @@ void Compiler::impCheckForPInvokeCall( if ((mflags & CORINFO_FLG_PINVOKE) != 0) { call->gtCallMoreFlags |= GTF_CALL_M_PINVOKE; + compHasPinvokes = true; } bool suppressGCTransition = false; diff --git a/src/coreclr/jit/inline.def b/src/coreclr/jit/inline.def index a28daf4e980fdc..28aa808668f83d 100644 --- a/src/coreclr/jit/inline.def +++ b/src/coreclr/jit/inline.def @@ -89,6 +89,7 @@ INLINE_OBSERVATION(HAS_LOCALLOC, bool, "has localloc", INLINE_OBSERVATION(HAS_PINNED_LOCALS, bool, "has pinned locals", INFORMATION, CALLEE) INLINE_OBSERVATION(HAS_SIMD, bool, "has SIMD arg, local, or ret", INFORMATION, CALLEE) INLINE_OBSERVATION(HAS_SWITCH, bool, "has switch", INFORMATION, CALLEE) +INLINE_OBSERVATION(HAS_PINVOKE, bool, "has pinvoke", INFORMATION, CALLEE) INLINE_OBSERVATION(IL_CODE_SIZE, int, "number of bytes of IL", INFORMATION, CALLEE) INLINE_OBSERVATION(IS_CLASS_CTOR, bool, "class constructor", INFORMATION, CALLEE) INLINE_OBSERVATION(IS_DISCRETIONARY_INLINE, bool, "can inline, check heuristics", INFORMATION, CALLEE) diff --git a/src/coreclr/jit/inlinepolicy.cpp b/src/coreclr/jit/inlinepolicy.cpp index 1ec47378872d54..4b671cb1a65fdd 100644 --- a/src/coreclr/jit/inlinepolicy.cpp +++ b/src/coreclr/jit/inlinepolicy.cpp @@ -1301,6 +1301,10 @@ void ExtendedDefaultPolicy::NoteBool(InlineObservation obs, bool value) m_Switch++; break; + case InlineObservation::CALLEE_HAS_PINVOKE: + m_HasPinvoke = value; + break; + case InlineObservation::CALLSITE_DIV_BY_CNS: m_DivByCns++; break; @@ -1485,7 +1489,7 @@ double ExtendedDefaultPolicy::DetermineMultiplier() // if (Math.Abs(arg0) > 10) { // same here // etc. // - multiplier += 3.0 + m_FoldableBranch; + multiplier += 3.0 + m_FoldableBranch * 1.5; JITDUMP("\nInline candidate has %d foldable branches. Multiplier increased to %g.", m_FoldableBranch, multiplier); } @@ -1521,7 +1525,7 @@ double ExtendedDefaultPolicy::DetermineMultiplier() if (m_Intrinsic > 0) { // In most cases such intrinsics are lowered as single CPU instructions - multiplier += 1.0 + m_Intrinsic * 0.3; + multiplier += 2.2 + m_Intrinsic * 0.5; JITDUMP("\nInline has %d intrinsics. Multiplier increased to %g.", m_Intrinsic, multiplier); } @@ -1726,6 +1730,23 @@ double ExtendedDefaultPolicy::DetermineMultiplier() JITDUMP("\nCallsite is in a no-return region. Multiplier limited to %g.", multiplier); } + if (m_HasPinvoke) + { + // Callees with pinvokes will inject a lot of code into callers even if the path with this callsite + // is not always taken. However, it's OK if we already have pinvokes in the caller + // Pinvokes with SuppressGCTransition are always fine but they are expensive to check in the inliner. + if (m_RootCompiler->compHasPinvokes) + { + multiplier += 1.5; + JITDUMP("\nCallee and caller have pinvoke(s). Multiplier increased to %g.", multiplier); + } + else + { + multiplier = 1.0; + JITDUMP("\nCallee has pinvoke(s). Multiplier limited to %g.", multiplier); + } + } + #ifdef DEBUG int additionalMultiplier = JitConfig.JitInlineAdditionalMultiplier(); @@ -1783,6 +1804,7 @@ void ExtendedDefaultPolicy::OnDumpXml(FILE* file, unsigned indent) const XATTR_B(m_NonGenericCallsGeneric) XATTR_B(m_IsCallsiteInNoReturnRegion) XATTR_B(m_HasProfile) + XATTR_B(m_HasPinvoke) } #endif diff --git a/src/coreclr/jit/inlinepolicy.h b/src/coreclr/jit/inlinepolicy.h index f5f200e9f5bd71..a85a466ba79e0d 100644 --- a/src/coreclr/jit/inlinepolicy.h +++ b/src/coreclr/jit/inlinepolicy.h @@ -212,6 +212,7 @@ class ExtendedDefaultPolicy : public DefaultPolicy , m_NonGenericCallsGeneric(false) , m_IsCallsiteInNoReturnRegion(false) , m_HasProfile(false) + , m_HasPinvoke(false) { // Empty } @@ -262,6 +263,7 @@ class ExtendedDefaultPolicy : public DefaultPolicy bool m_NonGenericCallsGeneric : 1; bool m_IsCallsiteInNoReturnRegion : 1; bool m_HasProfile : 1; + bool m_HasPinvoke : 1; }; // DiscretionaryPolicy is a variant of the default policy. It diff --git a/src/libraries/System.Private.CoreLib/src/System/ReadOnlySpan.cs b/src/libraries/System.Private.CoreLib/src/System/ReadOnlySpan.cs index 1dc3e052652fb7..6539097643a6f7 100644 --- a/src/libraries/System.Private.CoreLib/src/System/ReadOnlySpan.cs +++ b/src/libraries/System.Private.CoreLib/src/System/ReadOnlySpan.cs @@ -143,6 +143,7 @@ public ref readonly T this[int index] /// public int Length { + [Intrinsic] // a hint for the inliner that this method is special [NonVersionable] get => _length; } diff --git a/src/libraries/System.Private.CoreLib/src/System/Span.cs b/src/libraries/System.Private.CoreLib/src/System/Span.cs index a5580b2472d078..0b19343b2726a7 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Span.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Span.cs @@ -148,6 +148,7 @@ public ref T this[int index] /// public int Length { + [Intrinsic] // a hint for the inliner that this method is special [NonVersionable] get => _length; } From 69cea24024a19f57743ed4f9d40d3da0e1f2f2c3 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 14 Dec 2021 20:23:26 +0300 Subject: [PATCH 2/8] Add CONST_ARG_SETS_FLD heuristic to avoid write-barriers with const args Also, handle GetType intrinsic --- src/coreclr/jit/fgbasic.cpp | 55 +++++++++++++++++++++----------- src/coreclr/jit/inline.def | 3 +- src/coreclr/jit/inlinepolicy.cpp | 30 ++++++++++++----- src/coreclr/jit/inlinepolicy.h | 6 ++-- 4 files changed, 65 insertions(+), 29 deletions(-) diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index 43c1fc16873e6a..d7c0ed2d7ff704 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -838,6 +838,10 @@ class FgStack } static bool IsExactArgument(FgSlot value, InlineInfo* info) { + if (IsConstantOrConstArg(value, info)) + { + return true; + } if ((info == nullptr) || !IsArgument(value)) { return false; @@ -1107,7 +1111,7 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed { impResolveToken(codeAddr, &resolvedToken, CORINFO_TOKENKIND_Method); methodHnd = resolvedToken.hMethod; - isIntrinsic = eeIsJitIntrinsic(methodHnd); + isIntrinsic = eeIsIntrinsic(methodHnd); isPinvoke = info.compCompHnd->getMethodAttribs(methodHnd) & CORINFO_FLG_PINVOKE; } @@ -1147,6 +1151,17 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed break; } + case NI_System_Object_GetType: + { + if (FgStack::IsExactArgument(pushedStack.Top(0), impInlineInfo)) + { + // TODO: if arg is constant/valuetype/exact + pushedStack.PushConstant(); + foldableIntrinsc = true; + } + break; + } + // These are foldable if the first argument is a constant case NI_System_Type_get_IsValueType: case NI_System_Type_GetTypeFromHandle: @@ -1253,8 +1268,24 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed } break; + case CEE_STFLD: + case CEE_STSFLD: + { + if (makeInlineObservations) + { + // If we inline we'll end up setting a constant value to a field + // e.g. it might help us to avoid emitting a write barrier if the + // field is of a reference type and we set argument (which is null) to it. + if (FgStack::IsConstArgument(pushedStack.Top(), impInlineInfo)) + { + compInlineResult->Note(InlineObservation::CALLSITE_CONST_ARG_SETS_FLD); + } + } + } + break; + case CEE_LDSFLD: - if (resolveTokens && !isPreJit) + if (resolveTokens && !opts.jitFlags->IsSet(JitFlags::JIT_FLAG_PREJIT)) { // Push a constant to the stack for static readonly fields (initialized) // of primitive types. @@ -1553,7 +1584,7 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed if (FgStack::IsConstArgument(op1, impInlineInfo) || FgStack::IsConstArgument(op2, impInlineInfo)) { - compInlineResult->Note(InlineObservation::CALLSITE_CONSTANT_ARG_FEEDS_TEST); + compInlineResult->Note(InlineObservation::CALLSITE_CONST_ARG_FEEDS_TEST); } if ((FgStack::IsArgument(op1) && FgStack::IsArrayLen(op2)) || @@ -1602,18 +1633,6 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed } break; - case CEE_LDFLDA: - case CEE_LDFLD: - case CEE_STFLD: - { - if (FgStack::IsArgument(pushedStack.Top())) - { - compInlineResult->Note(InlineObservation::CALLEE_ARG_STRUCT_FIELD_ACCESS); - handled = true; // keep argument on top of the stack - } - break; - } - case CEE_LDELEM_I1: case CEE_LDELEM_U1: case CEE_LDELEM_I2: @@ -2302,7 +2321,7 @@ void Compiler::fgObserveInlineConstants(OPCODE opcode, const FgStack& stack, boo unsigned varNum = FgStack::SlotTypeToArgNum(slot0); if (impInlineInfo->inlArgInfo[varNum].argIsInvariant) { - compInlineResult->Note(InlineObservation::CALLSITE_CONSTANT_ARG_FEEDS_TEST); + compInlineResult->Note(InlineObservation::CALLSITE_CONST_ARG_FEEDS_TEST); } } } @@ -2344,7 +2363,7 @@ void Compiler::fgObserveInlineConstants(OPCODE opcode, const FgStack& stack, boo unsigned varNum = FgStack::SlotTypeToArgNum(slot0); if (impInlineInfo->inlArgInfo[varNum].argIsInvariant) { - compInlineResult->Note(InlineObservation::CALLSITE_CONSTANT_ARG_FEEDS_TEST); + compInlineResult->Note(InlineObservation::CALLSITE_CONST_ARG_FEEDS_TEST); } } @@ -2355,7 +2374,7 @@ void Compiler::fgObserveInlineConstants(OPCODE opcode, const FgStack& stack, boo unsigned varNum = FgStack::SlotTypeToArgNum(slot1); if (impInlineInfo->inlArgInfo[varNum].argIsInvariant) { - compInlineResult->Note(InlineObservation::CALLSITE_CONSTANT_ARG_FEEDS_TEST); + compInlineResult->Note(InlineObservation::CALLSITE_CONST_ARG_FEEDS_TEST); } } } diff --git a/src/coreclr/jit/inline.def b/src/coreclr/jit/inline.def index 28aa808668f83d..955ce2340320e2 100644 --- a/src/coreclr/jit/inline.def +++ b/src/coreclr/jit/inline.def @@ -184,7 +184,8 @@ INLINE_OBSERVATION(FOLDABLE_EXPR_UN, int, "foldable unary expression INLINE_OBSERVATION(FOLDABLE_BRANCH, int, "foldable branch", INFORMATION, CALLSITE) INLINE_OBSERVATION(FOLDABLE_SWITCH, int, "foldable switch", INFORMATION, CALLSITE) INLINE_OBSERVATION(DIV_BY_CNS, int, "dividy by const", INFORMATION, CALLSITE) -INLINE_OBSERVATION(CONSTANT_ARG_FEEDS_TEST, bool, "constant argument feeds test", INFORMATION, CALLSITE) +INLINE_OBSERVATION(CONST_ARG_FEEDS_TEST, bool, "constant argument feeds test", INFORMATION, CALLSITE) +INLINE_OBSERVATION(CONST_ARG_SETS_FLD, bool, "constant argument sets field", INFORMATION, CALLSITE) INLINE_OBSERVATION(DEPTH, int, "depth", INFORMATION, CALLSITE) INLINE_OBSERVATION(FREQUENCY, int, "rough call site frequency", INFORMATION, CALLSITE) INLINE_OBSERVATION(HAS_PROFILE, bool, "profile data is available", INFORMATION, CALLSITE) diff --git a/src/coreclr/jit/inlinepolicy.cpp b/src/coreclr/jit/inlinepolicy.cpp index 4b671cb1a65fdd..6dff7d4865a98d 100644 --- a/src/coreclr/jit/inlinepolicy.cpp +++ b/src/coreclr/jit/inlinepolicy.cpp @@ -330,11 +330,15 @@ void DefaultPolicy::NoteBool(InlineObservation obs, bool value) propagate = true; break; - case InlineObservation::CALLSITE_CONSTANT_ARG_FEEDS_TEST: + case InlineObservation::CALLSITE_CONST_ARG_SETS_FLD: + m_ConstArgSetsFld++; + break; + + case InlineObservation::CALLSITE_CONST_ARG_FEEDS_TEST: // We shouldn't see this for a prejit root since // we don't know anything about callers. assert(!m_IsPrejitRoot); - m_ConstantArgFeedsConstantTest++; + m_ConstArgFeedsConstantTest++; break; case InlineObservation::CALLEE_BEGIN_OPCODE_SCAN: @@ -727,7 +731,7 @@ double DefaultPolicy::DetermineMultiplier() JITDUMP("\nInline candidate has arg that feeds range check. Multiplier increased to %g.", multiplier); } - if (m_ConstantArgFeedsConstantTest > 0) + if (m_ConstArgFeedsConstantTest > 0) { multiplier += 3.0; JITDUMP("\nInline candidate has const arg that feeds a conditional. Multiplier increased to %g.", multiplier); @@ -998,7 +1002,8 @@ void DefaultPolicy::OnDumpXml(FILE* file, unsigned indent) const XATTR_I4(m_ArgFeedsTest); XATTR_I4(m_ArgFeedsConstantTest); XATTR_I4(m_ArgFeedsRangeCheck); - XATTR_I4(m_ConstantArgFeedsConstantTest); + XATTR_I4(m_ConstArgFeedsConstantTest); + XATTR_I4(m_ConstArgSetsFld); XATTR_I4(m_CalleeNativeSizeEstimate); XATTR_I4(m_CallsiteNativeSizeEstimate); XATTR_B(m_IsForceInline); @@ -1493,7 +1498,7 @@ double ExtendedDefaultPolicy::DetermineMultiplier() JITDUMP("\nInline candidate has %d foldable branches. Multiplier increased to %g.", m_FoldableBranch, multiplier); } - else if (m_ConstantArgFeedsConstantTest > 0) + else if (m_ConstArgFeedsConstantTest > 0) { multiplier += 3.0; JITDUMP("\nInline candidate has const arg that feeds a conditional. Multiplier increased to %g.", multiplier); @@ -1503,7 +1508,15 @@ double ExtendedDefaultPolicy::DetermineMultiplier() // TODO: handle 'if (SomeMethod(constArg))' patterns in fgFindJumpTargets // The previous version of inliner optimistically assumed this is "has const arg that feeds a conditional" multiplier += 3.0; - JITDUMP("\nCallsite passes a consant. Multiplier increased to %g.", multiplier); + JITDUMP("\nCallsite passes a constant. Multiplier increased to %g.", multiplier); + } + + if (m_ConstArgSetsFld > 0) + { + // What it means that for this specific st(s)fld we can avoid emitting a heavy write-barrier + // if we inline the candidate + multiplier += 1.5 + m_ConstArgSetsFld; + JITDUMP("\nInline candidate has const arg that sets %d fields. Multiplier increased to %g.", m_ConstArgSetsFld, multiplier); } if ((m_FoldableBox > 0) && m_NonGenericCallsGeneric) @@ -2460,7 +2473,7 @@ void DiscretionaryPolicy::EstimateCodeSize() 6.021 * m_CallCount + -0.238 * m_IsInstanceCtor + -5.357 * m_IsFromPromotableValueClass + - -7.901 * (m_ConstantArgFeedsConstantTest > 0 ? 1 : 0) + + -7.901 * (m_ConstArgFeedsConstantTest > 0 ? 1 : 0) + 0.065 * m_CalleeNativeSizeEstimate; // clang-format on @@ -2660,7 +2673,8 @@ void DiscretionaryPolicy::DumpData(FILE* file) const fprintf(file, ",%u", m_ArgFeedsConstantTest); fprintf(file, ",%u", m_MethodIsMostlyLoadStore ? 1 : 0); fprintf(file, ",%u", m_ArgFeedsRangeCheck); - fprintf(file, ",%u", m_ConstantArgFeedsConstantTest); + fprintf(file, ",%u", m_ConstArgFeedsConstantTest); + fprintf(file, ",%u", m_ConstArgSetsFld); fprintf(file, ",%d", m_CalleeNativeSizeEstimate); fprintf(file, ",%d", m_CallsiteNativeSizeEstimate); fprintf(file, ",%d", m_ModelCodeSizeEstimate); diff --git a/src/coreclr/jit/inlinepolicy.h b/src/coreclr/jit/inlinepolicy.h index a85a466ba79e0d..003d835c8998b5 100644 --- a/src/coreclr/jit/inlinepolicy.h +++ b/src/coreclr/jit/inlinepolicy.h @@ -96,7 +96,8 @@ class DefaultPolicy : public LegalPolicy , m_ArgFeedsTest(0) , m_ArgFeedsConstantTest(0) , m_ArgFeedsRangeCheck(0) - , m_ConstantArgFeedsConstantTest(0) + , m_ConstArgFeedsConstantTest(0) + , m_ConstArgSetsFld(0) , m_CalleeNativeSizeEstimate(0) , m_CallsiteNativeSizeEstimate(0) , m_IsForceInline(false) @@ -164,7 +165,8 @@ class DefaultPolicy : public LegalPolicy unsigned m_ArgFeedsTest; unsigned m_ArgFeedsConstantTest; unsigned m_ArgFeedsRangeCheck; - unsigned m_ConstantArgFeedsConstantTest; + unsigned m_ConstArgFeedsConstantTest; + unsigned m_ConstArgSetsFld; int m_CalleeNativeSizeEstimate; int m_CallsiteNativeSizeEstimate; bool m_IsForceInline : 1; From f69b81a9078b96b245ce09a5e75f773f62426ab3 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 14 Dec 2021 20:24:37 +0300 Subject: [PATCH 3/8] formatting --- src/coreclr/jit/inlinepolicy.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/inlinepolicy.cpp b/src/coreclr/jit/inlinepolicy.cpp index 6dff7d4865a98d..4bbd400946ad93 100644 --- a/src/coreclr/jit/inlinepolicy.cpp +++ b/src/coreclr/jit/inlinepolicy.cpp @@ -1516,7 +1516,8 @@ double ExtendedDefaultPolicy::DetermineMultiplier() // What it means that for this specific st(s)fld we can avoid emitting a heavy write-barrier // if we inline the candidate multiplier += 1.5 + m_ConstArgSetsFld; - JITDUMP("\nInline candidate has const arg that sets %d fields. Multiplier increased to %g.", m_ConstArgSetsFld, multiplier); + JITDUMP("\nInline candidate has const arg that sets %d fields. Multiplier increased to %g.", m_ConstArgSetsFld, + multiplier); } if ((m_FoldableBox > 0) && m_NonGenericCallsGeneric) From 8e8f24ddd3c6743b889edc5829fd76ce6bbd65c3 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 24 Dec 2021 02:51:53 +0300 Subject: [PATCH 4/8] Fix regressions --- src/coreclr/jit/fgbasic.cpp | 19 +++++++++++++++++-- src/coreclr/jit/importer.cpp | 8 ++++++++ src/coreclr/jit/inline.def | 1 + src/coreclr/jit/inlinepolicy.cpp | 14 +++++++++++++- src/coreclr/jit/inlinepolicy.h | 2 ++ src/coreclr/jit/namedintrinsiclist.h | 2 ++ 6 files changed, 43 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index d7c0ed2d7ff704..0985393c7bd6d0 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -1133,7 +1133,6 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed // These are most likely foldable without arguments case NI_System_Collections_Generic_Comparer_get_Default: case NI_System_Collections_Generic_EqualityComparer_get_Default: - case NI_System_Enum_HasFlag: case NI_System_GC_KeepAlive: { pushedStack.PushUnknown(); @@ -1141,6 +1140,13 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed break; } + case NI_System_Enum_HasFlag: + { + // keep its argument in the stack as a return value + foldableIntrinsc = true; + break; + } + case NI_System_Span_get_Item: case NI_System_ReadOnlySpan_get_Item: { @@ -1151,11 +1157,19 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed break; } + case NI_System_Span_get_Length: + case NI_System_ReadOnlySpan_get_Length: + { + // Keep argument on top of the stack + handled = FgStack::IsArgument(pushedStack.Top(0)); + break; + } + case NI_System_Object_GetType: { + // obj.GetType() is folded into typeof() if obj is exact if (FgStack::IsExactArgument(pushedStack.Top(0), impInlineInfo)) { - // TODO: if arg is constant/valuetype/exact pushedStack.PushConstant(); foldableIntrinsc = true; } @@ -1294,6 +1308,7 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed bool inited = false; if ((info.compCompHnd->getStaticFieldCurrentClass(fldHnd, &inited) == NO_CLASS_HANDLE) && inited) { + compInlineResult->Note(InlineObservation::CALLSITE_FOLDABLE_LDSFLD); pushedStack.PushConstant(); handled = true; } diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 7e3a08a30c743b..5088b8064b1d05 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -5239,6 +5239,10 @@ NamedIntrinsic Compiler::lookupNamedIntrinsic(CORINFO_METHOD_HANDLE method) { result = NI_System_Span_get_Item; } + else if (strcmp(methodName, "get_Length") == 0) + { + result = NI_System_Span_get_Length; + } } else if (strcmp(className, "ReadOnlySpan`1") == 0) { @@ -5246,6 +5250,10 @@ NamedIntrinsic Compiler::lookupNamedIntrinsic(CORINFO_METHOD_HANDLE method) { result = NI_System_ReadOnlySpan_get_Item; } + else if (strcmp(methodName, "get_Length") == 0) + { + result = NI_System_ReadOnlySpan_get_Length; + } } else if (strcmp(className, "EETypePtr") == 0) { diff --git a/src/coreclr/jit/inline.def b/src/coreclr/jit/inline.def index 955ce2340320e2..be44344d33ca2a 100644 --- a/src/coreclr/jit/inline.def +++ b/src/coreclr/jit/inline.def @@ -179,6 +179,7 @@ INLINE_OBSERVATION(ARG_EXACT_CLS_SIG_IS_NOT, int, "arg is more concrete than INLINE_OBSERVATION(ARG_CONST, int, "arg is a constant", INFORMATION, CALLSITE) INLINE_OBSERVATION(ARG_BOXED, int, "arg is boxed at call site", INFORMATION, CALLSITE) INLINE_OBSERVATION(FOLDABLE_INTRINSIC, int, "foldable intrinsic", INFORMATION, CALLSITE) +INLINE_OBSERVATION(FOLDABLE_LDSFLD, int, "foldable ldsfld", INFORMATION, CALLSITE) INLINE_OBSERVATION(FOLDABLE_EXPR, int, "foldable binary expression", INFORMATION, CALLSITE) INLINE_OBSERVATION(FOLDABLE_EXPR_UN, int, "foldable unary expression", INFORMATION, CALLSITE) INLINE_OBSERVATION(FOLDABLE_BRANCH, int, "foldable branch", INFORMATION, CALLSITE) diff --git a/src/coreclr/jit/inlinepolicy.cpp b/src/coreclr/jit/inlinepolicy.cpp index 4bbd400946ad93..e5d6df5bbf34d7 100644 --- a/src/coreclr/jit/inlinepolicy.cpp +++ b/src/coreclr/jit/inlinepolicy.cpp @@ -1286,6 +1286,10 @@ void ExtendedDefaultPolicy::NoteBool(InlineObservation obs, bool value) m_FoldableIntrinsic++; break; + case InlineObservation::CALLSITE_FOLDABLE_LDSFLD: + m_FoldableLdsfld++; + break; + case InlineObservation::CALLSITE_FOLDABLE_EXPR: m_FoldableExpr++; break; @@ -1583,6 +1587,13 @@ double ExtendedDefaultPolicy::DetermineMultiplier() JITDUMP("\nInline has %d foldable intrinsics. Multiplier increased to %g.", m_FoldableIntrinsic, multiplier); } + if (m_FoldableLdsfld > 0) + { + // static readonly fields of already initialized types + multiplier += m_FoldableLdsfld; + JITDUMP("\nInline has %d foldable ldslfd. Multiplier increased to %g.", m_FoldableLdsfld, multiplier); + } + if (m_FoldableExpr > 0) { // E.g. add/mul/ceq, etc. over constant/constant arguments @@ -1616,7 +1627,7 @@ double ExtendedDefaultPolicy::DetermineMultiplier() // ceq // // so at least we can note potential constant tests - multiplier += m_BinaryExprWithCns * 0.5; + multiplier += 1.5 + m_BinaryExprWithCns * 0.5; JITDUMP("\nInline candidate has %d binary expressions with constants. Multiplier increased to %g.", m_BinaryExprWithCns, multiplier); @@ -1807,6 +1818,7 @@ void ExtendedDefaultPolicy::OnDumpXml(FILE* file, unsigned indent) const XATTR_I4(m_ArgIsConst) XATTR_I4(m_ArgIsBoxedAtCallsite) XATTR_I4(m_FoldableIntrinsic) + XATTR_I4(m_FoldableLdsfld) XATTR_I4(m_FoldableExpr) XATTR_I4(m_FoldableExprUn) XATTR_I4(m_FoldableBranch) diff --git a/src/coreclr/jit/inlinepolicy.h b/src/coreclr/jit/inlinepolicy.h index 003d835c8998b5..3441e1051a375b 100644 --- a/src/coreclr/jit/inlinepolicy.h +++ b/src/coreclr/jit/inlinepolicy.h @@ -203,6 +203,7 @@ class ExtendedDefaultPolicy : public DefaultPolicy , m_ArgIsConst(0) , m_ArgIsBoxedAtCallsite(0) , m_FoldableIntrinsic(0) + , m_FoldableLdsfld(0) , m_FoldableExpr(0) , m_FoldableExprUn(0) , m_FoldableBranch(0) @@ -254,6 +255,7 @@ class ExtendedDefaultPolicy : public DefaultPolicy unsigned m_ArgIsConst; unsigned m_ArgIsBoxedAtCallsite; unsigned m_FoldableIntrinsic; + unsigned m_FoldableLdsfld; unsigned m_FoldableExpr; unsigned m_FoldableExprUn; unsigned m_FoldableBranch; diff --git a/src/coreclr/jit/namedintrinsiclist.h b/src/coreclr/jit/namedintrinsiclist.h index 25733a41a9c9ed..4538de4283a527 100644 --- a/src/coreclr/jit/namedintrinsiclist.h +++ b/src/coreclr/jit/namedintrinsiclist.h @@ -82,7 +82,9 @@ enum NamedIntrinsic : unsigned short NI_System_String_get_Chars, NI_System_String_get_Length, NI_System_Span_get_Item, + NI_System_Span_get_Length, NI_System_ReadOnlySpan_get_Item, + NI_System_ReadOnlySpan_get_Length, // These are used by HWIntrinsics but are defined more generally // to allow dead code optimization and handle the recursion case From 675edded3758ea791562fa1469ca4969f97a29d5 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 24 Dec 2021 03:04:17 +0300 Subject: [PATCH 5/8] recognize constants in isinst(exact-arg) --- src/coreclr/jit/fgbasic.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index 0985393c7bd6d0..b1db1118671a62 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -1076,8 +1076,12 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed if (FgStack::IsConstantOrConstArg(slot, impInlineInfo) || FgStack::IsExactArgument(slot, impInlineInfo)) { + if (opcode == CEE_ISINST) + { + pushedStack.PushConstant(); + } compInlineResult->Note(InlineObservation::CALLSITE_FOLDABLE_EXPR_UN); - handled = true; // and keep argument in the pushedStack + handled = true; } else if (FgStack::IsArgument(slot)) { From 195bd3f0b8b880a50e2de210a55c73853a82cad0 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 24 Dec 2021 03:20:16 +0300 Subject: [PATCH 6/8] Tuning --- src/coreclr/jit/inlinepolicy.cpp | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/coreclr/jit/inlinepolicy.cpp b/src/coreclr/jit/inlinepolicy.cpp index e5d6df5bbf34d7..938018b0ddfabb 100644 --- a/src/coreclr/jit/inlinepolicy.cpp +++ b/src/coreclr/jit/inlinepolicy.cpp @@ -1446,19 +1446,19 @@ double ExtendedDefaultPolicy::DetermineMultiplier() if (m_ReturnsStructByValue) { // For structs-passed-by-value we might avoid expensive copy operations if we inline. - multiplier += 2.0; + multiplier += 2.5; JITDUMP("\nInline candidate returns a struct by value. Multiplier increased to %g.", multiplier); } else if (m_ArgIsStructByValue > 0) { // Same here - multiplier += 2.0; + multiplier += 2.5; JITDUMP("\n%d arguments are structs passed by value. Multiplier increased to %g.", m_ArgIsStructByValue, multiplier); } else if (m_FldAccessOverArgStruct > 0) { - multiplier += 1.0; + multiplier += 1.5; // Such ldfld/stfld are cheap for promotable structs JITDUMP("\n%d ldfld or stfld over arguments which are structs. Multiplier increased to %g.", m_FldAccessOverArgStruct, multiplier); @@ -1484,7 +1484,7 @@ double ExtendedDefaultPolicy::DetermineMultiplier() if (m_NonGenericCallsGeneric) { - multiplier += 2.0; + multiplier += 4.0; JITDUMP("\nInline candidate is generic and caller is not. Multiplier increased to %g.", multiplier); } @@ -1498,7 +1498,7 @@ double ExtendedDefaultPolicy::DetermineMultiplier() // if (Math.Abs(arg0) > 10) { // same here // etc. // - multiplier += 3.0 + m_FoldableBranch * 1.5; + multiplier += 4.0 + m_FoldableBranch * 1.5; JITDUMP("\nInline candidate has %d foldable branches. Multiplier increased to %g.", m_FoldableBranch, multiplier); } @@ -1597,7 +1597,7 @@ double ExtendedDefaultPolicy::DetermineMultiplier() if (m_FoldableExpr > 0) { // E.g. add/mul/ceq, etc. over constant/constant arguments - multiplier += 1.0 + m_FoldableExpr; + multiplier += 1.5 + m_FoldableExpr; JITDUMP("\nInline has %d foldable binary expressions. Multiplier increased to %g.", m_FoldableExpr, multiplier); } @@ -1605,7 +1605,7 @@ double ExtendedDefaultPolicy::DetermineMultiplier() if (m_FoldableExprUn > 0) { // E.g. casts, negations, etc. over constants/constant arguments - multiplier += m_FoldableExprUn; + multiplier += 0.5 + m_FoldableExprUn; JITDUMP("\nInline has %d foldable unary expressions. Multiplier increased to %g.", m_FoldableExprUn, multiplier); } @@ -1655,8 +1655,8 @@ double ExtendedDefaultPolicy::DetermineMultiplier() { case InlineCallsiteFrequency::RARE: // Note this one is not additive, it uses '=' instead of '+=' - multiplier = 1.3; - JITDUMP("\nInline candidate callsite is rare. Multiplier limited to %g.", multiplier); + multiplier /= 2; + JITDUMP("\nInline candidate callsite is rare. Multiplier decreased to %g.", multiplier); break; case InlineCallsiteFrequency::BORING: multiplier += 1.3; From f7c3276a1744ba6ab8818b36ae86e01d9d47471d Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 25 Dec 2021 01:23:15 +0300 Subject: [PATCH 7/8] Revert some changes --- src/coreclr/jit/fgbasic.cpp | 86 ++++++++++++++++++-------------- src/coreclr/jit/inlinepolicy.cpp | 22 ++++---- 2 files changed, 60 insertions(+), 48 deletions(-) diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index b1db1118671a62..c2ba525248bac1 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -838,10 +838,6 @@ class FgStack } static bool IsExactArgument(FgSlot value, InlineInfo* info) { - if (IsConstantOrConstArg(value, info)) - { - return true; - } if ((info == nullptr) || !IsArgument(value)) { return false; @@ -1076,12 +1072,14 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed if (FgStack::IsConstantOrConstArg(slot, impInlineInfo) || FgStack::IsExactArgument(slot, impInlineInfo)) { - if (opcode == CEE_ISINST) + if ((opcode == CEE_ISINST) && FgStack::IsExactArgument(slot, impInlineInfo)) { + // isinst on top of non-constant "exact" argument (at callsite) + // most likely will fold this isinst pushedStack.PushConstant(); } compInlineResult->Note(InlineObservation::CALLSITE_FOLDABLE_EXPR_UN); - handled = true; + handled = true; // and keep argument in the pushedStack } else if (FgStack::IsArgument(slot)) { @@ -1172,11 +1170,13 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed case NI_System_Object_GetType: { // obj.GetType() is folded into typeof() if obj is exact - if (FgStack::IsExactArgument(pushedStack.Top(0), impInlineInfo)) + if (FgStack::IsExactArgument(pushedStack.Top(0), impInlineInfo) || + FgStack::IsConstantOrConstArg(pushedStack.Top(), impInlineInfo)) { pushedStack.PushConstant(); foldableIntrinsc = true; } + else if () break; } @@ -1283,42 +1283,17 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed // it is a wrapper method. compInlineResult->Note(InlineObservation::CALLEE_LOOKS_LIKE_WRAPPER); } - } - break; - case CEE_STFLD: - case CEE_STSFLD: - { - if (makeInlineObservations) + if (!isIntrinsic && !handled && FgStack::IsArgument(pushedStack.Top())) { - // If we inline we'll end up setting a constant value to a field - // e.g. it might help us to avoid emitting a write barrier if the - // field is of a reference type and we set argument (which is null) to it. - if (FgStack::IsConstArgument(pushedStack.Top(), impInlineInfo)) - { - compInlineResult->Note(InlineObservation::CALLSITE_CONST_ARG_SETS_FLD); - } + // Optimistically assume that "call(arg)" returns something arg-dependent. + // However, we don't know how many args it expects and its return type. + // TODO: Make IL scan 100% precise so we can guess it looking at the stack. + handled = true; } } break; - case CEE_LDSFLD: - if (resolveTokens && !opts.jitFlags->IsSet(JitFlags::JIT_FLAG_PREJIT)) - { - // Push a constant to the stack for static readonly fields (initialized) - // of primitive types. - impResolveToken(codeAddr, &resolvedToken, CORINFO_TOKENKIND_Field); - CORINFO_FIELD_HANDLE fldHnd = resolvedToken.hField; - bool inited = false; - if ((info.compCompHnd->getStaticFieldCurrentClass(fldHnd, &inited) == NO_CLASS_HANDLE) && inited) - { - compInlineResult->Note(InlineObservation::CALLSITE_FOLDABLE_LDSFLD); - pushedStack.PushConstant(); - handled = true; - } - } - break; - case CEE_LDIND_I1: case CEE_LDIND_U1: case CEE_LDIND_I2: @@ -1652,6 +1627,43 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed } break; + case CEE_LDFLDA: + case CEE_LDFLD: + case CEE_STFLD: + case CEE_STSFLD: + if (makeInlineObservations) + { + if (FgStack::IsConstArgument(pushedStack.Top(), impInlineInfo) && ((opcode == CEE_STFLD) || (opcode == CEE_STSFLD))) + { + // If we inline we'll set a constant value (e.g. null) to this field + // If it's of reference type we'll avoid an expensive write barrier here + compInlineResult->Note(InlineObservation::CALLSITE_CONST_ARG_SETS_FLD); + } + else if (FgStack::IsArgument(pushedStack.Top())) + { + compInlineResult->Note(InlineObservation::CALLEE_ARG_STRUCT_FIELD_ACCESS); + handled = true; // keep argument on top of the stack + } + break; + } + break; + + case CEE_LDSFLD: + if (resolveTokens && !opts.jitFlags->IsSet(JitFlags::JIT_FLAG_PREJIT)) + { + // Push a constant to the stack for static readonly fields (initialized) + impResolveToken(codeAddr, &resolvedToken, CORINFO_TOKENKIND_Field); + CORINFO_FIELD_HANDLE fldHnd = resolvedToken.hField; + bool inited = false; + if ((info.compCompHnd->getStaticFieldCurrentClass(fldHnd, &inited) == NO_CLASS_HANDLE) && inited) + { + compInlineResult->Note(InlineObservation::CALLSITE_FOLDABLE_LDSFLD); + pushedStack.PushConstant(); + handled = true; + } + } + break; + case CEE_LDELEM_I1: case CEE_LDELEM_U1: case CEE_LDELEM_I2: diff --git a/src/coreclr/jit/inlinepolicy.cpp b/src/coreclr/jit/inlinepolicy.cpp index 938018b0ddfabb..7e7fe669a7ae26 100644 --- a/src/coreclr/jit/inlinepolicy.cpp +++ b/src/coreclr/jit/inlinepolicy.cpp @@ -1446,19 +1446,19 @@ double ExtendedDefaultPolicy::DetermineMultiplier() if (m_ReturnsStructByValue) { // For structs-passed-by-value we might avoid expensive copy operations if we inline. - multiplier += 2.5; + multiplier += 2.0; JITDUMP("\nInline candidate returns a struct by value. Multiplier increased to %g.", multiplier); } else if (m_ArgIsStructByValue > 0) { // Same here - multiplier += 2.5; + multiplier += 2.0; JITDUMP("\n%d arguments are structs passed by value. Multiplier increased to %g.", m_ArgIsStructByValue, multiplier); } else if (m_FldAccessOverArgStruct > 0) { - multiplier += 1.5; + multiplier += 1.0; // Such ldfld/stfld are cheap for promotable structs JITDUMP("\n%d ldfld or stfld over arguments which are structs. Multiplier increased to %g.", m_FldAccessOverArgStruct, multiplier); @@ -1484,7 +1484,7 @@ double ExtendedDefaultPolicy::DetermineMultiplier() if (m_NonGenericCallsGeneric) { - multiplier += 4.0; + multiplier += 2.0; JITDUMP("\nInline candidate is generic and caller is not. Multiplier increased to %g.", multiplier); } @@ -1498,7 +1498,7 @@ double ExtendedDefaultPolicy::DetermineMultiplier() // if (Math.Abs(arg0) > 10) { // same here // etc. // - multiplier += 4.0 + m_FoldableBranch * 1.5; + multiplier += 3.0 + m_FoldableBranch; JITDUMP("\nInline candidate has %d foldable branches. Multiplier increased to %g.", m_FoldableBranch, multiplier); } @@ -1519,7 +1519,7 @@ double ExtendedDefaultPolicy::DetermineMultiplier() { // What it means that for this specific st(s)fld we can avoid emitting a heavy write-barrier // if we inline the candidate - multiplier += 1.5 + m_ConstArgSetsFld; + multiplier += m_ConstArgSetsFld; JITDUMP("\nInline candidate has const arg that sets %d fields. Multiplier increased to %g.", m_ConstArgSetsFld, multiplier); } @@ -1597,7 +1597,7 @@ double ExtendedDefaultPolicy::DetermineMultiplier() if (m_FoldableExpr > 0) { // E.g. add/mul/ceq, etc. over constant/constant arguments - multiplier += 1.5 + m_FoldableExpr; + multiplier += 1.0 + m_FoldableExpr; JITDUMP("\nInline has %d foldable binary expressions. Multiplier increased to %g.", m_FoldableExpr, multiplier); } @@ -1605,7 +1605,7 @@ double ExtendedDefaultPolicy::DetermineMultiplier() if (m_FoldableExprUn > 0) { // E.g. casts, negations, etc. over constants/constant arguments - multiplier += 0.5 + m_FoldableExprUn; + multiplier += m_FoldableExprUn; JITDUMP("\nInline has %d foldable unary expressions. Multiplier increased to %g.", m_FoldableExprUn, multiplier); } @@ -1627,7 +1627,7 @@ double ExtendedDefaultPolicy::DetermineMultiplier() // ceq // // so at least we can note potential constant tests - multiplier += 1.5 + m_BinaryExprWithCns * 0.5; + multiplier += m_BinaryExprWithCns * 0.5; JITDUMP("\nInline candidate has %d binary expressions with constants. Multiplier increased to %g.", m_BinaryExprWithCns, multiplier); @@ -1655,8 +1655,8 @@ double ExtendedDefaultPolicy::DetermineMultiplier() { case InlineCallsiteFrequency::RARE: // Note this one is not additive, it uses '=' instead of '+=' - multiplier /= 2; - JITDUMP("\nInline candidate callsite is rare. Multiplier decreased to %g.", multiplier); + multiplier = 1.3; + JITDUMP("\nInline candidate callsite is rare. Multiplier limited to %g.", multiplier); break; case InlineCallsiteFrequency::BORING: multiplier += 1.3; From 28385290faf3f5d3d6d9b10aae75c5129fbec675 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 25 Dec 2021 01:27:39 +0300 Subject: [PATCH 8/8] Clean up --- src/coreclr/jit/fgbasic.cpp | 10 +++++----- src/coreclr/jit/inlinepolicy.cpp | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index c2ba525248bac1..fb017083c59ee2 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -1075,7 +1075,7 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed if ((opcode == CEE_ISINST) && FgStack::IsExactArgument(slot, impInlineInfo)) { // isinst on top of non-constant "exact" argument (at callsite) - // most likely will fold this isinst + // most likely will fold this isinst pushedStack.PushConstant(); } compInlineResult->Note(InlineObservation::CALLSITE_FOLDABLE_EXPR_UN); @@ -1176,7 +1176,6 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed pushedStack.PushConstant(); foldableIntrinsc = true; } - else if () break; } @@ -1633,9 +1632,10 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed case CEE_STSFLD: if (makeInlineObservations) { - if (FgStack::IsConstArgument(pushedStack.Top(), impInlineInfo) && ((opcode == CEE_STFLD) || (opcode == CEE_STSFLD))) + if (FgStack::IsConstArgument(pushedStack.Top(), impInlineInfo) && + ((opcode == CEE_STFLD) || (opcode == CEE_STSFLD))) { - // If we inline we'll set a constant value (e.g. null) to this field + // If we inline we'll set a constant value (e.g. null) to this field // If it's of reference type we'll avoid an expensive write barrier here compInlineResult->Note(InlineObservation::CALLSITE_CONST_ARG_SETS_FLD); } @@ -1646,7 +1646,7 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed } break; } - break; + break; case CEE_LDSFLD: if (resolveTokens && !opts.jitFlags->IsSet(JitFlags::JIT_FLAG_PREJIT)) diff --git a/src/coreclr/jit/inlinepolicy.cpp b/src/coreclr/jit/inlinepolicy.cpp index 7e7fe669a7ae26..a50eb6029ad66a 100644 --- a/src/coreclr/jit/inlinepolicy.cpp +++ b/src/coreclr/jit/inlinepolicy.cpp @@ -1656,7 +1656,7 @@ double ExtendedDefaultPolicy::DetermineMultiplier() case InlineCallsiteFrequency::RARE: // Note this one is not additive, it uses '=' instead of '+=' multiplier = 1.3; - JITDUMP("\nInline candidate callsite is rare. Multiplier limited to %g.", multiplier); + JITDUMP("\nInline candidate callsite is rare. Multiplier limited to %g.", multiplier); break; case InlineCallsiteFrequency::BORING: multiplier += 1.3;