-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Keep this alive for classes with finalizers #103813
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
if (resultflags.HasInstructionSet(InstructionSet_AVX512CD_VL) && !resultflags.HasInstructionSet(InstructionSet_AVX512CD)) | ||
resultflags.RemoveInstructionSet(InstructionSet_AVX512CD_VL); | ||
if (resultflags.HasInstructionSet(InstructionSet_AVX512CD_VL) && !resultflags.HasInstructionSet(InstructionSet_AVX512F_VL)) | ||
resultflags.RemoveInstructionSet(InstructionSet_AVX512CD_VL); |
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.
I guess someone didn't use gen.bat
and modified this file by hands
if ((info.compCompHnd->getClassAttribs(info.compClassHnd) & CORINFO_FLG_HAS_FINALIZER) != 0) | ||
{ | ||
return true; | ||
} |
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.
Does this handle extending this
lifetime for inlined methods?
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.
Good question, it does not. E.g. for synchronized methods we simply never inline them, which is too conservative to do in this case. So the fix has to be done in the importer
Let's see how diffs look like, based on @jakobbotsch's suggestion #103522 (comment)