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

Move dead code removal in lower after last updategraph #69421

Merged
merged 2 commits into from
May 21, 2022

Conversation

kunalspathak
Copy link
Member

We perform "dead code removal" pass just before "gc poll insertion", but we should do it just after the last updateFlowGraph() happens which is inside lower. This PR moves the phase into lower eliminating some other dead blocks that were left behind because of which LSRA adds resolution moves in them.

image

See #69041 (comment) and #69041 (comment) for details.

@ghost ghost assigned kunalspathak May 17, 2022
@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 May 17, 2022
@ghost
Copy link

ghost commented May 17, 2022

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

Issue Details

We perform "dead code removal" pass just before "gc poll insertion", but we should do it just after the last updateFlowGraph() happens which is inside lower. This PR moves the phase into lower eliminating some other dead blocks that were left behind because of which LSRA adds resolution moves in them.

image

See #69041 (comment) and #69041 (comment) for details.

Author: kunalspathak
Assignees: kunalspathak
Labels:

area-CodeGen-coreclr

Milestone: -

@kunalspathak
Copy link
Member Author

There are few regressions in minopts because earlier we would always do dead code removal, but with this change, we would do it only if optimizing.

@kunalspathak
Copy link
Member Author

@dotnet/jit-contrib , @AndyAyersMS - any thoughts?

@kunalspathak kunalspathak marked this pull request as ready for review May 17, 2022 17:41
@Wraith2
Copy link
Contributor

Wraith2 commented May 17, 2022

There are few regressions in minopts because earlier we would always do dead code removal, but with this change, we would do it only if optimizing.

Is there any reason it shouldn't done be in minopts as well?

@kunalspathak
Copy link
Member Author

Is there any reason it shouldn't done be in minopts as well?

Not really. I think I should do it regardless, just like we do today.

@Wraith2
Copy link
Contributor

Wraith2 commented May 17, 2022

I agree. I don't see removing dead code as an optimization.

@@ -6393,12 +6393,19 @@ PhaseStatus Lowering::DoPhase()
{
comp->optLoopsMarked = false;
bool modified = comp->fgUpdateFlowGraph();
modified |= comp->fgRemoveDeadBlocks();
Copy link
Member

@jakobbotsch jakobbotsch May 17, 2022

Choose a reason for hiding this comment

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

If fgUpdateFlowGraph returned false, do we need to run this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. This should be run regardless (as we do today). I want to do it after update flow graph because it might make more blocks eligible for removing.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense.

@kunalspathak
Copy link
Member Author

@dotnet/jit-contrib - can someone sign off so we can unblock the verification of #69041 (comment)?

@kunalspathak kunalspathak merged commit c190f6b into dotnet:main May 21, 2022
@kunalspathak kunalspathak deleted the removeblocks_lower branch May 21, 2022 15:14
@AndyAyersMS
Copy link
Member

AndyAyersMS commented May 27, 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