Fix problem with scaling general loop blocks; add dumpers by BruceForstall · Pull Request #99742 · dotnet/runtime

BruceForstall · 2024-03-14T03:07:17Z

Fix a problem where scaling block weights of a range of basic blocks that compose a general loop could encounter a block that is unreachable. This can happen for various kinds of blocks that the JIT doesn't like to remove even if unreachable. Simply skip scaling those blocks.
Add dumpers for FlowGraphDfsTree and BlockToNaturalLoopMap. These can be called from the debugger (perhaps they should be called to output these things to the JitDump).
Update fgDfsBlocksAndRemove() to dump any blocks that it did not remove, even if they were unreachable.

This was found as part of fixing JitOptRepeat: #94250

1. Fix a problem where scaling block weights of a range of basic blocks that compose a general loop could encounter a block that is unreachable. This can happen for various kinds of blocks that the JIT doesn't like to remove even if unreachable. Simply skip scaling those blocks. 2. Add dumpers for `FlowGraphDfsTree` and `BlockToNaturalLoopMap`. These can be called from the debugger (perhaps they should be called to output these things to the JitDump). 3. Update `fgDfsBlocksAndRemove()` to dump any blocks that it did not remove, even if they were unreachable. This was found as part of fixing JitOptRepeat: dotnet#94250

BruceForstall · 2024-03-14T03:07:38Z

@jakobbotsch PTAL
cc @dotnet/jit-contrib

dotnet-policy-service · 2024-03-14T03:07:47Z

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

This includes various fixes that are being separately PR'ed: 1. dotnet#99744: Introduce HandleKindDataIsInvariant helper 2. dotnet#99743: Add basic support for TYP_MASK constants 3. dotnet#99742: Fix problem with scaling general loop blocks; add dumpers 4. dotnet#99740: Add edge likelihood dumping; fix one edge likelihood update case Also: 1. Add support for running JitOptRepeat under JitStress. This is still over-written by JitOptRepeat being forced on at 4 iterations.

jakobbotsch · 2024-03-14T08:58:42Z

+ printf("%02u %02u -> " FMT_BB "[%u, %u]\n", i, rpoNum, block->bbNum, block->bbPreorderNum,
+ block->bbPostorderNum);


i and block->bbPostorderNum is the same thing here.

Well, this is a dumper, so this can help verify that in the output.

jakobbotsch · 2024-03-14T09:09:29Z

+ // DoPhase(this, PHASE_SET_BLOCK_WEIGHTS, &Compiler::optSetBlockWeights);
+ // ? It could be called multiple times.
 optSetBlockWeights();



Hopefully we'll just end up removing optSetBlockWeights entirely in favor of some profile maintenance that means that this call will just go away at some point.

jakobbotsch · 2024-03-14T10:00:38Z

+#ifdef DEBUG
+ // Did we actually remove all the blocks we said we were going to?
+ if (verbose)
+ {
+ if (m_dfsTree->GetPostOrderCount() != fgBBcount)
+ {
+ printf("%u unreachable blocks were not removed:\n", fgBBcount - m_dfsTree->GetPostOrderCount());
+ for (BasicBlock* block : Blocks())
+ {
+ if (!m_dfsTree->Contains(block))
+ {
+ printf(" " FMT_BB "\n", block->bbNum);
+ }
+ }
+ }
+ }
+#endif // DEBUG


