From 28a32683e9995cfeb29e3d9620af90605ff2c659 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Fri, 24 Jan 2025 00:13:23 -0500 Subject: [PATCH] Check BBF_PROF_WEIGHT flag is set --- src/coreclr/jit/compiler.cpp | 2 +- src/coreclr/jit/compiler.h | 10 ++++++---- src/coreclr/jit/fginline.cpp | 2 +- src/coreclr/jit/fgprofile.cpp | 30 +++++++++++++++++++++++++++--- 4 files changed, 35 insertions(+), 9 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index 185c7c323c6adb..33dda8c734ca1b 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -4571,7 +4571,7 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl // Note: the importer is sensitive to block weights, so this has // to happen before importation. // - activePhaseChecks |= PhaseChecks::CHECK_PROFILE; + activePhaseChecks |= PhaseChecks::CHECK_PROFILE | PhaseChecks::CHECK_PROFILE_FLAGS; DoPhase(this, PHASE_INCPROFILE, &Compiler::fgIncorporateProfileData); activePhaseChecks |= PhaseChecks::CHECK_FG_INIT_BLOCK; diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 956e05e9b71f00..904342a5438efb 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -1542,8 +1542,9 @@ enum class PhaseChecks : unsigned int CHECK_LOOPS = 1 << 4, // loop integrity/canonicalization CHECK_LIKELIHOODS = 1 << 5, // profile data likelihood integrity CHECK_PROFILE = 1 << 6, // profile data full integrity - CHECK_LINKED_LOCALS = 1 << 7, // check linked list of locals - CHECK_FG_INIT_BLOCK = 1 << 8, // flow graph has an init block + CHECK_PROFILE_FLAGS = 1 << 7, // blocks with profile-derived weights have BBF_PROF_WEIGHT flag set + CHECK_LINKED_LOCALS = 1 << 8, // check linked list of locals + CHECK_FG_INIT_BLOCK = 1 << 9, // flow graph has an init block }; inline constexpr PhaseChecks operator ~(PhaseChecks a) @@ -1608,8 +1609,9 @@ enum class ProfileChecks : unsigned int CHECK_HASLIKELIHOOD = 1 << 0, // check all FlowEdges for hasLikelihood CHECK_LIKELIHOODSUM = 1 << 1, // check block succesor likelihoods sum to 1 CHECK_LIKELY = 1 << 2, // fully check likelihood based weights - RAISE_ASSERT = 1 << 3, // assert on check failure - CHECK_ALL_BLOCKS = 1 << 4, // check blocks even if bbHasProfileWeight is false + CHECK_FLAGS = 1 << 3, // check blocks with profile-derived weights have BBF_PROF_WEIGHT flag set + RAISE_ASSERT = 1 << 4, // assert on check failure + CHECK_ALL_BLOCKS = 1 << 5, // check blocks even if bbHasProfileWeight is false }; inline constexpr ProfileChecks operator ~(ProfileChecks a) diff --git a/src/coreclr/jit/fginline.cpp b/src/coreclr/jit/fginline.cpp index ee75f8b13cd774..47db8090dabdf4 100644 --- a/src/coreclr/jit/fginline.cpp +++ b/src/coreclr/jit/fginline.cpp @@ -1514,7 +1514,7 @@ void Compiler::fgInsertInlineeBlocks(InlineInfo* pInlineInfo) noway_assert(!block->hasTryIndex()); noway_assert(!block->hasHndIndex()); block->copyEHRegion(iciBlock); - block->CopyFlags(iciBlock, BBF_BACKWARD_JUMP); + block->CopyFlags(iciBlock, BBF_BACKWARD_JUMP | BBF_PROF_WEIGHT); // Update block nums appropriately // diff --git a/src/coreclr/jit/fgprofile.cpp b/src/coreclr/jit/fgprofile.cpp index 9cf332bf16c995..2a21e61f0d3a09 100644 --- a/src/coreclr/jit/fgprofile.cpp +++ b/src/coreclr/jit/fgprofile.cpp @@ -4571,6 +4571,12 @@ void Compiler::fgDebugCheckProfile(PhaseChecks checks) else if (hasFlag(checks, PhaseChecks::CHECK_PROFILE)) { ProfileChecks profileChecks = ProfileChecks::CHECK_LIKELY | ProfileChecks::RAISE_ASSERT; + + if (hasFlag(checks, PhaseChecks::CHECK_PROFILE_FLAGS)) + { + profileChecks |= ProfileChecks::CHECK_FLAGS; + } + fgDebugCheckProfileWeights(profileChecks); } else if (hasFlag(checks, PhaseChecks::CHECK_LIKELIHOODS)) @@ -4611,6 +4617,7 @@ bool Compiler::fgDebugCheckProfileWeights(ProfileChecks checks) const bool verifyLikelyWeights = hasFlag(checks, ProfileChecks::CHECK_LIKELY); const bool verifyHasLikelihood = hasFlag(checks, ProfileChecks::CHECK_HASLIKELIHOOD); const bool verifyLikelihoodSum = hasFlag(checks, ProfileChecks::CHECK_LIKELIHOODSUM); + const bool checkProfileFlag = hasFlag(checks, ProfileChecks::CHECK_FLAGS) && fgIsUsingProfileWeights(); const bool assertOnFailure = hasFlag(checks, ProfileChecks::RAISE_ASSERT) && fgPgoConsistent; const bool checkAllBlocks = hasFlag(checks, ProfileChecks::CHECK_ALL_BLOCKS); @@ -4633,6 +4640,7 @@ bool Compiler::fgDebugCheckProfileWeights(ProfileChecks checks) unsigned problemBlocks = 0; unsigned unprofiledBlocks = 0; unsigned profiledBlocks = 0; + unsigned unflaggedBlocks = 0; bool entryProfiled = false; bool exitProfiled = false; bool hasTry = false; @@ -4643,10 +4651,20 @@ bool Compiler::fgDebugCheckProfileWeights(ProfileChecks checks) // for (BasicBlock* const block : Blocks()) { - if (!block->hasProfileWeight() && !checkAllBlocks) + if (!block->hasProfileWeight()) { - unprofiledBlocks++; - continue; + if (checkProfileFlag && (block->bbPreds != nullptr)) + { + // Block has flow into it that isn't marked as profile-derived. + // + unflaggedBlocks++; + } + + if (!checkAllBlocks) + { + unprofiledBlocks++; + continue; + } } // There is some profile data to check. @@ -4831,6 +4849,12 @@ bool Compiler::fgDebugCheckProfileWeights(ProfileChecks checks) } } + if (unflaggedBlocks > 0) + { + JITDUMP("%d blocks are missing BBF_PROF_WEIGHT flag.\n", unflaggedBlocks); + assert(!"Missing BBF_PROF_WEIGHT flag"); + } + return (problemBlocks == 0); }