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

Introduce BBJ_CALLFINALLYRET block kind #95945

Merged
merged 2 commits into from
Dec 15, 2023

Conversation

BruceForstall
Copy link
Member

@BruceForstall BruceForstall commented Dec 13, 2023

Non-exceptional calls to finally blocks are represented in the JIT IR
as a pair of blocks:

  1. BBJ_CALLFINALLY targets the finally block
  2. BBJ_ALWAYS targets the finally "continuation", namely, the location
    where execution should continue after calling the finally. It is different
    from normal BBJ_ALWAYS, as it must be empty of code, must be the bbNext
    successor of the BBJ_CALLFINALLY, and must have the
    BBF_KEEP_BBJ_ALWAYS flag.

If the JIT knows the finally never returns (e.g., it unconditionally
throws), the paired BBJ_ALWAYS will not exist, and the BBJ_CALLFINALLY
will have the BBF_RETLESS_CALL flag.

This structure is unique in the JIT IR and leads to a lot of special
case code to maintain the pair relationship.

This PR makes a small change to the IR by introducing a new BBJ_CALLFINALLYRET
block type to be used instead of BBJ_ALWAYS when paired with a BBJ_CALLFINALLY.
The idea is to reduce overloading the semantics of BBJ_ALWAYS in this case,
so code that checks for BBJ_ALWAYS can do so knowing it always means the same
thing, without checking whether it is one of the special pair.

There one remaining "special" BBJ_ALWAYS: the "final step" block of an x86
callfinally. It is the only block that still uses the BBF_KEEP_BBJ_ALWAYS flag.

This change has a very few diffs where a slight difference in the order of
block removal (especially, in fgRemoveEmptyTry) leads to different
downstream optimizations.

Currently, this is implemented by running a pass to convert paired BBJ_ALWAYS
to BBJ_CALLFINALLYRET after the importer (namely, after importing LEAVE).
I'll go back and fix the LEAVE implementation if this is merged, to generate
the new IR form directly in the importer, and remove a few TODO-Quirk left
as a result (e.g., in fgFindInsertPoint, BBPredsChecker::CheckEhTryDsc,
fgComputeMissingBlockWeights).

The benefit of this change is the IR is more explicit; asserts can be better;
and there should be less confusion about BBJ_ALWAYS cases "accidentally"
kicking in for callfinally pairs.

Addresses #95355

@BruceForstall BruceForstall added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it labels Dec 13, 2023
@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 Dec 13, 2023
@ghost ghost assigned BruceForstall Dec 13, 2023
@ghost
Copy link

ghost commented Dec 13, 2023

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

Issue Details

null

Author: BruceForstall
Assignees: -
Labels:

NO-MERGE, NO-REVIEW, area-CodeGen-coreclr

Milestone: -

Non-exceptional calls to finally blocks are represented in the JIT IR
as a pair of blocks:
1. `BBJ_CALLFINALLY` targets the finally block
2. `BBJ_ALWAYS` targets the finally "continuation", namely, the location
where execution should continue after calling the finally. It is different
from normal `BBJ_ALWAYS`, as it must be empty of code, must be the `bbNext`
successor of the `BBJ_CALLFINALLY`, and must have the
`BBF_KEEP_BBJ_ALWAYS` flag.

If the JIT knows the finally never returns (e.g., it unconditionally
throws), the paired `BBJ_ALWAYS` will not exist, and the `BBJ_CALLFINALLY`
will have the `BBF_RETLESS_CALL` flag.

This structure is unique in the JIT IR and leads to a lot of special
case code to maintain the pair relationship.

This PR makes a small change to the IR by introducing a new `BBJ_CALLFINALLYRET`
block type to be used instead of `BBJ_ALWAYS` when paired with a `BBJ_CALLFINALLY`.
The idea is to reduce overloading the semantics of `BBJ_ALWAYS` in this case,
so code that checks for `BBJ_ALWAYS` can do so knowing it always means the same
thing, without checking whether it is one of the special pair.

There one remaining "special" `BBJ_ALWAYS`: the "final step" block of an x86
callfinally. It is the only block that still uses the `BBF_KEEP_BBJ_ALWAYS` flag.

This change has a very few diffs where a slight difference in the order of
block removal (especially, in `fgRemoveEmptyTry`) leads to different
downstream optimizations.

Currently, this is implemented by running a pass to convert paired `BBJ_ALWAYS`
to `BBJ_CALLFINALLYRET` after the importer (namely, after importing `LEAVE`).
I'll go back and fix the LEAVE implementation if this is merged, to generate
the new IR form directly in the importer, and remove a few `TODO-Quirk` left
as a result (e.g., in `fgFindInsertPoint`, `BBPredsChecker::CheckEhTryDsc`,
`fgComputeMissingBlockWeights`).

The benefit of this change is the IR is more explicit; asserts can be better;
and there should be less confusion about `BBJ_ALWAYS` cases "accidentally"
kicking in for callfinally pairs.
@BruceForstall
Copy link
Member Author

/azp run runtime-coreclr outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@BruceForstall BruceForstall changed the title [WIP] Introduce CALLFINALLYRET Introduce BBJ_CALLFINALLYRET Dec 15, 2023
@BruceForstall BruceForstall changed the title Introduce BBJ_CALLFINALLYRET Introduce BBJ_CALLFINALLYRET block kind Dec 15, 2023
@BruceForstall BruceForstall marked this pull request as ready for review December 15, 2023 00:26
@BruceForstall
Copy link
Member Author

@AndyAyersMS PTAL
cc @dotnet/jit-contrib

@AndyAyersMS AndyAyersMS removed the NO-REVIEW Experimental/testing PR, do NOT review it label Dec 15, 2023
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.

Just saw a few small things, feel free to defer addressing these.


JITDUMP("Converting BBF_DONT_REMOVE block " FMT_BB " to BBJ_THROW\n", block->bbNum);

// If the CALLFINALLY is being replace by a throw, then the CALLFINALLYRET is unreachable.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// If the CALLFINALLY is being replace by a throw, then the CALLFINALLYRET is unreachable.
// If the CALLFINALLY is being replaced by a throw, then the CALLFINALLYRET is unreachable.


currentBlock->SetKindAndTarget(BBJ_ALWAYS, postTryFinallyBlock);
currentBlock->RemoveFlags(BBF_RETLESS_CALL); // no longer a BBJ_CALLFINALLY
Copy link
Member

Choose a reason for hiding this comment

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

This is harmless, but if we have an empty finally I would expect the callfinally is not BBF_RETLESS_CALL, otherwise something is messed up.

Copy link
Member Author

Choose a reason for hiding this comment

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

What happens is the code above to remove the leaveBlock adds the retless flag, so here we just remove it.

@BruceForstall
Copy link
Member Author

Diffs. Very small asmdiffs, but there is a TP impact up to +0.10% -- not surprising, given some extra conditionals.

@BruceForstall BruceForstall merged commit 8725a6e into dotnet:main Dec 15, 2023
6 checks passed
@BruceForstall BruceForstall deleted the IntroduceCALLFINALLYRET branch December 15, 2023 18:22
@github-actions github-actions bot locked and limited conversation to collaborators Jan 15, 2024
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 NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants