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

Remove BBF_TRY_BEG #93367

Merged
merged 3 commits into from
Oct 13, 2023
Merged

Remove BBF_TRY_BEG #93367

merged 3 commits into from
Oct 13, 2023

Conversation

SingleAccretion
Copy link
Contributor

The flag duplicates information already present in the EH table.

Also delete some (not all) remnants of verification from impVerifyEHBlock.

@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 Oct 11, 2023
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Oct 11, 2023
@ghost
Copy link

ghost commented Oct 11, 2023

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

Issue Details

The flag duplicates information already present in the EH table.

Also delete some (not all) remnants of verification from impVerifyEHBlock.

Author: SingleAccretion
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@SingleAccretion SingleAccretion marked this pull request as ready for review October 11, 2023 23:13
@SingleAccretion SingleAccretion marked this pull request as draft October 12, 2023 09:13
@SingleAccretion SingleAccretion force-pushed the TryBegFlag branch 2 times, most recently from 5ddc9ee to a6011c1 Compare October 12, 2023 10:41
The flag duplicates information already present in the EH table.

Also delete some remnants of verification from "impVerifyEHBlock".
@SingleAccretion
Copy link
Contributor Author

Failures look like #93404.

No diffs, but a minor TP win, @dotnet/jit-contrib .

Copy link
Member

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

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

That's a nice cleanup. A couple notes.

As a follow-up: do you have any thoughts on #82336?

@@ -3499,12 +3498,6 @@ void Compiler::fgVerifyHandlerTab()
#endif // FEATURE_EH_FUNCLETS
}

// Only the first block of 'try' regions should have BBF_TRY_BEG set.
if (!blockTryBegSet[block->bbNum])
Copy link
Member

Choose a reason for hiding this comment

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

The only use of blockTryBegSet currently is in this check. You could remove it entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted.

/* Remove unreachable or empty blocks - do not consider blocks marked BBF_DONT_REMOVE or genReturnBB block
* These include first and last block of a TRY, exception handlers and RANGE_CHECK_FAIL THROW blocks */

if ((block->bbFlags & BBF_DONT_REMOVE) == BBF_DONT_REMOVE || block == genReturnBB)
Copy link
Member

Choose a reason for hiding this comment

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

Is the genReturnBB check just unnecessary? (obsolete?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unnecessary; the block is already always marked with BBF_DONT_REMOVE.

@SingleAccretion
Copy link
Contributor Author

As a follow-up: do you have any thoughts on #82336?

Great question :). I am looking at the EH-related flowgraph things and in particular the ability to remove dead protected regions and handlers.

@BruceForstall
Copy link
Member

@SingleAccretion Can you fix the merge conflict?

fwiw, all the test failures are "known"

@SingleAccretion
Copy link
Contributor Author

@BruceForstall conflict fixed.

@BruceForstall BruceForstall merged commit 29678e2 into dotnet:main Oct 13, 2023
123 of 129 checks passed
@SingleAccretion SingleAccretion deleted the TryBegFlag branch October 13, 2023 16:59
@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.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants