Skip to content

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

Merged
AndyAyersMS merged 1 commit into
dotnet:mainfrom
SingleAccretion:Redundant-Branches-SideEffects
Apr 25, 2022
Merged

Improve the redundant branch optimization to handle more side effects#68447
AndyAyersMS merged 1 commit into
dotnet:mainfrom
SingleAccretion:Redundant-Branches-SideEffects

Conversation

@SingleAccretion

@SingleAccretion SingleAccretion commented Apr 23, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 community-contribution Indicates that the PR has been added by a community member 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

ghost commented Apr 23, 2022

Copy link
Copy Markdown

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

SingleAccretion commented Apr 24, 2022

Copy link
Copy Markdown
Contributor Author

@dotnet/jit-contrib
@jakobbotsch

jakobbotsch commented Apr 24, 2022

Copy link
Copy Markdown
Member

/azp run Antigen, Fuzzlyn
@azure-pipelines

azure-pipelines Bot commented Apr 24, 2022

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).
AndyAyersMS
AndyAyersMS approved these changes Apr 25, 2022
View reviewed changes

@AndyAyersMS AndyAyersMS left a comment

Copy link
Copy Markdown
Member

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.
AndyAyersMS
AndyAyersMS reviewed Apr 25, 2022
View reviewed changes
Comment thread src/coreclr/jit/redundantbranchopts.cpp
}

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

@AndyAyersMS AndyAyersMS Apr 25, 2022

Copy link
Copy Markdown
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

kunalspathak commented Apr 25, 2022

Copy link
Copy Markdown
Contributor

Didn't look through the Antigen ones.

Antigen failures are also unrelated.
@AndyAyersMS

AndyAyersMS commented Apr 25, 2022

Copy link
Copy Markdown
Member

I suspect the Fuzzlyn failures are all unrelated

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

jakobbotsch commented Apr 25, 2022

Copy link
Copy Markdown
Member

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

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

AndyAyersMS commented Apr 25, 2022

Copy link
Copy Markdown
Member

All Fuzzlyn/Antigen issues are known.
@AndyAyersMS
AndyAyersMS merged commit 8b6830a into dotnet:main Apr 25, 2022
@AndyAyersMS

AndyAyersMS commented Apr 25, 2022

Copy link
Copy Markdown
Member

@SingleAccretion, thanks again!
@SingleAccretion
SingleAccretion deleted the Redundant-Branches-SideEffects branch April 26, 2022 15:45
@AndyAyersMS AndyAyersMS mentioned this pull request Apr 29, 2022
Open
@DrewScoggins DrewScoggins mentioned this pull request May 3, 2022
Closed
@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

4 participants

@SingleAccretion @jakobbotsch @kunalspathak @AndyAyersMS