From fd5a270c1dcff2055b11cbcd41c5b65567d1f5f3 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Fri, 12 May 2023 08:13:05 -0700 Subject: [PATCH 1/3] JIT: do early block merging in more cases OSR and PGO both rely on the fact that the early flowgraph the JIT builds is the same flowgraph as the one seen in a previous JIT complation of that method, since there is metadata (patchpoint offset, pgo schema) that refers to the flowgraph. Previous work here (#85860) didn't ensure this for enough cases. In particular if Tier0 does not do early block merging, but OSR does, it's possible for the OSR entry point to fall within a merged block range, which the JIT is not prepared to handle. This lead to the asserts seen in #86125. The fix is to enable early block merging unless we're truly in a minopts or debug codegen mode (previous to this Tier0 would not merge unless it also was instrumenting; now it will always merge). An alternative fix would be to find the block containing the OSR entry IL offset, scan its statements, and split the block at the right spot, but that seemed more involved. Fixes #86125. --- src/coreclr/jit/compiler.h | 6 ++---- src/coreclr/jit/fgbasic.cpp | 4 ++-- src/coreclr/jit/importer.cpp | 2 +- 3 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 53cf9349a73b3f..2136dec2464640 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -9387,11 +9387,9 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX return IsInstrumented() && jitFlags->IsSet(JitFlags::JIT_FLAG_BBOPT); } - bool CanBeInstrumentedOrIsOptimized() const + bool DoEarlyBlockMerging() const { - return IsInstrumented() || (jitFlags->IsSet(JitFlags::JIT_FLAG_TIER0) && - jitFlags->IsSet(JitFlags::JIT_FLAG_BBINSTR_IF_LOOPS)) || - jitFlags->IsSet(JitFlags::JIT_FLAG_BBOPT); + return IsInstrumented() || jitFlags->IsSet(JitFlags::JIT_FLAG_TIER0) || jitFlags->IsSet(JitFlags::JIT_FLAG_BBOPT); } // true if we should use the PINVOKE_{BEGIN,END} helpers instead of generating diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index 7b19cfd06c9150..eb08b6582eeb2c 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -1826,7 +1826,7 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed if ((jmpDist == 0) && (opcode == CEE_LEAVE || opcode == CEE_LEAVE_S || opcode == CEE_BR || opcode == CEE_BR_S) && - opts.CanBeInstrumentedOrIsOptimized()) + opts.DoEarlyBlockMerging()) { break; /* NOP */ } @@ -2975,7 +2975,7 @@ unsigned Compiler::fgMakeBasicBlocks(const BYTE* codeAddr, IL_OFFSET codeSize, F jmpDist = (sz == 1) ? getI1LittleEndian(codeAddr) : getI4LittleEndian(codeAddr); - if ((jmpDist == 0) && (opcode == CEE_BR || opcode == CEE_BR_S) && opts.CanBeInstrumentedOrIsOptimized()) + if ((jmpDist == 0) && (opcode == CEE_BR || opcode == CEE_BR_S) && opts.DoEarlyBlockMerging()) { continue; /* NOP */ } diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 29297a6b46d6cf..79338b25704f1d 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -7475,7 +7475,7 @@ void Compiler::impImportBlockCode(BasicBlock* block) case CEE_BR_S: jmpDist = (sz == 1) ? getI1LittleEndian(codeAddr) : getI4LittleEndian(codeAddr); - if ((jmpDist == 0) && opts.CanBeInstrumentedOrIsOptimized()) + if ((jmpDist == 0) && opts.DoEarlyBlockMerging()) { break; /* NOP */ } From 0df426f905d944b9c4c23543aa33948b80a5b88f Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Fri, 12 May 2023 11:36:26 -0700 Subject: [PATCH 2/3] format --- src/coreclr/jit/compiler.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 2136dec2464640..99ccf4381e9c59 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -9389,7 +9389,8 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX bool DoEarlyBlockMerging() const { - return IsInstrumented() || jitFlags->IsSet(JitFlags::JIT_FLAG_TIER0) || jitFlags->IsSet(JitFlags::JIT_FLAG_BBOPT); + return IsInstrumented() || jitFlags->IsSet(JitFlags::JIT_FLAG_TIER0) || + jitFlags->IsSet(JitFlags::JIT_FLAG_BBOPT); } // true if we should use the PINVOKE_{BEGIN,END} helpers instead of generating From 1be0489610dbed8f980802ef2fa8aa77ba9e93a5 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Fri, 12 May 2023 19:13:23 -0700 Subject: [PATCH 3/3] revise to not break EnC/Debuggable codegen --- src/coreclr/jit/compiler.h | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 99ccf4381e9c59..e3aa95dffd169a 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -9389,8 +9389,17 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX bool DoEarlyBlockMerging() const { - return IsInstrumented() || jitFlags->IsSet(JitFlags::JIT_FLAG_TIER0) || - jitFlags->IsSet(JitFlags::JIT_FLAG_BBOPT); + if (jitFlags->IsSet(JitFlags::JIT_FLAG_DEBUG_EnC) || jitFlags->IsSet(JitFlags::JIT_FLAG_DEBUG_CODE)) + { + return false; + } + + if (jitFlags->IsSet(JitFlags::JIT_FLAG_MIN_OPT) && !jitFlags->IsSet(JitFlags::JIT_FLAG_TIER0)) + { + return false; + } + + return true; } // true if we should use the PINVOKE_{BEGIN,END} helpers instead of generating