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

Improve the redundant branch optimization to handle more side effects #68447

Merged

Conversation

SingleAccretion
Copy link
Contributor

@SingleAccretion SingleAccretion commented Apr 23, 2022

The redundant branches optimization was only handling exact-exception-sets matches, but we can do better by:

  1. Discarding the exceptions if we know the dominating compare produces a superset.
  2. Otherwise, extracting the side-effects, but still simplifying the flow.

This change does just that, producing some nice diffs.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Apr 23, 2022
@dotnet-issue-labeler dotnet-issue-labeler bot added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI and removed community-contribution Indicates that the PR has been added by a community member labels Apr 23, 2022
@ghost
Copy link

ghost commented Apr 23, 2022

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

Issue Details

Testing CI.

Author: SingleAccretion
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@SingleAccretion SingleAccretion force-pushed the Redundant-Branches-SideEffects branch from aa76fc2 to 41d240c Compare April 23, 2022 20:33
@SingleAccretion SingleAccretion marked this pull request as ready for review April 24, 2022 20:18
@SingleAccretion
Copy link
Contributor Author

@dotnet/jit-contrib

@jakobbotsch
Copy link
Member

/azp run Antigen, Fuzzlyn

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

Changes look good. Thanks!

I suspect the Fuzzlyn failures are all unrelated. Didn't look through the Antigen ones.

@@ -258,34 +262,41 @@ bool Compiler::optRedundantBranch(BasicBlock* const block)
return false;
}

// Bail out if tree is has certain side effects
// Be conservative if there is an exception effect and we're in an EH region
Copy link
Member

Choose a reason for hiding this comment

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

Note to self: not sure why we don't check this condition earlier. No point in searching for dominating compares if we're just going to bail out here like this.

@kunalspathak
Copy link
Member

Didn't look through the Antigen ones.

Antigen failures are also unrelated.

@AndyAyersMS
Copy link
Member

I suspect the Fuzzlyn failures are all unrelated

@jakobbotsch can you look these over just to be sure?

@jakobbotsch
Copy link
Member

@jakobbotsch can you look these over just to be sure?

Yep they are preexisting, #60827, #66578 and #68136.

@AndyAyersMS
Copy link
Member

All Fuzzlyn/Antigen issues are known.

@AndyAyersMS AndyAyersMS merged commit 8b6830a into dotnet:main Apr 25, 2022
@AndyAyersMS
Copy link
Member

@SingleAccretion, thanks again!

@SingleAccretion SingleAccretion deleted the Redundant-Branches-SideEffects branch April 26, 2022 15:45
@ghost ghost locked as resolved and limited conversation to collaborators May 26, 2022
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.

4 participants