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

JIT: Spill newarr into temp #110518

Merged
merged 5 commits into from
Dec 13, 2024
Merged

JIT: Spill newarr into temp #110518

merged 5 commits into from
Dec 13, 2024

Conversation

hez2010
Copy link
Contributor

@hez2010 hez2010 commented Dec 9, 2024

Extracted from #104906

Spilling newarr into temp and introduce a new block flag for it. This is the prerequisites of escape analysis for arrays.

Question: should we always spill newarr instead of only do this when optimizations are enabled?

/cc: @AndyAyersMS

@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 Dec 9, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Dec 9, 2024
Copy link
Contributor

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

@AndyAyersMS
Copy link
Member

There are quite a few diffs. Can you investigate?

Question: should we always spill newarr instead of only do this when optimizations are enabled?

I would follow the pattern we have for newobj, which always spills to a temp.

@hez2010
Copy link
Contributor Author

hez2010 commented Dec 9, 2024

I would follow the pattern we have for newobj, which always spills to a temp.

This will lead to huge diffs for tier-0 so I will do this later after I investigated the diff.

@AndyAyersMS
Copy link
Member

Latest diffs look pretty good. Seems like if we're going to allocate and store an array into a static it is generally better to do the array creation before fetching the static's address.

Can you look at what happens if we also do this when not optimizing?

@hez2010
Copy link
Contributor Author

hez2010 commented Dec 11, 2024

Diffs

@AndyAyersMS
Copy link
Member

AndyAyersMS commented Dec 11, 2024

Diffs

Ah, ok. Let's undo that last commit and just do this when optimizing.

Suggests we should probably look into doing the same thing for newobj, or never split up front, and instead search and split in the object allocation code.

@MichalPetryka
Copy link
Contributor

Suggests we should probably look into doing the same thing for newobj, or never split up front, and instead search and split in the object allocation code.

Is a temp not always needed for newobj in order to call the ctor?

@AndyAyersMS
Copy link
Member

Suggests we should probably look into doing the same thing for newobj, or never split up front, and instead search and split in the object allocation code.

Is a temp not always needed for newobj in order to call the ctor?

It is today, but you could imagine arranging things so that the ctor returns the object, perhaps.

@hez2010
Copy link
Contributor Author

hez2010 commented Dec 12, 2024

Ah, ok. Let's undo that last commit and just do this when optimizing.

Done.

Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

Thanks!

@AndyAyersMS
Copy link
Member

I think the x86 failure was fixed, going to merge and see if we can get a green CI.

@AndyAyersMS AndyAyersMS merged commit f6af0b8 into dotnet:main Dec 13, 2024
108 checks passed
hez2010 added a commit to hez2010/runtime that referenced this pull request Dec 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.

3 participants