From 1ba22eb7994231cc09fcdbdce88db61a4b802737 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 17 Mar 2021 10:58:08 -0700 Subject: [PATCH] Allow jit to disregard PGO; PGO changes for SPMI and MCS (#49551) * COMPlus_JitDisablePGO will now block the jit from using profile data. Useful for investigations and (someday) bug triage. Also disable GDV if we're disabling PGO * `-base_jit_option` and `-diff_jit_option` can be used to pass options to superpmy.py asmdiffs. * Fix SPMI file name computation to better handle cases where the directory is a relative path. * Enhance `mcs -jitflags` output to describe how many method contexts have PGO data, and which kinds of data they hold. Useful for validating that an SPMI collection done via dynamic PGO has actually collected an interesting set of jit requests. --- .../ToolBox/superpmi/mcs/verbjitflags.cpp | 20 ++++ .../superpmi-shared/methodcontext.cpp | 38 ++++++++ .../superpmi/superpmi-shared/methodcontext.h | 17 ++++ .../superpmi-shared/spmidumphelper.cpp | 6 ++ .../superpmi/superpmi-shared/spmiutil.cpp | 92 +++++++++++-------- src/coreclr/jit/codegencommon.cpp | 4 +- src/coreclr/jit/compiler.cpp | 33 ++++--- src/coreclr/jit/compiler.h | 2 +- src/coreclr/jit/importer.cpp | 9 ++ src/coreclr/jit/jitconfigvalues.h | 3 + src/coreclr/scripts/superpmi.py | 36 +++++++- 11 files changed, 207 insertions(+), 53 deletions(-) diff --git a/src/coreclr/ToolBox/superpmi/mcs/verbjitflags.cpp b/src/coreclr/ToolBox/superpmi/mcs/verbjitflags.cpp index f5f9d7786a745..a40707846887d 100644 --- a/src/coreclr/ToolBox/superpmi/mcs/verbjitflags.cpp +++ b/src/coreclr/ToolBox/superpmi/mcs/verbjitflags.cpp @@ -24,6 +24,26 @@ int verbJitFlags::DoWork(const char* nameOfInput) mc->repGetJitFlags(&corJitFlags, sizeof(corJitFlags)); unsigned long long rawFlags = corJitFlags.GetFlagsRaw(); + // We co-opt unused flag bits to note if there's pgo data, + // and if so, what kind + // + bool hasEdgeProfile = false; + bool hasClassProfile = false; + if (mc->hasPgoData(hasEdgeProfile, hasClassProfile)) + { + rawFlags |= 1ULL << (EXTRA_JIT_FLAGS::HAS_PGO); + + if (hasEdgeProfile) + { + rawFlags |= 1ULL << (EXTRA_JIT_FLAGS::HAS_EDGE_PROFILE); + } + + if (hasClassProfile) + { + rawFlags |= 1ULL << (EXTRA_JIT_FLAGS::HAS_CLASS_PROFILE); + } + } + int index = flagMap.GetIndex(rawFlags); if (index == -1) { diff --git a/src/coreclr/ToolBox/superpmi/superpmi-shared/methodcontext.cpp b/src/coreclr/ToolBox/superpmi/superpmi-shared/methodcontext.cpp index 0a7da7e4b56c2..51faa23a60702 100644 --- a/src/coreclr/ToolBox/superpmi/superpmi-shared/methodcontext.cpp +++ b/src/coreclr/ToolBox/superpmi/superpmi-shared/methodcontext.cpp @@ -6765,6 +6765,44 @@ int MethodContext::dumpMD5HashToBuffer(BYTE* pBuffer, int bufLen, char* hash, in return m_hash.HashBuffer(pBuffer, bufLen, hash, hashLen); } +bool MethodContext::hasPgoData(bool& hasEdgeProfile, bool& hasClassProfile) +{ + hasEdgeProfile = false; + hasClassProfile = false; + + // Obtain the Method Info structure for this method + CORINFO_METHOD_INFO info; + unsigned flags = 0; + repCompileMethod(&info, &flags); + + if ((GetPgoInstrumentationResults != nullptr) && + (GetPgoInstrumentationResults->GetIndex(CastHandle(info.ftn)) != -1)) + { + ICorJitInfo::PgoInstrumentationSchema* schema = nullptr; + UINT32 schemaCount = 0; + BYTE* schemaData = nullptr; + HRESULT pgoHR = repGetPgoInstrumentationResults(info.ftn, &schema, &schemaCount, &schemaData); + + if (SUCCEEDED(pgoHR)) + { + for (UINT32 i = 0; i < schemaCount; i++) + { + hasEdgeProfile |= (schema[i].InstrumentationKind == ICorJitInfo::PgoInstrumentationKind::EdgeIntCount); + hasClassProfile |= (schema[i].InstrumentationKind == ICorJitInfo::PgoInstrumentationKind::TypeHandleHistogramCount); + + if (hasEdgeProfile && hasClassProfile) + { + break; + } + } + + return true; + } + } + + return false; +} + MethodContext::Environment MethodContext::cloneEnvironment() { MethodContext::Environment env; diff --git a/src/coreclr/ToolBox/superpmi/superpmi-shared/methodcontext.h b/src/coreclr/ToolBox/superpmi/superpmi-shared/methodcontext.h index 4851d36966953..d71f8d35f3b33 100644 --- a/src/coreclr/ToolBox/superpmi/superpmi-shared/methodcontext.h +++ b/src/coreclr/ToolBox/superpmi/superpmi-shared/methodcontext.h @@ -43,6 +43,21 @@ const char* toString(CorInfoType cit); #define METHOD_IDENTITY_INFO_SIZE 0x10000 // We assume that the METHOD_IDENTITY_INFO_SIZE will not exceed 64KB +// Special "jit flags" for noting some method context features + +enum EXTRA_JIT_FLAGS +{ + HAS_PGO = 63, + HAS_EDGE_PROFILE = 62, + HAS_CLASS_PROFILE = 61 +}; + +// Asserts to catch changes in corjit flags definitions. + +static_assert((int)EXTRA_JIT_FLAGS::HAS_PGO == (int)CORJIT_FLAGS::CorJitFlag::CORJIT_FLAG_UNUSED36, "Jit Flags Mismatch"); +static_assert((int)EXTRA_JIT_FLAGS::HAS_EDGE_PROFILE == (int)CORJIT_FLAGS::CorJitFlag::CORJIT_FLAG_UNUSED35, "Jit Flags Mismatch"); +static_assert((int)EXTRA_JIT_FLAGS::HAS_CLASS_PROFILE == (int)CORJIT_FLAGS::CorJitFlag::CORJIT_FLAG_UNUSED34, "Jit Flags Mismatch"); + class MethodContext { public: @@ -82,6 +97,8 @@ class MethodContext int dumpMethodIdentityInfoToBuffer(char* buff, int len, bool ignoreMethodName = false, CORINFO_METHOD_INFO* optInfo = nullptr, unsigned optFlags = 0); int dumpMethodMD5HashToBuffer(char* buff, int len, bool ignoreMethodName = false, CORINFO_METHOD_INFO* optInfo = nullptr, unsigned optFlags = 0); + bool hasPgoData(bool& hasEdgeProfile, bool& hasClassProfile); + void recGlobalContext(const MethodContext& other); void dmpEnvironment(DWORD key, const Agnostic_Environment& value); diff --git a/src/coreclr/ToolBox/superpmi/superpmi-shared/spmidumphelper.cpp b/src/coreclr/ToolBox/superpmi/superpmi-shared/spmidumphelper.cpp index 3530d369507ec..da63826e750b6 100644 --- a/src/coreclr/ToolBox/superpmi/superpmi-shared/spmidumphelper.cpp +++ b/src/coreclr/ToolBox/superpmi/superpmi-shared/spmidumphelper.cpp @@ -279,6 +279,12 @@ std::string SpmiDumpHelper::DumpJitFlags(unsigned long long flags) AddFlag(NO_INLINING); + // "Extra jit flag" support + // + AddFlagNumeric(HAS_PGO, EXTRA_JIT_FLAGS::HAS_PGO); + AddFlagNumeric(HAS_EDGE_PROFILE, EXTRA_JIT_FLAGS::HAS_EDGE_PROFILE); + AddFlagNumeric(HAS_CLASS_PROFILE, EXTRA_JIT_FLAGS::HAS_CLASS_PROFILE); + #undef AddFlag #undef AddFlagNumeric diff --git a/src/coreclr/ToolBox/superpmi/superpmi-shared/spmiutil.cpp b/src/coreclr/ToolBox/superpmi/superpmi-shared/spmiutil.cpp index 0d52ae35ed1ab..efbe6f13e1cd1 100644 --- a/src/coreclr/ToolBox/superpmi/superpmi-shared/spmiutil.cpp +++ b/src/coreclr/ToolBox/superpmi/superpmi-shared/spmiutil.cpp @@ -156,58 +156,76 @@ void ReplaceIllegalCharacters(WCHAR* fileName) // All lengths in this function exclude the terminal NULL. WCHAR* GetResultFileName(const WCHAR* folderPath, const WCHAR* fileName, const WCHAR* extension) { - const size_t folderPathLength = wcslen(folderPath); - const size_t fileNameLength = wcslen(fileName); const size_t extensionLength = wcslen(extension); - const size_t maxPathLength = MAX_PATH - 50; // subtract 50 because excel doesn't like paths longer then 230. + const size_t fileNameLength = wcslen(fileName); const size_t randomStringLength = 8; + const size_t maxPathLength = MAX_PATH - 50; + bool appendRandomString = false; - size_t fullPathLength = folderPathLength + 1 + extensionLength; - bool appendRandomString = false; + // See how long the folder part is, and start building the file path with the folder part. + // + WCHAR* fullPath = new WCHAR[MAX_PATH]; + fullPath[0] = W('\0'); + const size_t folderPathLength = GetFullPathNameW(folderPath, MAX_PATH, (LPWSTR)fullPath, NULL); - if (fileNameLength > 0) - { - fullPathLength += fileNameLength; - } - else + if (folderPathLength == 0) { - fullPathLength += randomStringLength; - appendRandomString = true; + LogError("GetResultFileName - can't resolve folder path '%ws'", folderPath); + return nullptr; } - size_t charsToDelete = 0; + // Account for the folder, directory separator and extension. + // + size_t fullPathLength = folderPathLength + 1 + extensionLength; - if (fullPathLength > maxPathLength) + // If we won't have room for a minimal file name part, bail. + // + if ((fullPathLength + randomStringLength) > maxPathLength) { - // The path name is too long; creating the file will fail. This can happen because we use the command line, - // which for ngen includes lots of environment variables, for example. - // Shorten the file name and add a random string to the end to avoid collisions. + LogError("GetResultFileName - folder path '%ws' length + minimal file name exceeds limit %d", fullPath, maxPathLength); + return nullptr; + } - charsToDelete = fullPathLength - maxPathLength + randomStringLength; + // Now figure out the file name part. + // + const size_t maxFileNameLength = maxPathLength - fullPathLength; + size_t usableFileNameLength = 0; - if (fileNameLength >= charsToDelete) - { - appendRandomString = true; - fullPathLength = maxPathLength; - } - else - { - LogError("GetResultFileName - path to the output file is too long '%ws\\%ws.%ws(%d)'", folderPath, fileName, extension, fullPathLength); - return nullptr; - } + if (fileNameLength == 0) + { + // No file name provided. Use random string. + // + fullPathLength += randomStringLength; + appendRandomString = true; + } + else if (fileNameLength < maxFileNameLength) + { + // Reasonable length file name, use as is. + // + usableFileNameLength = fileNameLength; + fullPathLength += fileNameLength; + appendRandomString = false; + } + else + { + // Overly long file name, truncate and add random string. + // + usableFileNameLength = maxFileNameLength - randomStringLength; + fullPathLength += maxFileNameLength; + appendRandomString = true; } - WCHAR* fullPath = new WCHAR[fullPathLength + 1]; - fullPath[0] = W('\0'); - wcsncat_s(fullPath, fullPathLength + 1, folderPath, folderPathLength); + // Append the file name part + // wcsncat_s(fullPath, fullPathLength + 1, DIRECTORY_SEPARATOR_STR_W, 1); + wcsncat_s(fullPath, fullPathLength + 1, fileName, usableFileNameLength); - if (fileNameLength > charsToDelete) - { - wcsncat_s(fullPath, fullPathLength + 1, fileName, fileNameLength - charsToDelete); - ReplaceIllegalCharacters(fullPath + folderPathLength + 1); - } + // Clean up anything in the file part that can't be in a file name. + // + ReplaceIllegalCharacters(fullPath + folderPathLength + 1); + // Append random string, if we're using it. + // if (appendRandomString) { unsigned randomNumber = 0; @@ -223,6 +241,8 @@ WCHAR* GetResultFileName(const WCHAR* folderPath, const WCHAR* fileName, const W wcsncat_s(fullPath, fullPathLength + 1, randomString, randomStringLength); } + // Append extension + // wcsncat_s(fullPath, fullPathLength + 1, extension, extensionLength); return fullPath; diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp index da78cb3c944d6..bb49d1ce0f1c6 100644 --- a/src/coreclr/jit/codegencommon.cpp +++ b/src/coreclr/jit/codegencommon.cpp @@ -2207,9 +2207,9 @@ void CodeGen::genGenerateMachineCode() compiler->fgHaveValidEdgeWeights ? "valid" : "invalid", compiler->fgCalledCount); } - if (compiler->fgProfileData_ILSizeMismatch) + if (compiler->fgPgoFailReason != nullptr) { - printf("; discarded IBC profile data due to mismatch in ILSize\n"); + printf("; %s\n", compiler->fgPgoFailReason); } if (compiler->opts.jitFlags->IsSet(JitFlags::JIT_FLAG_ALT_JIT)) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index e1be2ce02a039..3570d37fc3386 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -2830,11 +2830,12 @@ void Compiler::compInitOptions(JitFlags* jitFlags) // Profile data // - fgPgoSchema = nullptr; - fgPgoData = nullptr; - fgPgoSchemaCount = 0; - fgPgoQueryResult = E_FAIL; - fgProfileData_ILSizeMismatch = false; + fgPgoSchema = nullptr; + fgPgoData = nullptr; + fgPgoSchemaCount = 0; + fgPgoQueryResult = E_FAIL; + fgPgoFailReason = nullptr; + if (jitFlags->IsSet(JitFlags::JIT_FLAG_BBOPT)) { fgPgoQueryResult = info.compCompHnd->getPgoInstrumentationResults(info.compMethodHnd, &fgPgoSchema, @@ -2846,12 +2847,22 @@ void Compiler::compInitOptions(JitFlags* jitFlags) // // We will discard the IBC data in this case // - if (FAILED(fgPgoQueryResult) && (fgPgoSchema != nullptr)) + if (FAILED(fgPgoQueryResult)) + { + fgPgoFailReason = (fgPgoSchema != nullptr) ? "No matching PGO data" : "No PGO data"; + fgPgoData = nullptr; + fgPgoSchema = nullptr; + } + // Optionally, discard the profile data. + // + else if (JitConfig.JitDisablePGO() != 0) { - fgProfileData_ILSizeMismatch = true; - fgPgoData = nullptr; - fgPgoSchema = nullptr; + fgPgoFailReason = "PGO data available, but JitDisablePGO != 0"; + fgPgoQueryResult = E_FAIL; + fgPgoData = nullptr; + fgPgoSchema = nullptr; } + #ifdef DEBUG // A successful result implies a non-NULL fgPgoSchema // @@ -3364,9 +3375,9 @@ void Compiler::compInitOptions(JitFlags* jitFlags) printf("OPTIONS: optimized using profile data\n"); } - if (fgProfileData_ILSizeMismatch) + if (fgPgoFailReason != nullptr) { - printf("OPTIONS: discarded IBC profile data due to mismatch in ILSize\n"); + printf("OPTIONS: %s\n", fgPgoFailReason); } if (jitFlags->IsSet(JitFlags::JIT_FLAG_PREJIT)) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 80cded4dd8ab9..f48697168c596 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5636,7 +5636,7 @@ class Compiler void fgIncorporateEdgeCounts(); public: - bool fgProfileData_ILSizeMismatch; + const char* fgPgoFailReason; ICorJitInfo::PgoInstrumentationSchema* fgPgoSchema; BYTE* fgPgoData; UINT32 fgPgoSchemaCount; diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 026e333d2ef64..390597df7ebc3 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -21504,6 +21504,15 @@ void Compiler::considerGuardedDevirtualization( JITDUMP("Considering guarded devirtualization\n"); + // We currently only get likely class guesses when there is PGO data. So if we've disabled + // PGO, just bail out. + + if (JitConfig.JitDisablePGO() != 0) + { + JITDUMP("Not guessing for class; pgo disabled\n"); + return; + } + // See if there's a likely guess for the class. // const unsigned likelihoodThreshold = isInterface ? 25 : 30; diff --git a/src/coreclr/jit/jitconfigvalues.h b/src/coreclr/jit/jitconfigvalues.h index c892abe0a4b9c..d283522b2cdd4 100644 --- a/src/coreclr/jit/jitconfigvalues.h +++ b/src/coreclr/jit/jitconfigvalues.h @@ -462,6 +462,9 @@ CONFIG_INTEGER(JitMinimalPrejitProfiling, W("JitMinimalPrejitProfiling"), 0) CONFIG_INTEGER(JitClassProfiling, W("JitClassProfiling"), 1) CONFIG_INTEGER(JitEdgeProfiling, W("JitEdgeProfiling"), 1) +// Profile consumption options +CONFIG_INTEGER(JitDisablePGO, W("JitDisablePGO"), 0) // Ignore pgo data + // Control when Virtual Calls are expanded CONFIG_INTEGER(JitExpandCallsEarly, W("JitExpandCallsEarly"), 1) // Expand Call targets early (in the global morph // phase) diff --git a/src/coreclr/scripts/superpmi.py b/src/coreclr/scripts/superpmi.py index 5146f91c154ef..b2036a98ce325 100755 --- a/src/coreclr/scripts/superpmi.py +++ b/src/coreclr/scripts/superpmi.py @@ -294,6 +294,8 @@ asm_diff_parser.add_argument("--diff_jit_dump", action="store_true", help="Generate JitDump output for diffs. Default: only generate asm, not JitDump.") asm_diff_parser.add_argument("-temp_dir", help="Specify a temporary directory used for a previous ASM diffs run (for which --skip_cleanup was used) to view the results. The replay command is skipped.") asm_diff_parser.add_argument("--gcinfo", action="store_true", help="Include GC info in disassembly (sets COMPlus_JitGCDump/COMPlus_NgenGCDump; requires instructions to be prefixed by offsets).") +asm_diff_parser.add_argument("-base_jit_option", action="append", help="Option to pass to the baselne JIT. Format is key=value, where key is the option name without leading COMPlus_...") +asm_diff_parser.add_argument("-diff_jit_option", action="append", help="Option to pass to the diff JIT. Format is key=value, where key is the option name without leading COMPlus_...") # subparser for upload upload_parser = subparsers.add_parser("upload", description=upload_description, parents=[core_root_parser, target_parser]) @@ -1721,6 +1723,22 @@ def replay_with_asm_diffs(self): altjit_asm_diffs_flags = target_flags altjit_replay_flags = target_flags + if self.coreclr_args.jitoption: + logging.warning("Ignoring -jitoption; use -base_jit_option or -diff_jit_option instead"); + + base_option_flags = [] + if self.coreclr_args.base_jit_option: + for o in self.coreclr_args.base_jit_option: + base_option_flags += "-jitoption", o + base_option_flags_for_diff_artifact = base_option_flags + + diff_option_flags = [] + diff_option_flags_for_diff_artifact = [] + if self.coreclr_args.diff_jit_option: + for o in self.coreclr_args.diff_jit_option: + diff_option_flags += "-jit2option", o + diff_option_flags_for_diff_artifact += "-jitoption", o + if self.coreclr_args.altjit: altjit_asm_diffs_flags += [ "-jitoption", "force", "AltJit=*", @@ -1770,6 +1788,8 @@ def replay_with_asm_diffs(self): "-r", os.path.join(temp_location, "repro") # Repro name, create .mc repro files ] flags += altjit_asm_diffs_flags + flags += base_option_flags + flags += diff_option_flags if not self.coreclr_args.sequential: flags += [ "-p" ] @@ -1858,7 +1878,7 @@ async def create_replay_artifacts(print_prefix, item, self, mch_file, env_vars, # as the LoadLibrary path will be relative to the current directory. with ChangeDir(self.coreclr_args.core_root): - async def create_one_artifact(jit_path: str, location: str) -> str: + async def create_one_artifact(jit_path: str, location: str, flags: list[str]) -> str: command = [self.superpmi_path] + flags + [jit_path, mch_file] item_path = os.path.join(location, "{}{}".format(item, extension)) with open(item_path, 'w') as file_handle: @@ -1871,8 +1891,8 @@ async def create_one_artifact(jit_path: str, location: str) -> str: return generated_txt # Generate diff and base JIT dumps - base_txt = await create_one_artifact(self.base_jit_path, base_location) - diff_txt = await create_one_artifact(self.diff_jit_path, diff_location) + base_txt = await create_one_artifact(self.base_jit_path, base_location, flags + base_option_flags_for_diff_artifact) + diff_txt = await create_one_artifact(self.diff_jit_path, diff_location, flags + diff_option_flags_for_diff_artifact) if base_txt != diff_txt: jit_differences_queue.put_nowait(item) @@ -3410,6 +3430,16 @@ def verify_replay_common_args(): lambda unused: True, "Unable to set diff_jit_dump.") + coreclr_args.verify(args, + "base_jit_option", + lambda unused: True, + "Unable to set base_jit_option.") + + coreclr_args.verify(args, + "diff_jit_option", + lambda unused: True, + "Unable to set diff_jit_option.") + process_base_jit_path_arg(coreclr_args) jit_in_product_location = False