-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Conversation
Fixes a TODO and centralizes the assumptions about helpers in one place.
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
@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: |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Fixes a TODO and centralizes the assumptions about helpers in one place.
(Also, makes things simpler downstream.)
No diffs.