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

Move the NoGC helper classification to HelperCallProperties #105040

Merged
merged 1 commit into from
Jul 18, 2024

Conversation

SingleAccretion
Copy link
Contributor

@SingleAccretion SingleAccretion commented Jul 17, 2024

Fixes a TODO and centralizes the assumptions about helpers in one place.

(Also, makes things simpler downstream.)

No diffs.

Fixes a TODO and centralizes the assumptions about helpers in one place.
@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 Jul 17, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jul 17, 2024
Copy link
Contributor

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

@SingleAccretion SingleAccretion marked this pull request as ready for review July 17, 2024 21:14
@SingleAccretion
Copy link
Contributor Author

@dotnet/jit-contrib


switch (helper)
{
// Arithmetic helpers that cannot throw
case CORINFO_HELP_LLSH:
case CORINFO_HELP_LRSH:
case CORINFO_HELP_LRSZ:
isNoGC = true;
FALLTHROUGH;
case CORINFO_HELP_LMUL:
case CORINFO_HELP_LNG2DBL:
case CORINFO_HELP_ULNG2DBL:
Copy link
Member

Choose a reason for hiding this comment

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

just randomly checked CORINFO_HELP_ULNG2DBL and it also looks nogc, but I presume your PR just cleans things up

Copy link
Contributor Author

@SingleAccretion SingleAccretion Jul 17, 2024

Choose a reason for hiding this comment

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

Right, this is intentionally meant to be no-diff. Additionally, these no-gc annotations are a bit scary to maintain: they must match between CoreCLR and NAOT, and only CoreCLR is tested with GCStress.

Copy link
Member

@EgorBo EgorBo left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@EgorBo EgorBo merged commit 0785303 into dotnet:main Jul 18, 2024
107 checks passed
@SingleAccretion SingleAccretion deleted the NoGC branch July 21, 2024 09:43
@github-actions github-actions bot locked and limited conversation to collaborators Aug 21, 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 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