Luckily the blocks are still removed in the "graph" sense -- they don't have any preds/succs left anymore, so they don't interact with reachable blocks in any way. I.e. asserts like these are ok:
runtime/src/coreclr/jit/morph.cpp
Lines 13833 to 13835 in b60a541
for (BasicBlock* const pred : block->PredBlocks())
{
assert(m_dfsTree->Contains(pred)); // We should have removed dead blocks before this.
But yeah, we still have this annoying case to check for when iterating the block list.

Co-authored-by: Jakob Botsch Nielsen <Jakob.botsch.nielsen@gmail.com>

BruceForstall · 2024-03-14T03:07:17Z

Fix a problem where scaling block weights of a range of basic blocks that compose a general loop could encounter a block that is unreachable. This can happen for various kinds of blocks that the JIT doesn't like to remove even if unreachable. Simply skip scaling those blocks.
Add dumpers for FlowGraphDfsTree and BlockToNaturalLoopMap. These can be called from the debugger (perhaps they should be called to output these things to the JitDump).
Update fgDfsBlocksAndRemove() to dump any blocks that it did not remove, even if they were unreachable.

This was found as part of fixing JitOptRepeat: #94250

1. Fix a problem where scaling block weights of a range of basic blocks that compose a general loop could encounter a block that is unreachable. This can happen for various kinds of blocks that the JIT doesn't like to remove even if unreachable. Simply skip scaling those blocks. 2. Add dumpers for `FlowGraphDfsTree` and `BlockToNaturalLoopMap`. These can be called from the debugger (perhaps they should be called to output these things to the JitDump). 3. Update `fgDfsBlocksAndRemove()` to dump any blocks that it did not remove, even if they were unreachable. This was found as part of fixing JitOptRepeat: dotnet#94250

BruceForstall · 2024-03-14T03:07:38Z

@jakobbotsch PTAL
cc @dotnet/jit-contrib

dotnet-policy-service · 2024-03-14T03:07:47Z

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

This includes various fixes that are being separately PR'ed: 1. dotnet#99744: Introduce HandleKindDataIsInvariant helper 2. dotnet#99743: Add basic support for TYP_MASK constants 3. dotnet#99742: Fix problem with scaling general loop blocks; add dumpers 4. dotnet#99740: Add edge likelihood dumping; fix one edge likelihood update case Also: 1. Add support for running JitOptRepeat under JitStress. This is still over-written by JitOptRepeat being forced on at 4 iterations.

jakobbotsch · 2024-03-14T08:58:42Z

+ printf("%02u %02u -> " FMT_BB "[%u, %u]\n", i, rpoNum, block->bbNum, block->bbPreorderNum,
+ block->bbPostorderNum);


i and block->bbPostorderNum is the same thing here.

Well, this is a dumper, so this can help verify that in the output.

jakobbotsch · 2024-03-14T09:09:29Z

+ // DoPhase(this, PHASE_SET_BLOCK_WEIGHTS, &Compiler::optSetBlockWeights);
+ // ? It could be called multiple times.
 optSetBlockWeights();



Hopefully we'll just end up removing optSetBlockWeights entirely in favor of some profile maintenance that means that this call will just go away at some point.

jakobbotsch · 2024-03-14T10:00:38Z

+#ifdef DEBUG
+ // Did we actually remove all the blocks we said we were going to?
+ if (verbose)
+ {
+ if (m_dfsTree->GetPostOrderCount() != fgBBcount)
+ {
+ printf("%u unreachable blocks were not removed:\n", fgBBcount - m_dfsTree->GetPostOrderCount());
+ for (BasicBlock* block : Blocks())
+ {
+ if (!m_dfsTree->Contains(block))
+ {
+ printf(" " FMT_BB "\n", block->bbNum);
+ }
+ }
+ }
+ }
+#endif // DEBUG


Luckily the blocks are still removed in the "graph" sense -- they don't have any preds/succs left anymore, so they don't interact with reachable blocks in any way. I.e. asserts like these are ok:
runtime/src/coreclr/jit/morph.cpp
Lines 13833 to 13835 in b60a541
for (BasicBlock* const pred : block->PredBlocks())
{
assert(m_dfsTree->Contains(pred)); // We should have removed dead blocks before this.
But yeah, we still have this annoying case to check for when iterating the block list.

Co-authored-by: Jakob Botsch Nielsen <Jakob.botsch.nielsen@gmail.com>

BruceForstall · 2024-03-14T03:07:17Z

Fix a problem where scaling block weights of a range of basic blocks that compose a general loop could encounter a block that is unreachable. This can happen for various kinds of blocks that the JIT doesn't like to remove even if unreachable. Simply skip scaling those blocks.
Add dumpers for FlowGraphDfsTree and BlockToNaturalLoopMap. These can be called from the debugger (perhaps they should be called to output these things to the JitDump).
Update fgDfsBlocksAndRemove() to dump any blocks that it did not remove, even if they were unreachable.

This was found as part of fixing JitOptRepeat: #94250

1. Fix a problem where scaling block weights of a range of basic blocks that compose a general loop could encounter a block that is unreachable. This can happen for various kinds of blocks that the JIT doesn't like to remove even if unreachable. Simply skip scaling those blocks. 2. Add dumpers for `FlowGraphDfsTree` and `BlockToNaturalLoopMap`. These can be called from the debugger (perhaps they should be called to output these things to the JitDump). 3. Update `fgDfsBlocksAndRemove()` to dump any blocks that it did not remove, even if they were unreachable. This was found as part of fixing JitOptRepeat: dotnet#94250

BruceForstall · 2024-03-14T03:07:38Z

@jakobbotsch PTAL
cc @dotnet/jit-contrib

dotnet-policy-service · 2024-03-14T03:07:47Z

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

This includes various fixes that are being separately PR'ed: 1. dotnet#99744: Introduce HandleKindDataIsInvariant helper 2. dotnet#99743: Add basic support for TYP_MASK constants 3. dotnet#99742: Fix problem with scaling general loop blocks; add dumpers 4. dotnet#99740: Add edge likelihood dumping; fix one edge likelihood update case Also: 1. Add support for running JitOptRepeat under JitStress. This is still over-written by JitOptRepeat being forced on at 4 iterations.

jakobbotsch · 2024-03-14T08:58:42Z

+ printf("%02u %02u -> " FMT_BB "[%u, %u]\n", i, rpoNum, block->bbNum, block->bbPreorderNum,
+ block->bbPostorderNum);


i and block->bbPostorderNum is the same thing here.

Well, this is a dumper, so this can help verify that in the output.

jakobbotsch · 2024-03-14T09:09:29Z

+ // DoPhase(this, PHASE_SET_BLOCK_WEIGHTS, &Compiler::optSetBlockWeights);
+ // ? It could be called multiple times.
 optSetBlockWeights();



Hopefully we'll just end up removing optSetBlockWeights entirely in favor of some profile maintenance that means that this call will just go away at some point.

jakobbotsch · 2024-03-14T10:00:38Z

+#ifdef DEBUG
+ // Did we actually remove all the blocks we said we were going to?
+ if (verbose)
+ {
+ if (m_dfsTree->GetPostOrderCount() != fgBBcount)
+ {
+ printf("%u unreachable blocks were not removed:\n", fgBBcount - m_dfsTree->GetPostOrderCount());
+ for (BasicBlock* block : Blocks())
+ {
+ if (!m_dfsTree->Contains(block))
+ {
+ printf(" " FMT_BB "\n", block->bbNum);
+ }
+ }
+ }
+ }
+#endif // DEBUG


Luckily the blocks are still removed in the "graph" sense -- they don't have any preds/succs left anymore, so they don't interact with reachable blocks in any way. I.e. asserts like these are ok:
runtime/src/coreclr/jit/morph.cpp
Lines 13833 to 13835 in b60a541
for (BasicBlock* const pred : block->PredBlocks())
{
assert(m_dfsTree->Contains(pred)); // We should have removed dead blocks before this.
But yeah, we still have this annoying case to check for when iterating the block list.

Co-authored-by: Jakob Botsch Nielsen <Jakob.botsch.nielsen@gmail.com>

BruceForstall · 2024-03-14T03:07:17Z

Fix a problem where scaling block weights of a range of basic blocks that compose a general loop could encounter a block that is unreachable. This can happen for various kinds of blocks that the JIT doesn't like to remove even if unreachable. Simply skip scaling those blocks.
Add dumpers for FlowGraphDfsTree and BlockToNaturalLoopMap. These can be called from the debugger (perhaps they should be called to output these things to the JitDump).
Update fgDfsBlocksAndRemove() to dump any blocks that it did not remove, even if they were unreachable.

This was found as part of fixing JitOptRepeat: #94250

1. Fix a problem where scaling block weights of a range of basic blocks that compose a general loop could encounter a block that is unreachable. This can happen for various kinds of blocks that the JIT doesn't like to remove even if unreachable. Simply skip scaling those blocks. 2. Add dumpers for `FlowGraphDfsTree` and `BlockToNaturalLoopMap`. These can be called from the debugger (perhaps they should be called to output these things to the JitDump). 3. Update `fgDfsBlocksAndRemove()` to dump any blocks that it did not remove, even if they were unreachable. This was found as part of fixing JitOptRepeat: dotnet#94250

BruceForstall · 2024-03-14T03:07:38Z

@jakobbotsch PTAL
cc @dotnet/jit-contrib

dotnet-policy-service · 2024-03-14T03:07:47Z

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

This includes various fixes that are being separately PR'ed: 1. dotnet#99744: Introduce HandleKindDataIsInvariant helper 2. dotnet#99743: Add basic support for TYP_MASK constants 3. dotnet#99742: Fix problem with scaling general loop blocks; add dumpers 4. dotnet#99740: Add edge likelihood dumping; fix one edge likelihood update case Also: 1. Add support for running JitOptRepeat under JitStress. This is still over-written by JitOptRepeat being forced on at 4 iterations.

jakobbotsch · 2024-03-14T08:58:42Z

+ printf("%02u %02u -> " FMT_BB "[%u, %u]\n", i, rpoNum, block->bbNum, block->bbPreorderNum,
+ block->bbPostorderNum);


i and block->bbPostorderNum is the same thing here.

Well, this is a dumper, so this can help verify that in the output.

jakobbotsch · 2024-03-14T09:09:29Z

+ // DoPhase(this, PHASE_SET_BLOCK_WEIGHTS, &Compiler::optSetBlockWeights);
+ // ? It could be called multiple times.
 optSetBlockWeights();



Hopefully we'll just end up removing optSetBlockWeights entirely in favor of some profile maintenance that means that this call will just go away at some point.

jakobbotsch · 2024-03-14T10:00:38Z

+#ifdef DEBUG
+ // Did we actually remove all the blocks we said we were going to?
+ if (verbose)
+ {
+ if (m_dfsTree->GetPostOrderCount() != fgBBcount)
+ {
+ printf("%u unreachable blocks were not removed:\n", fgBBcount - m_dfsTree->GetPostOrderCount());
+ for (BasicBlock* block : Blocks())
+ {
+ if (!m_dfsTree->Contains(block))
+ {
+ printf(" " FMT_BB "\n", block->bbNum);
+ }
+ }
+ }
+ }
+#endif // DEBUG


Luckily the blocks are still removed in the "graph" sense -- they don't have any preds/succs left anymore, so they don't interact with reachable blocks in any way. I.e. asserts like these are ok:
runtime/src/coreclr/jit/morph.cpp
Lines 13833 to 13835 in b60a541
for (BasicBlock* const pred : block->PredBlocks())
{
assert(m_dfsTree->Contains(pred)); // We should have removed dead blocks before this.
But yeah, we still have this annoying case to check for when iterating the block list.

Co-authored-by: Jakob Botsch Nielsen <Jakob.botsch.nielsen@gmail.com>

BruceForstall · 2024-03-14T03:07:17Z

Fix a problem where scaling block weights of a range of basic blocks that compose a general loop could encounter a block that is unreachable. This can happen for various kinds of blocks that the JIT doesn't like to remove even if unreachable. Simply skip scaling those blocks.
Add dumpers for FlowGraphDfsTree and BlockToNaturalLoopMap. These can be called from the debugger (perhaps they should be called to output these things to the JitDump).
Update fgDfsBlocksAndRemove() to dump any blocks that it did not remove, even if they were unreachable.

This was found as part of fixing JitOptRepeat: #94250

1. Fix a problem where scaling block weights of a range of basic blocks that compose a general loop could encounter a block that is unreachable. This can happen for various kinds of blocks that the JIT doesn't like to remove even if unreachable. Simply skip scaling those blocks. 2. Add dumpers for `FlowGraphDfsTree` and `BlockToNaturalLoopMap`. These can be called from the debugger (perhaps they should be called to output these things to the JitDump). 3. Update `fgDfsBlocksAndRemove()` to dump any blocks that it did not remove, even if they were unreachable. This was found as part of fixing JitOptRepeat: dotnet#94250

BruceForstall · 2024-03-14T03:07:38Z

@jakobbotsch PTAL
cc @dotnet/jit-contrib

dotnet-policy-service · 2024-03-14T03:07:47Z

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

This includes various fixes that are being separately PR'ed: 1. dotnet#99744: Introduce HandleKindDataIsInvariant helper 2. dotnet#99743: Add basic support for TYP_MASK constants 3. dotnet#99742: Fix problem with scaling general loop blocks; add dumpers 4. dotnet#99740: Add edge likelihood dumping; fix one edge likelihood update case Also: 1. Add support for running JitOptRepeat under JitStress. This is still over-written by JitOptRepeat being forced on at 4 iterations.

jakobbotsch · 2024-03-14T08:58:42Z

+ printf("%02u %02u -> " FMT_BB "[%u, %u]\n", i, rpoNum, block->bbNum, block->bbPreorderNum,
+ block->bbPostorderNum);


i and block->bbPostorderNum is the same thing here.

Well, this is a dumper, so this can help verify that in the output.

jakobbotsch · 2024-03-14T09:09:29Z

+ // DoPhase(this, PHASE_SET_BLOCK_WEIGHTS, &Compiler::optSetBlockWeights);
+ // ? It could be called multiple times.
 optSetBlockWeights();



Hopefully we'll just end up removing optSetBlockWeights entirely in favor of some profile maintenance that means that this call will just go away at some point.

jakobbotsch · 2024-03-14T10:00:38Z

+#ifdef DEBUG
+ // Did we actually remove all the blocks we said we were going to?
+ if (verbose)
+ {
+ if (m_dfsTree->GetPostOrderCount() != fgBBcount)
+ {
+ printf("%u unreachable blocks were not removed:\n", fgBBcount - m_dfsTree->GetPostOrderCount());
+ for (BasicBlock* block : Blocks())
+ {
+ if (!m_dfsTree->Contains(block))
+ {
+ printf(" " FMT_BB "\n", block->bbNum);
+ }
+ }
+ }
+ }
+#endif // DEBUG


Luckily the blocks are still removed in the "graph" sense -- they don't have any preds/succs left anymore, so they don't interact with reachable blocks in any way. I.e. asserts like these are ok:
runtime/src/coreclr/jit/morph.cpp
Lines 13833 to 13835 in b60a541
for (BasicBlock* const pred : block->PredBlocks())
{
assert(m_dfsTree->Contains(pred)); // We should have removed dead blocks before this.
But yeah, we still have this annoying case to check for when iterating the block list.

Co-authored-by: Jakob Botsch Nielsen <Jakob.botsch.nielsen@gmail.com>

BruceForstall · 2024-03-14T03:07:17Z

Fix a problem where scaling block weights of a range of basic blocks that compose a general loop could encounter a block that is unreachable. This can happen for various kinds of blocks that the JIT doesn't like to remove even if unreachable. Simply skip scaling those blocks.
Add dumpers for FlowGraphDfsTree and BlockToNaturalLoopMap. These can be called from the debugger (perhaps they should be called to output these things to the JitDump).
Update fgDfsBlocksAndRemove() to dump any blocks that it did not remove, even if they were unreachable.

This was found as part of fixing JitOptRepeat: #94250

1. Fix a problem where scaling block weights of a range of basic blocks that compose a general loop could encounter a block that is unreachable. This can happen for various kinds of blocks that the JIT doesn't like to remove even if unreachable. Simply skip scaling those blocks. 2. Add dumpers for `FlowGraphDfsTree` and `BlockToNaturalLoopMap`. These can be called from the debugger (perhaps they should be called to output these things to the JitDump). 3. Update `fgDfsBlocksAndRemove()` to dump any blocks that it did not remove, even if they were unreachable. This was found as part of fixing JitOptRepeat: dotnet#94250

BruceForstall · 2024-03-14T03:07:38Z

@jakobbotsch PTAL
cc @dotnet/jit-contrib

dotnet-policy-service · 2024-03-14T03:07:47Z

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

This includes various fixes that are being separately PR'ed: 1. dotnet#99744: Introduce HandleKindDataIsInvariant helper 2. dotnet#99743: Add basic support for TYP_MASK constants 3. dotnet#99742: Fix problem with scaling general loop blocks; add dumpers 4. dotnet#99740: Add edge likelihood dumping; fix one edge likelihood update case Also: 1. Add support for running JitOptRepeat under JitStress. This is still over-written by JitOptRepeat being forced on at 4 iterations.

jakobbotsch · 2024-03-14T08:58:42Z

+ printf("%02u %02u -> " FMT_BB "[%u, %u]\n", i, rpoNum, block->bbNum, block->bbPreorderNum,
+ block->bbPostorderNum);


i and block->bbPostorderNum is the same thing here.

Well, this is a dumper, so this can help verify that in the output.

jakobbotsch · 2024-03-14T09:09:29Z

+ // DoPhase(this, PHASE_SET_BLOCK_WEIGHTS, &Compiler::optSetBlockWeights);
+ // ? It could be called multiple times.
 optSetBlockWeights();



Hopefully we'll just end up removing optSetBlockWeights entirely in favor of some profile maintenance that means that this call will just go away at some point.

jakobbotsch · 2024-03-14T10:00:38Z

+#ifdef DEBUG
+ // Did we actually remove all the blocks we said we were going to?
+ if (verbose)
+ {
+ if (m_dfsTree->GetPostOrderCount() != fgBBcount)
+ {
+ printf("%u unreachable blocks were not removed:\n", fgBBcount - m_dfsTree->GetPostOrderCount());
+ for (BasicBlock* block : Blocks())
+ {
+ if (!m_dfsTree->Contains(block))
+ {
+ printf(" " FMT_BB "\n", block->bbNum);
+ }
+ }
+ }
+ }
+#endif // DEBUG


Luckily the blocks are still removed in the "graph" sense -- they don't have any preds/succs left anymore, so they don't interact with reachable blocks in any way. I.e. asserts like these are ok:
runtime/src/coreclr/jit/morph.cpp
Lines 13833 to 13835 in b60a541
for (BasicBlock* const pred : block->PredBlocks())
{
assert(m_dfsTree->Contains(pred)); // We should have removed dead blocks before this.
But yeah, we still have this annoying case to check for when iterating the block list.

Co-authored-by: Jakob Botsch Nielsen <Jakob.botsch.nielsen@gmail.com>

BruceForstall · 2024-03-14T03:07:17Z

Fix a problem where scaling block weights of a range of basic blocks that compose a general loop could encounter a block that is unreachable. This can happen for various kinds of blocks that the JIT doesn't like to remove even if unreachable. Simply skip scaling those blocks.
Add dumpers for FlowGraphDfsTree and BlockToNaturalLoopMap. These can be called from the debugger (perhaps they should be called to output these things to the JitDump).
Update fgDfsBlocksAndRemove() to dump any blocks that it did not remove, even if they were unreachable.

This was found as part of fixing JitOptRepeat: #94250

1. Fix a problem where scaling block weights of a range of basic blocks that compose a general loop could encounter a block that is unreachable. This can happen for various kinds of blocks that the JIT doesn't like to remove even if unreachable. Simply skip scaling those blocks. 2. Add dumpers for `FlowGraphDfsTree` and `BlockToNaturalLoopMap`. These can be called from the debugger (perhaps they should be called to output these things to the JitDump). 3. Update `fgDfsBlocksAndRemove()` to dump any blocks that it did not remove, even if they were unreachable. This was found as part of fixing JitOptRepeat: dotnet#94250

BruceForstall · 2024-03-14T03:07:38Z

@jakobbotsch PTAL
cc @dotnet/jit-contrib

dotnet-policy-service · 2024-03-14T03:07:47Z

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

This includes various fixes that are being separately PR'ed: 1. dotnet#99744: Introduce HandleKindDataIsInvariant helper 2. dotnet#99743: Add basic support for TYP_MASK constants 3. dotnet#99742: Fix problem with scaling general loop blocks; add dumpers 4. dotnet#99740: Add edge likelihood dumping; fix one edge likelihood update case Also: 1. Add support for running JitOptRepeat under JitStress. This is still over-written by JitOptRepeat being forced on at 4 iterations.

jakobbotsch · 2024-03-14T08:58:42Z

+ printf("%02u %02u -> " FMT_BB "[%u, %u]\n", i, rpoNum, block->bbNum, block->bbPreorderNum,
+ block->bbPostorderNum);


i and block->bbPostorderNum is the same thing here.

Well, this is a dumper, so this can help verify that in the output.

jakobbotsch · 2024-03-14T09:09:29Z

+ // DoPhase(this, PHASE_SET_BLOCK_WEIGHTS, &Compiler::optSetBlockWeights);
+ // ? It could be called multiple times.
 optSetBlockWeights();



Hopefully we'll just end up removing optSetBlockWeights entirely in favor of some profile maintenance that means that this call will just go away at some point.

jakobbotsch · 2024-03-14T10:00:38Z

+#ifdef DEBUG
+ // Did we actually remove all the blocks we said we were going to?
+ if (verbose)
+ {
+ if (m_dfsTree->GetPostOrderCount() != fgBBcount)
+ {
+ printf("%u unreachable blocks were not removed:\n", fgBBcount - m_dfsTree->GetPostOrderCount());
+ for (BasicBlock* block : Blocks())
+ {
+ if (!m_dfsTree->Contains(block))
+ {
+ printf(" " FMT_BB "\n", block->bbNum);
+ }
+ }
+ }
+ }
+#endif // DEBUG


Luckily the blocks are still removed in the "graph" sense -- they don't have any preds/succs left anymore, so they don't interact with reachable blocks in any way. I.e. asserts like these are ok:
runtime/src/coreclr/jit/morph.cpp
Lines 13833 to 13835 in b60a541
for (BasicBlock* const pred : block->PredBlocks())
{
assert(m_dfsTree->Contains(pred)); // We should have removed dead blocks before this.
But yeah, we still have this annoying case to check for when iterating the block list.

Co-authored-by: Jakob Botsch Nielsen <Jakob.botsch.nielsen@gmail.com>

BruceForstall · 2024-03-14T03:07:17Z

Fix a problem where scaling block weights of a range of basic blocks that compose a general loop could encounter a block that is unreachable. This can happen for various kinds of blocks that the JIT doesn't like to remove even if unreachable. Simply skip scaling those blocks.
Add dumpers for FlowGraphDfsTree and BlockToNaturalLoopMap. These can be called from the debugger (perhaps they should be called to output these things to the JitDump).
Update fgDfsBlocksAndRemove() to dump any blocks that it did not remove, even if they were unreachable.

This was found as part of fixing JitOptRepeat: #94250

1. Fix a problem where scaling block weights of a range of basic blocks that compose a general loop could encounter a block that is unreachable. This can happen for various kinds of blocks that the JIT doesn't like to remove even if unreachable. Simply skip scaling those blocks. 2. Add dumpers for `FlowGraphDfsTree` and `BlockToNaturalLoopMap`. These can be called from the debugger (perhaps they should be called to output these things to the JitDump). 3. Update `fgDfsBlocksAndRemove()` to dump any blocks that it did not remove, even if they were unreachable. This was found as part of fixing JitOptRepeat: dotnet#94250

BruceForstall · 2024-03-14T03:07:38Z

@jakobbotsch PTAL
cc @dotnet/jit-contrib

dotnet-policy-service · 2024-03-14T03:07:47Z

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

This includes various fixes that are being separately PR'ed: 1. dotnet#99744: Introduce HandleKindDataIsInvariant helper 2. dotnet#99743: Add basic support for TYP_MASK constants 3. dotnet#99742: Fix problem with scaling general loop blocks; add dumpers 4. dotnet#99740: Add edge likelihood dumping; fix one edge likelihood update case Also: 1. Add support for running JitOptRepeat under JitStress. This is still over-written by JitOptRepeat being forced on at 4 iterations.

jakobbotsch · 2024-03-14T08:58:42Z

+ printf("%02u %02u -> " FMT_BB "[%u, %u]\n", i, rpoNum, block->bbNum, block->bbPreorderNum,
+ block->bbPostorderNum);


i and block->bbPostorderNum is the same thing here.

Well, this is a dumper, so this can help verify that in the output.

jakobbotsch · 2024-03-14T09:09:29Z

+ // DoPhase(this, PHASE_SET_BLOCK_WEIGHTS, &Compiler::optSetBlockWeights);
+ // ? It could be called multiple times.
 optSetBlockWeights();



Hopefully we'll just end up removing optSetBlockWeights entirely in favor of some profile maintenance that means that this call will just go away at some point.

jakobbotsch · 2024-03-14T10:00:38Z

+#ifdef DEBUG
+ // Did we actually remove all the blocks we said we were going to?
+ if (verbose)
+ {
+ if (m_dfsTree->GetPostOrderCount() != fgBBcount)
+ {
+ printf("%u unreachable blocks were not removed:\n", fgBBcount - m_dfsTree->GetPostOrderCount());
+ for (BasicBlock* block : Blocks())
+ {
+ if (!m_dfsTree->Contains(block))
+ {
+ printf(" " FMT_BB "\n", block->bbNum);
+ }
+ }
+ }
+ }
+#endif // DEBUG


Luckily the blocks are still removed in the "graph" sense -- they don't have any preds/succs left anymore, so they don't interact with reachable blocks in any way. I.e. asserts like these are ok:
runtime/src/coreclr/jit/morph.cpp
Lines 13833 to 13835 in b60a541
for (BasicBlock* const pred : block->PredBlocks())
{
assert(m_dfsTree->Contains(pred)); // We should have removed dead blocks before this.
But yeah, we still have this annoying case to check for when iterating the block list.

Co-authored-by: Jakob Botsch Nielsen <Jakob.botsch.nielsen@gmail.com>

ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 14, 2024

BruceForstall requested a review from jakobbotsch March 14, 2024 03:07

dotnet-policy-serviceBot assigned BruceForstall Mar 14, 2024

build-analysisBot mentioned this pull request Mar 14, 2024
System.DirectoryServices.Protocols.Tests.BerConverterTests.Decode_Bytes_ReturnsExpected failing on Windows x86 #99725
Closed

jakobbotsch reviewed Mar 14, 2024
View reviewed changes

Comment threadsrc/coreclr/jit/flowgraph.cpp

jakobbotsch reviewed Mar 14, 2024
View reviewed changes

BruceForstalland others added 2 commits March 14, 2024 10:03

Update src/coreclr/jit/flowgraph.cpp
60a1611
Co-authored-by: Jakob Botsch Nielsen <Jakob.botsch.nielsen@gmail.com>

Update dump header
0511199

jakobbotsch approved these changes Mar 14, 2024
View reviewed changes

BruceForstall merged commit 33d737a into dotnet:mainMar 14, 2024

BruceForstall deleted the HandleUnreachableBlocksInGeneralLoopScaling branch March 14, 2024 21:26

github-actionsBot locked and limited conversation to collaborators Apr 14, 2024