From 30295732f46165794fd409b48ae37728dac43fa9 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Wed, 6 Nov 2024 16:52:19 -0500 Subject: [PATCH 1/6] Move checks after fgAddInternal --- src/coreclr/jit/compiler.cpp | 10 +++++----- src/coreclr/jit/flowgraph.cpp | 7 ------- 2 files changed, 5 insertions(+), 12 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index 3d2f568301bfa..b09b9f272fdad 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -4617,11 +4617,6 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl // Record "start" values for post-inlining cycles and elapsed time. RecordStateAtEndOfInlining(); - // Drop back to just checking profile likelihoods. - // - activePhaseChecks &= ~PhaseChecks::CHECK_PROFILE; - activePhaseChecks |= PhaseChecks::CHECK_LIKELIHOODS; - // Transform each GT_ALLOCOBJ node into either an allocation helper call or // local variable allocation on the stack. ObjectAllocator objectAllocator(this); // PHASE_ALLOCATE_OBJECTS @@ -4643,6 +4638,11 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl DoPhase(this, PHASE_SWIFT_ERROR_RET, &Compiler::fgAddSwiftErrorReturns); #endif // SWIFT_SUPPORT + // Drop back to just checking profile likelihoods. + // + activePhaseChecks &= ~PhaseChecks::CHECK_PROFILE; + activePhaseChecks |= PhaseChecks::CHECK_LIKELIHOODS; + // Remove empty try regions // DoPhase(this, PHASE_EMPTY_TRY, &Compiler::fgRemoveEmptyTry); diff --git a/src/coreclr/jit/flowgraph.cpp b/src/coreclr/jit/flowgraph.cpp index d770ddf2630ed..de2b583d03357 100644 --- a/src/coreclr/jit/flowgraph.cpp +++ b/src/coreclr/jit/flowgraph.cpp @@ -1401,15 +1401,8 @@ void Compiler::fgAddSyncMethodEnterExit() // Create a block for the start of the try region, where the monitor enter call // will go. BasicBlock* const tryBegBB = fgSplitBlockAtEnd(fgFirstBB); - BasicBlock* const tryNextBB = tryBegBB->Next(); BasicBlock* const tryLastBB = fgLastBB; - // If we have profile data the new block will inherit the next block's weight - if (tryNextBB->hasProfileWeight()) - { - tryBegBB->inheritWeight(tryNextBB); - } - // Create a block for the fault. // It gets an artificial ref count. From 5f941dd506b2116f1cd208cb467ea8cf868c1391 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Mon, 11 Nov 2024 10:41:51 -0500 Subject: [PATCH 2/6] wip --- src/coreclr/jit/compiler.cpp | 10 +++++----- src/coreclr/jit/fgehopt.cpp | 7 +++++++ 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index b09b9f272fdad..381cd24297977 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -4638,11 +4638,6 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl DoPhase(this, PHASE_SWIFT_ERROR_RET, &Compiler::fgAddSwiftErrorReturns); #endif // SWIFT_SUPPORT - // Drop back to just checking profile likelihoods. - // - activePhaseChecks &= ~PhaseChecks::CHECK_PROFILE; - activePhaseChecks |= PhaseChecks::CHECK_LIKELIHOODS; - // Remove empty try regions // DoPhase(this, PHASE_EMPTY_TRY, &Compiler::fgRemoveEmptyTry); @@ -4659,6 +4654,11 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl // DoPhase(this, PHASE_CLONE_FINALLY, &Compiler::fgCloneFinally); + // Drop back to just checking profile likelihoods. + // + activePhaseChecks &= ~PhaseChecks::CHECK_PROFILE; + activePhaseChecks |= PhaseChecks::CHECK_LIKELIHOODS; + // Do some flow-related optimizations // if (opts.OptimizationEnabled()) diff --git a/src/coreclr/jit/fgehopt.cpp b/src/coreclr/jit/fgehopt.cpp index 1ea7bc6557191..9b1d50b47fea2 100644 --- a/src/coreclr/jit/fgehopt.cpp +++ b/src/coreclr/jit/fgehopt.cpp @@ -1333,6 +1333,13 @@ PhaseStatus Compiler::fgCloneFinally() } } + // normalCallFinallyReturn->bbWeight = BB_ZERO_WEIGHT; + // for (BasicBlock* const predBlock : normalCallFinallyReturn->PredBlocks()) + // { + // assert(predBlock->NumSucc() == 1); + // normalCallFinallyReturn->bbWeight += predBlock->bbWeight; + // } + // Done! JITDUMP("\nDone with EH#%u\n\n", XTnum); cloneCount++; From cbad712e106f804ed5740ed707efe2a1f510eaa3 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Wed, 13 Nov 2024 12:39:46 -0500 Subject: [PATCH 3/6] Move checks after fgCloneFinally --- src/coreclr/jit/fgehopt.cpp | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/fgehopt.cpp b/src/coreclr/jit/fgehopt.cpp index 9b1d50b47fea2..55bc5f722a994 100644 --- a/src/coreclr/jit/fgehopt.cpp +++ b/src/coreclr/jit/fgehopt.cpp @@ -1105,7 +1105,26 @@ PhaseStatus Compiler::fgCloneFinally() BasicBlock* insertAfter = nullptr; BlockToBlockMap blockMap(getAllocator()); unsigned cloneBBCount = 0; - weight_t const originalWeight = firstBlock->hasProfileWeight() ? firstBlock->bbWeight : BB_ZERO_WEIGHT; + weight_t originalWeight; + + // When distributing weight between the original and cloned regions, + // ensure only weight from region entries is considered. + // Flow from loop backedges within the region should not influence the weight distribution ratio. + if (firstBlock->hasProfileWeight()) + { + originalWeight = firstBlock->bbWeight; + for (BasicBlock* const predBlock : firstBlock->PredBlocks()) + { + if (!predBlock->KindIs(BBJ_CALLFINALLY)) + { + originalWeight = max(0.0, originalWeight - predBlock->bbWeight); + } + } + } + else + { + originalWeight = BB_ZERO_WEIGHT; + } for (BasicBlock* block = firstBlock; block != nextBlock; block = block->Next()) { From 52d8902744332e67ec7b23e3907502b2e9f27bdd Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Wed, 13 Nov 2024 13:27:54 -0500 Subject: [PATCH 4/6] Remove commented out code --- src/coreclr/jit/fgehopt.cpp | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/coreclr/jit/fgehopt.cpp b/src/coreclr/jit/fgehopt.cpp index 55bc5f722a994..64167ff251cf8 100644 --- a/src/coreclr/jit/fgehopt.cpp +++ b/src/coreclr/jit/fgehopt.cpp @@ -1352,13 +1352,6 @@ PhaseStatus Compiler::fgCloneFinally() } } - // normalCallFinallyReturn->bbWeight = BB_ZERO_WEIGHT; - // for (BasicBlock* const predBlock : normalCallFinallyReturn->PredBlocks()) - // { - // assert(predBlock->NumSucc() == 1); - // normalCallFinallyReturn->bbWeight += predBlock->bbWeight; - // } - // Done! JITDUMP("\nDone with EH#%u\n\n", XTnum); cloneCount++; From 218beba3ea5ba65a39a88ba477caa51c33ce08ee Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Wed, 13 Nov 2024 14:03:33 -0500 Subject: [PATCH 5/6] Style --- src/coreclr/jit/fgehopt.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/fgehopt.cpp b/src/coreclr/jit/fgehopt.cpp index 64167ff251cf8..b0fe8c0a12187 100644 --- a/src/coreclr/jit/fgehopt.cpp +++ b/src/coreclr/jit/fgehopt.cpp @@ -1104,8 +1104,8 @@ PhaseStatus Compiler::fgCloneFinally() const unsigned finallyTryIndex = firstBlock->bbTryIndex; BasicBlock* insertAfter = nullptr; BlockToBlockMap blockMap(getAllocator()); - unsigned cloneBBCount = 0; - weight_t originalWeight; + unsigned cloneBBCount = 0; + weight_t originalWeight; // When distributing weight between the original and cloned regions, // ensure only weight from region entries is considered. From 8deddc73a541facb06cd05a9b36ac7b52553d277 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Wed, 13 Nov 2024 16:35:22 -0500 Subject: [PATCH 6/6] Mark profile inconsistent when removing empty static alloc --- src/coreclr/jit/objectalloc.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/coreclr/jit/objectalloc.cpp b/src/coreclr/jit/objectalloc.cpp index ed68aa9323197..e8c85d7ca48bc 100644 --- a/src/coreclr/jit/objectalloc.cpp +++ b/src/coreclr/jit/objectalloc.cpp @@ -673,6 +673,9 @@ unsigned int ObjectAllocator::MorphAllocObjNodeIntoStackAlloc( if (predBlock->hasProfileWeight()) { block->setBBProfileWeight(predBlock->bbWeight); + JITDUMP("Profile weight into " FMT_BB " needs to be propagated to successors. Profile %s inconsistent.\n", + block->bbNum, comp->fgPgoConsistent ? "is now" : "was already"); + comp->fgPgoConsistent = false; } // Just lop off the JTRUE, the rest can clean up later