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 GenTreeDebugOperKind #64498

Merged
merged 6 commits into from
Feb 11, 2022
Merged

Conversation

SingleAccretion
Copy link
Contributor

@SingleAccretion SingleAccretion commented Jan 29, 2022

Recently, I've compressed all "oper kinds" into an unsigned char, however, that meant all 8 bits were taken, while not all of the kinds are actually needed in Release builds. This change fixes that by introducing another, DEBUG-only, type that serves the same purpose - GenTreeDebugOperKind and frees 2 bits (GTK_EXOP requires a bit more work) in the process. It also introduces a new "kind" for opers that should not appear in HIR - DBK_NOTHIR, counterpart to DBK_NOTLIR.

It also cleans up gtlist.h a little by getting rid of redundant parenthesis and moving opers around to more logical places. This speeds up OperIsIndir by a good margin.

No diffs.

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

ghost commented Jan 29, 2022

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

Issue Details

Recently, I've compressed all "oper kinds" into an unsigned char, however, that meant all 8 bits were taken, while not all of the kinds are actually needed in Release builds. This change fixes that by introducing another, DEBUG-only, type that serves the same purpose - GenTreeDebugOperKind and frees 2 bits (GTK_EXOP requires a bit more work) in the process. It also introduces a new "kind" for opers that should not appear in HIR - DBK_NOTHIR, counterpart to DBK_NOTLIR.

It also cleans up gtlist.h a little by getting rid of redundant parenthesis and moving opers around to more logical places. This speeds up OperIsIndir by a good margin.

Diffs are not expected.

Author: SingleAccretion
Assignees: -
Labels:

area-CodeGen-coreclr, community-contribution

Milestone: -

// making the table for those larger in Release builds. However, it resides in the same
// "namespace" and so all values here must be distinct from those in "GenTreeOperKind".
//
enum GenTreeDebugOperKind
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would have been possible to make the values here part of GenTreeOperKind, but I think having a separate enum is clearer overall.

{
DBK_FIRST_FLAG = GTK_MASK + 1,

DBK_NOTHIR = DBK_FIRST_FLAG, // This oper is not supported in HIR (before rationalization).
Copy link
Contributor Author

@SingleAccretion SingleAccretion Jan 29, 2022

Choose a reason for hiding this comment

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

We also have "early HIR" in the compiler, i. e. HIR before morph: GT_FIELD, GT_PUTARG_TYPE, GT_RET_EXPR, GT_RUNTIMELOOKUP, GT_CNS_STR, GT_FTN_ADDR, GT_INDEX, so something like DBK_EHIR can now be easily added, if people feel like it is valuable (I personally do not see a lot of value).

@SingleAccretion SingleAccretion marked this pull request as ready for review January 30, 2022 00:14
@SingleAccretion
Copy link
Contributor Author

@dotnet/jit-contrib

To track invariants related to opers in asserts
without increasing the size of the primary oper
kind table.

Some shuffling of the oper table to make it look better.
Put all OperIsIdir opers together, fix up formatting,
move opers around to more logical places.
There is not a lot of point in this being a "release"
oper kind, as it is really only useful for debug checks.
@AndyAyersMS
Copy link
Member

Looks good to me, but want to give the rest @dotnet/jit-contrib one last chance to weigh in...

@AndyAyersMS AndyAyersMS merged commit 5422e1f into dotnet:main Feb 11, 2022
@SingleAccretion SingleAccretion deleted the Not-HIR branch February 11, 2022 18:11
@ghost ghost locked as resolved and limited conversation to collaborators Mar 13, 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 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