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.

Sign up for GitHub

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

Jump to bottom

Flow graph successors representation for BBJ_EHFINALLYRET #84278

Closed
BruceForstall opened this issue Apr 4, 2023 · 2 comments · Fixed by #93377
Closed

Flow graph successors representation for BBJ_EHFINALLYRET #84278

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

Comments

@BruceForstall
Copy link
Member

The try of a try / finally clause can contain a set of leave IL specifying to where the code should continue after the finally is invoked. The JIT transforms this into IR like:

// try
 ... code ...
BBJ_CALLFINALLY / bbJumpDest = the finally funclet
BBJ_ALWAYS / bbJumpDest = the "continuation" address specified by the `leave`
...
// finally
 ... code ...
BBJ_EHFINALLYRET

Note that there can be multiple leave IL that require invoking the same finally, and thus multiple BBJ_CALLFINALLY targeting the same finally. Each of the corresponding BBJ_ALWAYS (continuation) targets can be different, however.

Note also that if the finally is known to never return (due to an unconditional throw or infinite loop), the BBJ_CALLFINALLY will not have a BBJ_ALWAYS and the finally will not have a BBJ_EHFINALLYRET.

The "flowgraph successors" of the BBJ_EHFINALLYRET block are all the BBJ_ALWAYS blocks corresponding to the BBJ_CALLFINALLY blocks that target the finally.

Currently, the JIT does not directly represent the BasicBlock successors of a BBJ_EHFINALLYRET block. Finding those successors, as is done with the versions of GetSucc/NumSucc that take a Compiler*, is very expensive.

Can we directly represent the BBJ_EHFINALLYRET successors in the IR? E.g., as an array of BasicBlock*, the same way BBJ_SWITCH successors are implemented?

This would make forward GetSucc/NumSucc flowgraph traversals much cheaper.

Note that the IR is asymmetric here: the BBJ_ALWAYS pair of BBJ_CALLFINALLY has the BBJ_EHFINALLYRET in its predecessor list.

@AndyAyersMS @dotnet/jit-contrib Comments?
@BruceForstall BruceForstall added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Apr 4, 2023
@BruceForstall BruceForstall added this to the 8.0.0 milestone Apr 4, 2023
@BruceForstall BruceForstall self-assigned this Apr 4, 2023
@ghost
Copy link

ghost commented Apr 4, 2023

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

Issue Details

The try of a try / finally clause can contain a set of leave IL specifying to where the code should continue after the finally is invoked. The JIT transforms this into IR like:

// try
 ... code ...
BBJ_CALLFINALLY / bbJumpDest = the finally funclet
BBJ_ALWAYS / bbJumpDest = the "continuation" address specified by the `leave`
...
// finally
 ... code ...
BBJ_EHFINALLYRET

Note that there can be multiple leave IL that require invoking the same finally, and thus multiple BBJ_CALLFINALLY targeting the same finally. Each of the corresponding BBJ_ALWAYS (continuation) targets can be different, however.

Note also that if the finally is known to never return (due to an unconditional throw or infinite loop), the BBJ_CALLFINALLY will not have a BBJ_ALWAYS and the finally will not have a BBJ_EHFINALLYRET.

The "flowgraph successors" of the BBJ_EHFINALLYRET block are all the BBJ_ALWAYS blocks corresponding to the BBJ_CALLFINALLY blocks that target the finally.

Currently, the JIT does not directly represent the BasicBlock successors of a BBJ_EHFINALLYRET block. Finding those successors, as is done with the versions of GetSucc/NumSucc that take a Compiler*, is very expensive.

Can we directly represent the BBJ_EHFINALLYRET successors in the IR? E.g., as an array of BasicBlock*, the same way BBJ_SWITCH successors are implemented?

This would make forward GetSucc/NumSucc flowgraph traversals much cheaper.

Note that the IR is asymmetric here: the BBJ_ALWAYS pair of BBJ_CALLFINALLY has the BBJ_EHFINALLYRET in its predecessor list.

@AndyAyersMS @dotnet/jit-contrib Comments?

Author: BruceForstall
Assignees: BruceForstall
Labels:

area-CodeGen-coreclr
Milestone: 8.0.0

@BruceForstall
Copy link
Member Author

Related: #84307 -- "Consider adding BBJ_EHFAULTRET"

@BruceForstall BruceForstall modified the milestones: 8.0.0, Future Jun 22, 2023
@BruceForstall BruceForstall modified the milestones: Future, 9.0.0 Oct 10, 2023
BruceForstall added a commit to BruceForstall/runtime that referenced this issue Oct 12, 2023
@BruceForstall
Explicitly represent BBJ_EHFINALLYRET successors
e039bd6 
Currently, BBJ_EHFINALLYRET blocks have no explicit successors in the IR.
To implement successor iteration, a very expensive process is followed to
(1) find the region of blocks where a BBJ_CALLFINALLY block calling the
`finally` might be found, (2) search the region for such blocks, and (3)
return as a successor all the BBJ_ALWAYS blocks in the corresponding
BBJ_CALLFINALLY/BBJ_ALWAYS pair.

Change the IR to explicitly represent and maintain this list of successors
for BBJ_EHFINALLYRET blocks. The representation is a simple array of
`BasicBlock*`, similar to how BBJ_SWITCH block targets are represented.

Fixes dotnet#84278

Notes:
1. The BBJ_EHFINALLYRET successors are computed in `impFixPredLists()`.
There are various dumpers that run before this, so we need to tolerate
incomplete successor information in some places.
2. `ehGetCallFinallyBlockRange()` is still used by some code. I changed the
semantics to return a `[first..last]` range inclusive of `last` instead of
the previous `[beginning..end)` range exclusive of `end`. This makes it easier
to use with our BasicBlock iterators.
@BruceForstall BruceForstall mentioned this issue Oct 12, 2023
Merged
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Oct 12, 2023
BruceForstall added a commit that referenced this issue Oct 13, 2023
@BruceForstall
Explicitly represent BBJ_EHFINALLYRET successors (#93377)
851c274 
* Explicitly represent BBJ_EHFINALLYRET successors

Currently, BBJ_EHFINALLYRET blocks have no explicit successors in the IR.
To implement successor iteration, a very expensive process is followed to
(1) find the region of blocks where a BBJ_CALLFINALLY block calling the
`finally` might be found, (2) search the region for such blocks, and (3)
return as a successor all the BBJ_ALWAYS blocks in the corresponding
BBJ_CALLFINALLY/BBJ_ALWAYS pair.

Change the IR to explicitly represent and maintain this list of successors
for BBJ_EHFINALLYRET blocks. The representation is a simple array of
`BasicBlock*`, similar to how BBJ_SWITCH block targets are represented.

Fixes #84278

Notes:
1. The BBJ_EHFINALLYRET successors are computed in `impFixPredLists()`.
There are various dumpers that run before this, so we need to tolerate
incomplete successor information in some places.
2. `ehGetCallFinallyBlockRange()` is still used by some code. I changed the
semantics to return a `[first..last]` range inclusive of `last` instead of
the previous `[beginning..end)` range exclusive of `end`. This makes it easier
to use with our BasicBlock iterators.

* Review suggestions
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Oct 13, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Nov 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@BruceForstall BruceForstall

Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Milestone
9.0.0
Development

Successfully merging a pull request may close this issue.

Explicitly represent BBJ_EHFINALLYRET successors BruceForstall/runtime
1 participant
@BruceForstall