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: build pred lists early, persist them throughout compilation #80193

Closed
Tracked by #74873
AndyAyersMS opened this issue Jan 4, 2023 · 2 comments · Fixed by #81582
Closed
Tracked by #74873

JIT: build pred lists early, persist them throughout compilation #80193

AndyAyersMS opened this issue Jan 4, 2023 · 2 comments · Fixed by #81582
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Milestone

Comments

@AndyAyersMS
Copy link
Member

Currently there's a lag between incorporation of profile data (which happens quite early and uses its own flow edge representation) and building pred lists, and pred lists are sometimes dropped and recomputed from scratch (#49030).

We should be building pred lists early on and using those for profile incorporation, and then keep the lists valid and persistent throughout compilation.

@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 Jan 4, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jan 4, 2023
@AndyAyersMS AndyAyersMS added this to the 8.0.0 milestone Jan 4, 2023
@ghost
Copy link

ghost commented Jan 4, 2023

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

Issue Details

Currently there's a lag between incorporation of profile data (which happens quite early and uses its own flow edge representation) and building pred lists, and pred lists are sometimes dropped and recomputed from scratch (#49030).

We should be building pred lists early on and using those for profile incorporation, and then keep the lists valid and persistent throughout compilation.

Author: AndyAyersMS
Assignees: -
Labels:

area-CodeGen-coreclr, untriaged

Milestone: -

@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Jan 4, 2023
@AndyAyersMS AndyAyersMS self-assigned this Jan 4, 2023
@AndyAyersMS
Copy link
Member Author

AndyAyersMS commented Jan 4, 2023

Note this will likely happen incrementally -- moving the point where pred lists are built earlier and earlier and gradually fixing up the impacted areas of the jit.

Related work:

AndyAyersMS added a commit that referenced this issue Jan 6, 2023
Add finer-grained notions of what post-phase checks are enabled. Start checking
some IR and FG invariants earlier than we did before.

