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

Support hot cold splitting in crossgen2 #74963

Merged
merged 2 commits into from
Oct 28, 2022

Conversation

cshung
Copy link
Member

@cshung cshung commented Sep 1, 2022

This is a PR created by merging the code from the feature branch on runtimelab.

@amanasifkhalid
@EugenioPena
@dotnet/crossgen-contrib

@cshung cshung added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) area-crossgen2-coreclr labels Sep 1, 2022
@cshung cshung self-assigned this Sep 1, 2022
@cshung cshung force-pushed the public/hot-cold-splitting-crossgen2 branch from 52ed3c2 to 05be8ec Compare September 3, 2022 03:35
@cshung cshung requested a review from trylek September 15, 2022 17:32
@cshung cshung force-pushed the public/hot-cold-splitting-crossgen2 branch from e186ef0 to bdbab66 Compare September 20, 2022 19:16
@cshung cshung force-pushed the public/hot-cold-splitting-crossgen2 branch 2 times, most recently from 4ba78a9 to 324b616 Compare October 12, 2022 23:21
src/coreclr/vm/codeman.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/codeman.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/codeman.cpp Outdated Show resolved Hide resolved
Copy link
Member

@trylek trylek left a comment

Choose a reason for hiding this comment

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

Thanks Andrew, I believe you, Aman and Eugenio have done a great job, overall this looks great to me, I haven't noticed any blocking issues, I have shared a couple of suggestions for local code cleanups and deduplications and I have a few questions regarding bits of code I have a hard time to comprehend but that's about it.

@trylek
Copy link
Member

trylek commented Oct 19, 2022

Please also note that there are some conflicts you'll need to resolve. For lab testing, I suppose we should probably tweak the Crossgen2 pipelines to use the hot-cold code splitting and run the crossgen2 outerloop pipeline with this change.

@trylek
Copy link
Member

trylek commented Oct 19, 2022

I should probably also emphasize that I mostly focused on Crossgen2 and runtime parts of the change, while I think I've been able to mostly follow the intent of the JIT changes, these definitely merit reviewing by a better JIT expert than me.

@cshung cshung force-pushed the public/hot-cold-splitting-crossgen2 branch from 324b616 to 7598273 Compare October 19, 2022 22:02
Copy link
Member

@davidwrighton davidwrighton left a comment

Choose a reason for hiding this comment

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

This generally looks good. I'm looking forward to the tweaks from in response to code review.

cshung and others added 2 commits October 20, 2022 21:14
Co-authored-by: Aman Khalid <58230338+amanasifkhalid@users.noreply.github.com>
Co-authored-by: Eugenio Peña García <70240915+EugenioPena@users.noreply.github.com>
@cshung cshung force-pushed the public/hot-cold-splitting-crossgen2 branch from 3a5aed2 to f17547b Compare October 21, 2022 04:14
@cshung cshung changed the title [WIP] Support hot cold splitting in crossgen2 Support hot cold splitting in crossgen2 Oct 21, 2022
@cshung cshung removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Oct 21, 2022
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.

JIT changes LGTM

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants