JIT: produce consistent flow graph when producing or consuming profile data by AndyAyersMS · Pull Request #85860 · dotnet/runtime

AndyAyersMS · 2023-05-06T03:04:17Z

Always try and merge "branch to next" blocks when building the initial flow graph if BBINSTR or BBOPT is set.

…e data Always try and merge "branch to next" blocks when building the intial flow graph if BBINSTR or BBOPT is set. Fixesdotnet#85856.

ghost · 2023-05-06T03:04:29Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Always try and merge "branch to next" blocks when building the initial flow graph if BBINSTR or BBOPT is set.

Fixes #85856.

Author:	AndyAyersMS
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

AndyAyersMS · 2023-05-06T03:05:14Z

@EgorBo PTAL
cc @dotnet/jit-contrib

This is going to cause some diffs and perhaps some (short lived) perf regressions, until static PGO catches up with updated schemas for some methods.

AndyAyersMS · 2023-05-06T14:38:34Z

Diffs from CI are more extensive than what I saw locally, let me pull down the latest MCHs and recheck this.

Hmm, I guess I just misremembered what I'd seen.

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 (dotnet#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 dotnet#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. Fixesdotnet#86125.

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.

AndyAyersMS · 2023-05-06T03:04:17Z

Always try and merge "branch to next" blocks when building the initial flow graph if BBINSTR or BBOPT is set.

Fixes#85856.

…e data Always try and merge "branch to next" blocks when building the intial flow graph if BBINSTR or BBOPT is set. Fixesdotnet#85856.

ghost · 2023-05-06T03:04:29Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Always try and merge "branch to next" blocks when building the initial flow graph if BBINSTR or BBOPT is set.

Fixes #85856.

Author:	AndyAyersMS
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

AndyAyersMS · 2023-05-06T03:05:14Z

@EgorBo PTAL
cc @dotnet/jit-contrib

This is going to cause some diffs and perhaps some (short lived) perf regressions, until static PGO catches up with updated schemas for some methods.

AndyAyersMS · 2023-05-06T14:38:34Z

Diffs from CI are more extensive than what I saw locally, let me pull down the latest MCHs and recheck this.

Hmm, I guess I just misremembered what I'd seen.

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 (dotnet#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 dotnet#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. Fixesdotnet#86125.

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.

AndyAyersMS · 2023-05-06T03:04:17Z

Always try and merge "branch to next" blocks when building the initial flow graph if BBINSTR or BBOPT is set.

Fixes#85856.

…e data Always try and merge "branch to next" blocks when building the intial flow graph if BBINSTR or BBOPT is set. Fixesdotnet#85856.

ghost · 2023-05-06T03:04:29Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Always try and merge "branch to next" blocks when building the initial flow graph if BBINSTR or BBOPT is set.

Fixes #85856.

Author:	AndyAyersMS
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

AndyAyersMS · 2023-05-06T03:05:14Z

@EgorBo PTAL
cc @dotnet/jit-contrib

This is going to cause some diffs and perhaps some (short lived) perf regressions, until static PGO catches up with updated schemas for some methods.

AndyAyersMS · 2023-05-06T14:38:34Z

Diffs from CI are more extensive than what I saw locally, let me pull down the latest MCHs and recheck this.

Hmm, I guess I just misremembered what I'd seen.

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 (dotnet#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 dotnet#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. Fixesdotnet#86125.

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.

AndyAyersMS · 2023-05-06T03:04:17Z

Always try and merge "branch to next" blocks when building the initial flow graph if BBINSTR or BBOPT is set.

Fixes#85856.

…e data Always try and merge "branch to next" blocks when building the intial flow graph if BBINSTR or BBOPT is set. Fixesdotnet#85856.

ghost · 2023-05-06T03:04:29Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Always try and merge "branch to next" blocks when building the initial flow graph if BBINSTR or BBOPT is set.

Fixes #85856.

Author:	AndyAyersMS
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

AndyAyersMS · 2023-05-06T03:05:14Z

@EgorBo PTAL
cc @dotnet/jit-contrib

This is going to cause some diffs and perhaps some (short lived) perf regressions, until static PGO catches up with updated schemas for some methods.

AndyAyersMS · 2023-05-06T14:38:34Z

Diffs from CI are more extensive than what I saw locally, let me pull down the latest MCHs and recheck this.

Hmm, I guess I just misremembered what I'd seen.

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 (dotnet#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 dotnet#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. Fixesdotnet#86125.

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.

AndyAyersMS · 2023-05-06T03:04:17Z

Always try and merge "branch to next" blocks when building the initial flow graph if BBINSTR or BBOPT is set.

Fixes#85856.

…e data Always try and merge "branch to next" blocks when building the intial flow graph if BBINSTR or BBOPT is set. Fixesdotnet#85856.

ghost · 2023-05-06T03:04:29Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Always try and merge "branch to next" blocks when building the initial flow graph if BBINSTR or BBOPT is set.

Fixes #85856.

Author:	AndyAyersMS
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

AndyAyersMS · 2023-05-06T03:05:14Z

@EgorBo PTAL
cc @dotnet/jit-contrib

This is going to cause some diffs and perhaps some (short lived) perf regressions, until static PGO catches up with updated schemas for some methods.

AndyAyersMS · 2023-05-06T14:38:34Z

Diffs from CI are more extensive than what I saw locally, let me pull down the latest MCHs and recheck this.

Hmm, I guess I just misremembered what I'd seen.

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 (dotnet#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 dotnet#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. Fixesdotnet#86125.

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.

AndyAyersMS · 2023-05-06T03:04:17Z

Always try and merge "branch to next" blocks when building the initial flow graph if BBINSTR or BBOPT is set.

Fixes#85856.

…e data Always try and merge "branch to next" blocks when building the intial flow graph if BBINSTR or BBOPT is set. Fixesdotnet#85856.

ghost · 2023-05-06T03:04:29Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Always try and merge "branch to next" blocks when building the initial flow graph if BBINSTR or BBOPT is set.

Fixes #85856.

Author:	AndyAyersMS
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

AndyAyersMS · 2023-05-06T03:05:14Z

@EgorBo PTAL
cc @dotnet/jit-contrib

This is going to cause some diffs and perhaps some (short lived) perf regressions, until static PGO catches up with updated schemas for some methods.

AndyAyersMS · 2023-05-06T14:38:34Z

Diffs from CI are more extensive than what I saw locally, let me pull down the latest MCHs and recheck this.

Hmm, I guess I just misremembered what I'd seen.

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 (dotnet#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 dotnet#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. Fixesdotnet#86125.

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.

AndyAyersMS · 2023-05-06T03:04:17Z

Always try and merge "branch to next" blocks when building the initial flow graph if BBINSTR or BBOPT is set.

Fixes#85856.

…e data Always try and merge "branch to next" blocks when building the intial flow graph if BBINSTR or BBOPT is set. Fixesdotnet#85856.

ghost · 2023-05-06T03:04:29Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Always try and merge "branch to next" blocks when building the initial flow graph if BBINSTR or BBOPT is set.

Fixes #85856.

Author:	AndyAyersMS
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

AndyAyersMS · 2023-05-06T03:05:14Z

@EgorBo PTAL
cc @dotnet/jit-contrib

This is going to cause some diffs and perhaps some (short lived) perf regressions, until static PGO catches up with updated schemas for some methods.

AndyAyersMS · 2023-05-06T14:38:34Z

Diffs from CI are more extensive than what I saw locally, let me pull down the latest MCHs and recheck this.

Hmm, I guess I just misremembered what I'd seen.

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 (dotnet#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 dotnet#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. Fixesdotnet#86125.

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.

AndyAyersMS · 2023-05-06T03:04:17Z

Always try and merge "branch to next" blocks when building the initial flow graph if BBINSTR or BBOPT is set.

Fixes#85856.

…e data Always try and merge "branch to next" blocks when building the intial flow graph if BBINSTR or BBOPT is set. Fixesdotnet#85856.

ghost · 2023-05-06T03:04:29Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Always try and merge "branch to next" blocks when building the initial flow graph if BBINSTR or BBOPT is set.

Fixes #85856.

Author:	AndyAyersMS
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

AndyAyersMS · 2023-05-06T03:05:14Z

@EgorBo PTAL
cc @dotnet/jit-contrib

This is going to cause some diffs and perhaps some (short lived) perf regressions, until static PGO catches up with updated schemas for some methods.

AndyAyersMS · 2023-05-06T14:38:34Z

Diffs from CI are more extensive than what I saw locally, let me pull down the latest MCHs and recheck this.

Hmm, I guess I just misremembered what I'd seen.

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 (dotnet#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 dotnet#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. Fixesdotnet#86125.

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.

JIT: produce consistent flow graph when producing or consuming profil…
cfebab3
…e data Always try and merge "branch to next" blocks when building the intial flow graph if BBINSTR or BBOPT is set. Fixesdotnet#85856.

ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label May 6, 2023

ghost assigned AndyAyersMS May 6, 2023

EgorBo approved these changes May 6, 2023
View reviewed changes

AndyAyersMS merged commit ae1be12 into dotnet:mainMay 6, 2023

This was referenced May 8, 2023
JIT: assert if we see schema mismatches with dynamic pgo data #85898
Merged
Investigate microbenchmarks that regress with PGO enabled #84264
Closed

AndyAyersMS mentioned this pull request May 12, 2023
OSR tests failing in jit-experimental pipeline #86125
Closed

AndyAyersMS mentioned this pull request May 12, 2023
JIT: do early block merging in more cases #86157
Merged

ghost locked as resolved and limited conversation to collaborators Jun 5, 2023