This is largely done in anticipation of moving pred list building earlier
(#80193). We should also consider moving some of the IR checking
earlier as well.
AndyAyersMS added a commit to AndyAyersMS/runtime that referenced this issue Jan 19, 2023
Fix the EH opts to properly maintain pred lists, and move building of the
pred lists to just before the EH opts.

Contributes to dotnet#80193.
AndyAyersMS added a commit that referenced this issue Jan 20, 2023
Fix the EH opts to properly maintain pred lists, and move building of the
pred lists to just before the EH opts.

Contributes to #80193.
AndyAyersMS added a commit to AndyAyersMS/runtime that referenced this issue Jan 20, 2023
Move the pred list building up a bit further.

Contributes to dotnet#80193.
AndyAyersMS added a commit that referenced this issue Jan 20, 2023
Move the pred list building up a bit further.

Contributes to #80193.
AndyAyersMS added a commit to AndyAyersMS/runtime that referenced this issue Jan 20, 2023
In anticipation of moving pred list building before inlining,
use the common block split utility instead of custom code. This does a
better job of propagating IL offsets, and automatically handles
pred list maintenance.

Contributes to dotnet#80193.
AndyAyersMS added a commit that referenced this issue Jan 20, 2023
In anticipation of moving pred list building before inlining,
use the common block split utility instead of custom code. This does a
better job of propagating IL offsets, and automatically handles
pred list maintenance.

Contributes to #80193.
AndyAyersMS added a commit to AndyAyersMS/runtime that referenced this issue Jan 22, 2023
Move pred list building up a bit further. Note that this impacts both the root
method and all inlinees, since we run a number of the initial phases on both.

Since inlinees build their basic blocks and flow edges in the root compiler's
memory pool, the only work required to unify the pred lists for a successful
inline is to get things right at the boundaries. And for failed inlines there
is no cross-referencing so we can just let the new pred lists leak away (like
we alredy do for the inlinee blocks).

Contributes towards dotnet#80193.
mdh1418 pushed a commit to mdh1418/runtime that referenced this issue Jan 24, 2023
Fix the EH opts to properly maintain pred lists, and move building of the
pred lists to just before the EH opts.

Contributes to dotnet#80193.
mdh1418 pushed a commit to mdh1418/runtime that referenced this issue Jan 24, 2023
Move the pred list building up a bit further.

Contributes to dotnet#80193.
mdh1418 pushed a commit to mdh1418/runtime that referenced this issue Jan 24, 2023
In anticipation of moving pred list building before inlining,
use the common block split utility instead of custom code. This does a
better job of propagating IL offsets, and automatically handles
pred list maintenance.

Contributes to dotnet#80193.
AndyAyersMS added a commit that referenced this issue Jan 24, 2023
Move pred list building up a bit further. Note that this impacts both the root
method and all inlinees, since we run a number of the initial phases on both.

Since inlinees build their basic blocks and flow edges in the root compiler's
memory pool, the only work required to unify the pred lists for a successful
inline is to get things right at the boundaries. And for failed inlines there
is no cross-referencing so we can just let the new pred lists leak away (like
we already do for the inlinee blocks).

Contributes towards #80193.
AndyAyersMS added a commit to AndyAyersMS/runtime that referenced this issue Jan 25, 2023
Move pred list building 3 phases earlier. It now happens just after
instrumentation (or importation if we're not instrumenting) and just before
expanding patchpoints.

Revise the patchpoint, indirect call, and post importer cleanup phases
to do proper pred list maintenance.

Update the flow checker to handle cases we see when we haven't yet run
the post importer cleanup.

Contributes to dotnet#80193.
AndyAyersMS added a commit that referenced this issue Jan 26, 2023
Move pred list building 3 phases earlier. It now happens just after
instrumentation (or importation if we're not instrumenting) and just before
expanding patchpoints.

Revise the patchpoint, indirect call, and post importer cleanup phases
to do proper pred list maintenance.

Update the flow checker to handle cases we see when we haven't yet run
the post importer cleanup.

Contributes to #80193.
AndyAyersMS added a commit to AndyAyersMS/runtime that referenced this issue Jan 26, 2023
This is used some early phases; make it pred list aware.

Contributes to dotnet#80193.
AndyAyersMS added a commit that referenced this issue Jan 27, 2023
This is used some early phases; make it pred list aware.

Contributes to #80193.
AndyAyersMS added a commit to AndyAyersMS/runtime that referenced this issue Jan 27, 2023
Move pred list building to just before instrumentation (and just after
importation -- we are getting very close to the front of the phase list now).

The block and edge count instrumenters were both using cheap preds to keep
track of some relocated count probes. Revise this so they can use the regular
pred lists. Also rework both approaches so their `RelocateProbes` methods are
fairly similar and perhaps could be unified one day.

Contributes to dotnet#80193.
AndyAyersMS added a commit that referenced this issue Jan 30, 2023
Move pred list building to just before instrumentation (and just after
importation -- we are getting very close to the front of the phase list now).

The block and edge count instrumenters were both using cheap preds to keep
track of some relocated count probes. Revise this so they can use the regular
pred lists. Also rework both approaches so their `RelocateProbes` methods are
fairly similar and perhaps could be unified one day.

Contributes to #80193.
AndyAyersMS added a commit to AndyAyersMS/runtime that referenced this issue Feb 1, 2023
Move pred list building before importation. It now runs as the first phase in
the phase list.

* Split up some unions in block.h as some things can't share storage anymore
(may revisit this later).
* Modify importer not to rely on cheap preds. Most of the work here is in
`impImportLeave`.
* Adjust OSR protection strategy for the method entry. In particular, watch
for the degenerate case where the OSR entry is the method entry.
* Ensure we don't double-decrement some ref counts when blocks with degenerate
or folded flow are re-imported.
* Update spill clique discovery to use the actual pred lists.
* Add new method `impFixPredLists` for the importer to use at the end of
importation. This adds pred list entries finally returns, which can't be
done until all `BBJ_LEAVE` blocks are processed.

Contributes to dotnet#80193.
AndyAyersMS added a commit that referenced this issue Feb 2, 2023
Move pred list building before importation. It now runs as the first phase in
the phase list.

* Split up some unions in block.h as some things can't share storage anymore
(may revisit this later).
* Modify importer not to rely on cheap preds. Most of the work here is in
`impImportLeave`.
* Adjust OSR protection strategy for the method entry. In particular, watch
for the degenerate case where the OSR entry is the method entry.
* Ensure we don't double-decrement some ref counts when blocks with degenerate
or folded flow are re-imported.
* Update spill clique discovery to use the actual pred lists.
* Add new method `impFixPredLists` for the importer to use at the end of
importation. This adds pred list entries finally returns, which can't be
done until all `BBJ_LEAVE` blocks are processed.
* trigger badcode inside `fgComputePreds`
* fix issue with `STRESS_CATCH_ARG`

Contributes to #80193.
AndyAyersMS added a commit to AndyAyersMS/runtime that referenced this issue Feb 2, 2023
This is the last in a (long) series. We now build the pred lists at the same
time we are initially connecting up the flow graph. Pred lists are now always
valid and need to be maintained by all phases.

There are some changes needed in EH normalization, and one special case we
need to handle in debug codegen where we create the scratch BB very early on.
This was the last client for the cheap pred lists.

Note some of the pred list info can't be added right away, in particular
the "return" edges from finallies do not appear until we've made it through
the importer.

I have deferred cleaning up dead code; will do it in follow-up changes.

Contributes to dotnet#80193.
AndyAyersMS added a commit that referenced this issue Feb 2, 2023
This is the last in a (long) series. We now build the pred lists at the same
time we are initially connecting up the flow graph. Pred lists are now always
valid and need to be maintained by all phases.

There are some changes needed in EH normalization, and one special case we
need to handle in debug codegen where we create the scratch BB very early on.
This was the last client for the cheap pred lists.

Note some of the pred list info can't be added right away, in particular
the "return" edges from finallies do not appear until we've made it through
the importer.

I have deferred cleaning up dead code; will do it in follow-up changes.

Contributes to #80193.
AndyAyersMS added a commit to AndyAyersMS/runtime that referenced this issue Feb 2, 2023
We no longer need cheap preds. We always have full preds available.

Also, remove the ability to drop and recompute full preds, as we now compute
full preds incrementally while building the flow graph, and never drop or need
to rebuild them.

Contributes to dotnet#80193.
AndyAyersMS added a commit that referenced this issue Feb 3, 2023
We no longer need cheap preds. We always have full preds available.

Also, remove the ability to drop and recompute full preds, as we now compute
full preds incrementally while building the flow graph, and never drop or need
to rebuild them.

Contributes to #80193.
AndyAyersMS added a commit to AndyAyersMS/runtime that referenced this issue Feb 3, 2023
Since pred lists now exist pervasively, change most methods that had
conditional pred list updates to assert preds exist and always update.

To make sure I got all uses, I renamed `fgComputePredsDone` to
`fgPredsComputed`.

Remove the ability to drop EH, as it doesn't update pred lists properly
and so has been broken for quite a while now.

Remove some of the logic for fixing up finally targets, since we now
always have pred lists around and so don't need the blanket invaliation.

Closes dotnet#80193.
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Feb 3, 2023
AndyAyersMS added a commit that referenced this issue Feb 3, 2023
Since pred lists now exist pervasively, change most methods that had
conditional pred list updates to assert preds exist and always update.

To make sure I got all uses, I renamed `fgComputePredsDone` to
`fgPredsComputed`.

Remove the ability to drop EH, as it doesn't update pred lists properly
and so has been broken for quite a while now.

Remove some of the logic for fixing up finally targets, since we now
always have pred lists around and so don't need the blanket invaliation.

Closes #80193.
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Feb 3, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Mar 5, 2023
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
Archived in project
Development

Successfully merging a pull request may close this issue.

1 participant