From 1adb9bf88b6baa1024c0f67716192c563dccf195 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 13 Jan 2025 13:38:31 +0100 Subject: [PATCH 1/3] JIT: Switch `DataFlow::ForwardAnalysis` to be RPO based `DataFlow::ForwardAnalysis` used a worklist based algorithm where it kept visiting successor blocks as long as information was being propagated until closure. However, this successor-based visit was using regular successors only, which does not account for EH second-pass flow. This was not generally a correctness problem as the only users of this class are CSE and assertion prop, and neither of them have a correctness requirement of propagating facts along filter -> finally/fault edges, but the imprecision in the modelling could result in disagreements in reachability with the DFS visit. Concretely we could end up with CSE's availability analysis finding that a nested finally/fault block was unreachable (because its corresponding try-entry was unreachable), while the DFS visit considered it reachable due to a spurious filter predecessor. This mismatch meant that the handler was never visited by the CSE dataflow which would result in all CSEs in some of the handler blocks being marked as available. We would then end up creating a use in one of these blocks, which SSA updating would not be able to find a corresponding def for. Fix the situation by switching `DataFlow::ForwardAnalysis` to compute a DFS tree and computing the closure based on its RPO. Fix #111345 --- src/coreclr/jit/dataflow.h | 70 +++++++++++--------------------------- 1 file changed, 20 insertions(+), 50 deletions(-) diff --git a/src/coreclr/jit/dataflow.h b/src/coreclr/jit/dataflow.h index 6f27c6a998d771..ec11abc1c08d61 100644 --- a/src/coreclr/jit/dataflow.h +++ b/src/coreclr/jit/dataflow.h @@ -42,64 +42,34 @@ class DataFlow template void DataFlow::ForwardAnalysis(TCallback& callback) { - jitstd::list worklist(jitstd::allocator(m_pCompiler->getAllocator())); - - worklist.insert(worklist.begin(), m_pCompiler->fgFirstBB); - while (!worklist.empty()) + if (m_pCompiler->m_dfsTree == nullptr) { - BasicBlock* block = *(worklist.begin()); - worklist.erase(worklist.begin()); + m_pCompiler->m_dfsTree = m_pCompiler->fgComputeDfs(); + } - callback.StartMerge(block); - if (m_pCompiler->bbIsHandlerBeg(block)) - { - EHblkDsc* ehDsc = m_pCompiler->ehGetBlockHndDsc(block); - callback.MergeHandler(block, ehDsc->ebdTryBeg, ehDsc->ebdTryLast); - } - else + bool changed; + do + { + changed = false; + for (unsigned i = m_pCompiler->m_dfsTree->GetPostOrderCount(); i > 0; i--) { - for (FlowEdge* pred : block->PredEdges()) + BasicBlock* block = m_pCompiler->m_dfsTree->GetPostOrder(i - 1); + + callback.StartMerge(block); + if (m_pCompiler->bbIsHandlerBeg(block)) { - callback.Merge(block, pred->getSourceBlock(), pred->getDupCount()); + EHblkDsc* ehDsc = m_pCompiler->ehGetBlockHndDsc(block); + callback.MergeHandler(block, ehDsc->ebdTryBeg, ehDsc->ebdTryLast); } - } - - if (callback.EndMerge(block)) - { - // The clients using DataFlow (CSE, assertion prop) currently do - // not need EH successors here: - // - // 1. CSE does not CSE into handlers, so it considers no - // expressions available at the beginning of handlers; - // - // 2. Facts in global assertion prop are VN-based and can only - // become false because of control flow, so it is sufficient to - // propagate facts available into the 'try' head block, since that - // block dominates all other blocks in the 'try'. That will happen - // as part of processing handlers below. - // - block->VisitRegularSuccs(m_pCompiler, [&worklist](BasicBlock* succ) { - worklist.insert(worklist.end(), succ); - return BasicBlockVisit::Continue; - }); - } - - if (m_pCompiler->bbIsTryBeg(block)) - { - // Handlers of the try are reachable (and may require special - // handling compared to the normal "at-the-end" propagation above). - EHblkDsc* eh = m_pCompiler->ehGetDsc(block->getTryIndex()); - do + else { - worklist.insert(worklist.end(), eh->ExFlowBlock()); - - if (eh->ebdEnclosingTryIndex == EHblkDsc::NO_ENCLOSING_INDEX) + for (FlowEdge* pred : block->PredEdges()) { - break; + callback.Merge(block, pred->getSourceBlock(), pred->getDupCount()); } + } - eh = m_pCompiler->ehGetDsc(eh->ebdEnclosingTryIndex); - } while (eh->ebdTryBeg == block); + changed |= callback.EndMerge(block); } - } + } while (changed && m_pCompiler->m_dfsTree->HasCycle()); } From 7f6f7d6b24b80271511c9c26a2022769819d31e5 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 22 Jan 2025 14:38:35 +0100 Subject: [PATCH 2/3] Keep JIT native symbols --- eng/pipelines/coreclr/superpmi-diffs.yml | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/eng/pipelines/coreclr/superpmi-diffs.yml b/eng/pipelines/coreclr/superpmi-diffs.yml index c9cae0c63ac397..811edf5343a279 100644 --- a/eng/pipelines/coreclr/superpmi-diffs.yml +++ b/eng/pipelines/coreclr/superpmi-diffs.yml @@ -74,7 +74,6 @@ extends: platforms: - windows_x64 - windows_x86 - - linux_x64 jobParameters: buildArgs: -s clr.alljits+clr.spmi -c $(_BuildConfig) /p:NoPgoOptimize=true postBuildSteps: @@ -89,6 +88,26 @@ extends: displayName: JIT and SuperPMI Assets condition: not(eq(stageDependencies.EvaluatePaths.evaluate_paths.outputs['SetPathVars_jiteeversionguid.containsChange'], true)) + - template: /eng/pipelines/common/platform-matrix.yml + parameters: + jobTemplate: /eng/pipelines/common/global-build-job.yml + buildConfig: release + platforms: + - linux_x64 + jobParameters: + buildArgs: -s clr.alljits+clr.spmi -c $(_BuildConfig) /p:NoPgoOptimize=true -keepnativesymbols true + postBuildSteps: + - template: /eng/pipelines/common/upload-artifact-step.yml + parameters: + rootFolder: $(Build.SourcesDirectory)/artifacts/bin/coreclr + includeRootFolder: false + archiveType: $(archiveType) + tarCompression: $(tarCompression) + archiveExtension: $(archiveExtension) + artifactName: ReleaseJIT_$(osGroup)$(osSubgroup)_$(archType) + displayName: JIT and SuperPMI Assets + condition: not(eq(stageDependencies.EvaluatePaths.evaluate_paths.outputs['SetPathVars_jiteeversionguid.containsChange'], true)) + - template: /eng/pipelines/common/platform-matrix.yml parameters: jobTemplate: /eng/pipelines/coreclr/templates/superpmi-diffs-job.yml From 7a164bf015b842a501c0d509c61582a7e23036f7 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 23 Jan 2025 10:22:55 +0100 Subject: [PATCH 3/3] Revert "Keep JIT native symbols" This reverts commit 7f6f7d6b24b80271511c9c26a2022769819d31e5. --- eng/pipelines/coreclr/superpmi-diffs.yml | 21 +-------------------- 1 file changed, 1 insertion(+), 20 deletions(-) diff --git a/eng/pipelines/coreclr/superpmi-diffs.yml b/eng/pipelines/coreclr/superpmi-diffs.yml index 811edf5343a279..c9cae0c63ac397 100644 --- a/eng/pipelines/coreclr/superpmi-diffs.yml +++ b/eng/pipelines/coreclr/superpmi-diffs.yml @@ -74,28 +74,9 @@ extends: platforms: - windows_x64 - windows_x86 - jobParameters: - buildArgs: -s clr.alljits+clr.spmi -c $(_BuildConfig) /p:NoPgoOptimize=true - postBuildSteps: - - template: /eng/pipelines/common/upload-artifact-step.yml - parameters: - rootFolder: $(Build.SourcesDirectory)/artifacts/bin/coreclr - includeRootFolder: false - archiveType: $(archiveType) - tarCompression: $(tarCompression) - archiveExtension: $(archiveExtension) - artifactName: ReleaseJIT_$(osGroup)$(osSubgroup)_$(archType) - displayName: JIT and SuperPMI Assets - condition: not(eq(stageDependencies.EvaluatePaths.evaluate_paths.outputs['SetPathVars_jiteeversionguid.containsChange'], true)) - - - template: /eng/pipelines/common/platform-matrix.yml - parameters: - jobTemplate: /eng/pipelines/common/global-build-job.yml - buildConfig: release - platforms: - linux_x64 jobParameters: - buildArgs: -s clr.alljits+clr.spmi -c $(_BuildConfig) /p:NoPgoOptimize=true -keepnativesymbols true + buildArgs: -s clr.alljits+clr.spmi -c $(_BuildConfig) /p:NoPgoOptimize=true postBuildSteps: - template: /eng/pipelines/common/upload-artifact-step.yml parameters: