Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JIT: rely on DFS for cycle entry detection in optRemoveRedundantZeroInits #103788

Merged
merged 3 commits into from
Jun 21, 2024

Conversation

AndyAyersMS
Copy link
Member

If a block is DFS ancestor of one of its preds, the block is a cycle entry. Use this instead of BBF_BACKWARD_JUMP to stop the scan of blocks when looking for redundant zero inits.

Fixes #103477 (in main).

…nits

If a block is DFS ancestor of one of its preds, the block is a cycle entry.
Use this instead of `BBF_BACKWARD_JUMP` to stop the scan of blocks when
looking for redundant zero inits.

Fixes dotnet#103477 (in main).
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jun 20, 2024
Copy link
Contributor

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

@AndyAyersMS
Copy link
Member Author

@jakobbotsch PTAL
cc @dotnet/jit-contrib

Small number of diffs expected.

@EgorBo
Copy link
Member

EgorBo commented Jun 20, 2024

should we leave a comment on BBF_BACKWARD_JUMP flag that it's not precise outside of importer? also, do we want to add the original issue as a test?

@AndyAyersMS
Copy link
Member Author

should we leave a comment on BBF_BACKWARD_JUMP flag that it's not precise outside of importer? also, do we want to add the original issue as a test?

It's still used in some other places, we'll need to dig into those. Early enough is ok.

I am trying to adapt the test case above into something that I can add.

// See if this block is a cycle entry
//
bool stop = false;
for (BasicBlock* const pred : block->PredBlocks())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could use BlockPredsWithEH... It probably doesn't actually matter since we walk forwards with block->GetUniqueSucc(), so won't ever end up at a handler, but it would be more consistent with the graph that the DFS tree is constructed over.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done... no change in SPMI diffs.

@AndyAyersMS
Copy link
Member Author

Some thoughts for follow up:

  • do we need to run this opt this early? It should would work better later when we've had a chance to optimize branches
  • could this be subsumed by VN dead stores?

@AndyAyersMS
Copy link
Member Author

CI errors are known.

@AndyAyersMS AndyAyersMS merged commit ceb0a16 into dotnet:main Jun 21, 2024
108 of 113 checks passed
rzikm pushed a commit to rzikm/dotnet-runtime that referenced this pull request Jun 24, 2024
…nits (dotnet#103788)

If a block is DFS ancestor of one of its preds, the block is a cycle entry.
Use this instead of `BBF_BACKWARD_JUMP` to stop the scan of blocks when
looking for redundant zero inits.

Fixes dotnet#103477 (in main).
AndyAyersMS added a commit to AndyAyersMS/runtime that referenced this pull request Jun 24, 2024
"Port" the changes from dotnet#103788 to .NET 8.

We don't have the DFS tree, so enhance the topological sort that SSA uses
to detect cycles and backedges.

Use this info to stop searching for redundant zero inits once flow from
entry has entered a cycle.

Fixes dotnet#103477 (for .NET 8).
AndyAyersMS added a commit to AndyAyersMS/runtime that referenced this pull request Jun 24, 2024
"Port" the changes from dotnet#103788 to .NET 8.

We don't have the DFS tree, so enhance the topological sort that SSA uses
to detect cycles and backedges.

Use this info to stop searching for redundant zero inits once flow from
entry has entered a cycle.

Fixes dotnet#103477 (for .NET 8).
AndyAyersMS added a commit to AndyAyersMS/runtime that referenced this pull request Jun 24, 2024
"Port" the changes from dotnet#103788 to .NET 8.

We don't have the DFS tree, so enhance the topological sort that SSA uses
to detect cycles and backedges.

Use this info to stop searching for redundant zero inits once flow from
entry has entered a cycle.

Fixes dotnet#103477 (for .NET 8).
AndyAyersMS added a commit to AndyAyersMS/runtime that referenced this pull request Jun 24, 2024
"Port" the changes from dotnet#103788 to .NET 8.

We don't have the DFS tree, so enhance the topological sort that SSA uses
to detect cycles and backedges.

Use this info to stop searching for redundant zero inits once flow from
entry has entered a cycle.

Fixes dotnet#103477 (for .NET 8).
AndyAyersMS added a commit that referenced this pull request Jul 12, 2024
"Port" the changes from #103788 to .NET 8.

We don't have the DFS tree, so enhance the topological sort that SSA uses
to detect cycles and backedges.

Use this info to stop searching for redundant zero inits once flow from
entry has entered a cycle.

Fixes #103477 (for .NET 8).
@github-actions github-actions bot locked and limited conversation to collaborators Jul 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optimization removes necessary code
3 participants