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: initial support for stack allocating arrays of GC type #112250

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AndyAyersMS
Copy link
Member

Add the ability to create a ClassLayout for arrays, and use this to unblock stack allocation for arrays with GC types.

Replaces #111686.

Add the ability to create a ClassLayout for arrays, and use this to
unblock stack allocation for arrays with GC types.

Replaces dotnet#111686.
@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 Feb 6, 2025
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 Author

AndyAyersMS commented Feb 6, 2025

@jakobbotsch PTAL
cc @dotnet/jit-contrib @hez2010

This unblocks GC arrays of final types, but for GC arrays of non-final types many stores will use the covariant store check helper which currently causes the array to escape. So only a handful of diffs (and lots of context misses; I may go boost SPMI extra queries again before trying to merge this [edit: my local SPMI cache was stale]).

We need both a "only do the covariant store check" variant and a "do the covariant store check and then a checked write barrier" variant.

@AndyAyersMS AndyAyersMS mentioned this pull request Feb 6, 2025
}
else
{
elementSize = genTypeSize(type);
Copy link
Member

Choose a reason for hiding this comment

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

I suppose if we really wanted to we could provide a constructor for a class layout for primitive types, which could be used to unify paths like these.

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems like that might have been the original intention since we bias the indexes.

Copy link
Member

Choose a reason for hiding this comment

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

In any case we can just create them as custom layouts now, but I suppose it would be slightly less efficient than this approach.

@@ -828,7 +828,7 @@ unsigned int ObjectAllocator::MorphNewArrNodeIntoStackAlloc(GenTreeCall*
blockSize = AlignUp(blockSize, 8);
}

comp->lvaSetStruct(lclNum, comp->typGetBlkLayout(blockSize), /* unsafeValueClsCheck */ false);
comp->lvaSetStruct(lclNum, comp->typGetArrayLayout(clsHnd, length), /* unsafe */ false);
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to propagate the layout through instead of blockSize? We could probably save a call to getChildType.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I should get rid of the redundant layout fetch.

Copy link
Member Author

Choose a reason for hiding this comment

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

Only a small step from there to using layouts everywhere.

I think I should do that first (leaving GC arrays disabled) and verify zero diff, then come back to this and enable gc arrays.

Copy link
Member Author

Choose a reason for hiding this comment

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

After poking at it some, I think it might be simpler to take this change first and then revise the entire thing to be more layout centric (getting rid of the special box type, etc).

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds fine to me.

@AndyAyersMS AndyAyersMS marked this pull request as ready for review February 7, 2025 20:49
@AndyAyersMS
Copy link
Member Author

Diffs

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants