From 95f10fc435edf9f042f64819090b4d7ae5b65d78 Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Thu, 1 Apr 2021 09:28:18 -0700 Subject: [PATCH 1/4] Add some basic handling to allow resolving call tokens in fgFindJumpTargets when in Tier1 making inline observations --- src/coreclr/jit/compiler.h | 20 ++++++ src/coreclr/jit/fgbasic.cpp | 131 +++++++++++++++++++++++++++++++---- src/coreclr/jit/importer.cpp | 21 +----- 3 files changed, 141 insertions(+), 31 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 6138d87379c2fe..00dfbf90fc6360 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -3844,6 +3844,26 @@ class Compiler XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX */ +private: + // For prefixFlags + enum + { + PREFIX_TAILCALL_EXPLICIT = 0x00000001, // call has "tail" IL prefix + PREFIX_TAILCALL_IMPLICIT = + 0x00000010, // call is treated as having "tail" prefix even though there is no "tail" IL prefix + PREFIX_TAILCALL_STRESS = + 0x00000100, // call doesn't "tail" IL prefix but is treated as explicit because of tail call stress + PREFIX_TAILCALL = (PREFIX_TAILCALL_EXPLICIT | PREFIX_TAILCALL_IMPLICIT | PREFIX_TAILCALL_STRESS), + PREFIX_VOLATILE = 0x00001000, + PREFIX_UNALIGNED = 0x00010000, + PREFIX_CONSTRAINED = 0x00100000, + PREFIX_READONLY = 0x01000000 + }; + + static void impValidateMemoryAccessOpcode(const BYTE* codeAddr, const BYTE* codeEndp, bool volatilePrefix); + static OPCODE impGetNonPrefixOpcode(const BYTE* codeAddr, const BYTE* codeEndp); + static bool impOpcodeIsCallOpcode(OPCODE opcode); + public: void impInit(); void impImport(); diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index 9ac2e8bc9fe564..e35edb3aeb57ad 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -849,7 +849,12 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed const bool isForceInline = (info.compFlags & CORINFO_FLG_FORCEINLINE) != 0; const bool makeInlineObservations = (compInlineResult != nullptr); const bool isInlining = compIsForInlining(); + const bool isTier1 = opts.jitFlags->IsSet(JitFlags::JIT_FLAG_TIER1); + const bool resolveTokens = makeInlineObservations && isTier1; unsigned retBlocks = 0; + unsigned intrinsicCalls = 0; + int prefixFlags = 0; + int value = 0; if (makeInlineObservations) { @@ -883,12 +888,14 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed compInlineResult->Note(InlineObservation::CALLEE_BEGIN_OPCODE_SCAN); } + CORINFO_RESOLVED_TOKEN resolvedToken; + CORINFO_RESOLVED_TOKEN constrainedResolvedToken; + CORINFO_CALL_INFO callInfo; + while (codeAddr < codeEndp) { OPCODE opcode = (OPCODE)getU1LittleEndian(codeAddr); codeAddr += sizeof(__int8); - opts.instrCount++; - typeIsNormed = false; DECODE_OPCODE: @@ -939,18 +946,43 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed // There has to be code after the call, otherwise the inlinee is unverifiable. if (isInlining) { - noway_assert(codeAddr < codeEndp - sz); } - // If the method has a call followed by a ret, assume that - // it is a wrapper method. - if (makeInlineObservations) + if (!makeInlineObservations) { - if ((OPCODE)getU1LittleEndian(codeAddr + sz) == CEE_RET) - { - compInlineResult->Note(InlineObservation::CALLEE_LOOKS_LIKE_WRAPPER); - } + break; + } + + CORINFO_METHOD_HANDLE methodHnd = nullptr; + unsigned methodFlags = 0; + bool mustExpand = false; + CorInfoIntrinsics intrinsicID = CORINFO_INTRINSIC_Illegal; + NamedIntrinsic ni = NI_Illegal; + + if (resolveTokens) + { + impResolveToken(codeAddr, &resolvedToken, CORINFO_TOKENKIND_Method); + eeGetCallInfo(&resolvedToken, + (prefixFlags & PREFIX_CONSTRAINED) ? &constrainedResolvedToken : nullptr, + combine(CORINFO_CALLINFO_KINDONLY, + (opcode == CEE_CALLVIRT) ? CORINFO_CALLINFO_CALLVIRT : CORINFO_CALLINFO_NONE), + &callInfo); + + methodHnd = callInfo.hMethod; + methodFlags = callInfo.methodFlags; + } + + if ((methodFlags & (CORINFO_FLG_INTRINSIC | CORINFO_FLG_JIT_INTRINSIC)) != 0) + { + intrinsicCalls++; + } + + if ((OPCODE)getU1LittleEndian(codeAddr + sz) == CEE_RET) + { + // If the method has a call followed by a ret, assume that + // it is a wrapper method. + compInlineResult->Note(InlineObservation::CALLEE_LOOKS_LIKE_WRAPPER); } } break; @@ -1082,17 +1114,79 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed break; case CEE_UNALIGNED: + { + noway_assert(sz == sizeof(__int8)); + prefixFlags |= PREFIX_UNALIGNED; + + value = getU1LittleEndian(codeAddr); + codeAddr += sizeof(__int8); + + impValidateMemoryAccessOpcode(codeAddr, codeEndp, false); + goto OBSERVE_OPCODE; + } + case CEE_CONSTRAINED: + { + noway_assert(sz == sizeof(unsigned)); + prefixFlags |= PREFIX_CONSTRAINED; + + if (resolveTokens) + { + impResolveToken(codeAddr, &constrainedResolvedToken, CORINFO_TOKENKIND_Constrained); + } + codeAddr += sizeof(unsigned); + + { + OPCODE actualOpcode = impGetNonPrefixOpcode(codeAddr, codeEndp); + + if (actualOpcode != CEE_CALLVIRT) + { + BADCODE("constrained. has to be followed by callvirt"); + } + } + goto OBSERVE_OPCODE; + } + case CEE_READONLY: + { + noway_assert(sz == 0); + prefixFlags |= PREFIX_READONLY; + + { + OPCODE actualOpcode = impGetNonPrefixOpcode(codeAddr, codeEndp); + + if ((actualOpcode != CEE_LDELEMA) && !impOpcodeIsCallOpcode(actualOpcode)) + { + BADCODE("readonly. has to be followed by ldelema or call"); + } + } + goto OBSERVE_OPCODE; + } + case CEE_VOLATILE: + { + noway_assert(sz == 0); + prefixFlags |= PREFIX_VOLATILE; + + impValidateMemoryAccessOpcode(codeAddr, codeEndp, true); + goto OBSERVE_OPCODE; + } + case CEE_TAILCALL: { - if (codeAddr >= codeEndp) + noway_assert(sz == 0); + prefixFlags |= PREFIX_TAILCALL_EXPLICIT; + { - goto TOO_FAR; + OPCODE actualOpcode = impGetNonPrefixOpcode(codeAddr, codeEndp); + + if (!impOpcodeIsCallOpcode(actualOpcode)) + { + BADCODE("tailcall. has to be followed by call, callvirt or calli"); + } } + goto OBSERVE_OPCODE; } - break; case CEE_STARG: case CEE_STARG_S: @@ -1383,6 +1477,7 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed fgObserveInlineConstants(opcode, pushedStack, isInlining); } break; + case CEE_RET: retBlocks++; break; @@ -1394,6 +1489,14 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed // Skip any remaining operands this opcode may have codeAddr += sz; + // Clear any prefix flags that may have been set + prefixFlags = 0; + +OBSERVE_OPCODE: + + // Increment the number of observed instructions + opts.instrCount++; + // Note the opcode we just saw if (makeInlineObservations) { @@ -1401,6 +1504,8 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed typeIsNormed ? InlineObservation::CALLEE_OPCODE_NORMED : InlineObservation::CALLEE_OPCODE; compInlineResult->NoteInt(obs, opcode); } + + typeIsNormed = false; } if (codeAddr != codeEndp) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 54deec92534c97..12583c2d730858 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -3109,7 +3109,7 @@ unsigned Compiler::impInitBlockLineInfo() /*****************************************************************************/ -static inline bool impOpcodeIsCallOpcode(OPCODE opcode) +bool Compiler::impOpcodeIsCallOpcode(OPCODE opcode) { switch (opcode) { @@ -7849,21 +7849,6 @@ bool Compiler::impTailCallRetTypeCompatible(var_types callerRetTy return false; } -// For prefixFlags -enum -{ - PREFIX_TAILCALL_EXPLICIT = 0x00000001, // call has "tail" IL prefix - PREFIX_TAILCALL_IMPLICIT = - 0x00000010, // call is treated as having "tail" prefix even though there is no "tail" IL prefix - PREFIX_TAILCALL_STRESS = - 0x00000100, // call doesn't "tail" IL prefix but is treated as explicit because of tail call stress - PREFIX_TAILCALL = (PREFIX_TAILCALL_EXPLICIT | PREFIX_TAILCALL_IMPLICIT | PREFIX_TAILCALL_STRESS), - PREFIX_VOLATILE = 0x00001000, - PREFIX_UNALIGNED = 0x00010000, - PREFIX_CONSTRAINED = 0x00100000, - PREFIX_READONLY = 0x01000000 -}; - /******************************************************************************** * * Returns true if the current opcode and and the opcodes following it correspond @@ -10643,7 +10628,7 @@ void Compiler::impResetLeaveBlock(BasicBlock* block, unsigned jmpAddr) // Get the first non-prefix opcode. Used for verification of valid combinations // of prefixes and actual opcodes. -static OPCODE impGetNonPrefixOpcode(const BYTE* codeAddr, const BYTE* codeEndp) +OPCODE Compiler::impGetNonPrefixOpcode(const BYTE* codeAddr, const BYTE* codeEndp) { while (codeAddr < codeEndp) { @@ -10681,7 +10666,7 @@ static OPCODE impGetNonPrefixOpcode(const BYTE* codeAddr, const BYTE* codeEndp) /*****************************************************************************/ // Checks whether the opcode is a valid opcode for volatile. and unaligned. prefixes -static void impValidateMemoryAccessOpcode(const BYTE* codeAddr, const BYTE* codeEndp, bool volatilePrefix) +void Compiler::impValidateMemoryAccessOpcode(const BYTE* codeAddr, const BYTE* codeEndp, bool volatilePrefix) { OPCODE opcode = impGetNonPrefixOpcode(codeAddr, codeEndp); From 4d4cdc3a721fb2537d84a59e088e0b1e0a6e3536 Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Thu, 1 Apr 2021 20:58:10 -0700 Subject: [PATCH 2/4] Don't count prefix opcodes as instructions and handle NI_IsSupported_True/False as constants in fgFindJumpTargets --- src/coreclr/jit/fgbasic.cpp | 33 +++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index e35edb3aeb57ad..a8d5e2617f8e3a 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -976,6 +976,34 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed if ((methodFlags & (CORINFO_FLG_INTRINSIC | CORINFO_FLG_JIT_INTRINSIC)) != 0) { intrinsicCalls++; + + if ((methodFlags & CORINFO_FLG_INTRINSIC) != 0) + { + intrinsicID = info.compCompHnd->getIntrinsicID(methodHnd, &mustExpand); + } + + if ((methodFlags & CORINFO_FLG_JIT_INTRINSIC) != 0) + { + if (intrinsicID == CORINFO_INTRINSIC_Illegal) + { + ni = lookupNamedIntrinsic(methodHnd); + + switch (ni) + { + case NI_IsSupported_True: + case NI_IsSupported_False: + { + pushedStack.PushConstant(); + break; + } + + default: + { + break; + } + } + } + } } if ((OPCODE)getU1LittleEndian(codeAddr + sz) == CEE_RET) @@ -1403,6 +1431,7 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed // the list of other opcodes (for all platforms). FALLTHROUGH; + case CEE_MKREFANY: case CEE_RETHROW: if (makeInlineObservations) @@ -1492,11 +1521,11 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed // Clear any prefix flags that may have been set prefixFlags = 0; -OBSERVE_OPCODE: - // Increment the number of observed instructions opts.instrCount++; +OBSERVE_OPCODE: + // Note the opcode we just saw if (makeInlineObservations) { From c633127f847d3e22ed1273ed4c6bc3ac20e0db64 Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Fri, 2 Apr 2021 19:28:40 -0700 Subject: [PATCH 3/4] Update src/coreclr/jit/fgbasic.cpp --- src/coreclr/jit/fgbasic.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index a8d5e2617f8e3a..5403e7d0b1229e 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -850,7 +850,8 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed const bool makeInlineObservations = (compInlineResult != nullptr); const bool isInlining = compIsForInlining(); const bool isTier1 = opts.jitFlags->IsSet(JitFlags::JIT_FLAG_TIER1); - const bool resolveTokens = makeInlineObservations && isTier1; + const bool isPreJit = opts.jitFlags->IsSet(JitFlags::JIT_FLAG_PREJIT); + const bool resolveTokens = makeInlineObservations && (isTier1 || isPreJit); unsigned retBlocks = 0; unsigned intrinsicCalls = 0; int prefixFlags = 0; From 9796b1ac12ea496be5ae9b7371dc6ff7472b8190 Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Fri, 2 Apr 2021 20:19:42 -0700 Subject: [PATCH 4/4] Applying formatting patch --- src/coreclr/jit/fgbasic.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index 5403e7d0b1229e..f184a090b08714 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -1525,7 +1525,7 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed // Increment the number of observed instructions opts.instrCount++; -OBSERVE_OPCODE: + OBSERVE_OPCODE: // Note the opcode we just saw if (makeInlineObservations)