Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Disqualify byref-like fields from ManagedSequential layout #25056

Closed

Conversation

MichalStrehovsky
Copy link
Member

Seems to do the trick to get packing ignored on ref structs.

Fixes #20794.

Seems to do the trick to get packing ignored on ref structs.

Fixes #20794.
@MichalStrehovsky
Copy link
Member Author

Cc @sergiy-k

@jkotas
Copy link
Member

jkotas commented Jun 10, 2019

Do we need a test?

@MichalStrehovsky
Copy link
Member Author

MichalStrehovsky commented Jun 11, 2019

I realized we also need to make sure these are not considered blittable for this to really work. It only worked for the test that was in the issue because bool happens to be not blittable.

@@ -174,7 +174,8 @@ do \
TypeHandle pNestedType = fsig.GetLastTypeHandleThrowing(ClassLoader::LoadTypes,
CLASS_LOAD_APPROXPARENTS,
TRUE);
if (pNestedType.GetMethodTable()->IsManagedSequential())
if (pNestedType.GetMethodTable()->IsManagedSequential() &&
!pNestedType.GetMethodTable()->IsByRefLike())
Copy link
Member

Choose a reason for hiding this comment

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

I believe this may disable struct promotion on Span as I believe that ContainsPointers() is false. See Compiler::StructPromotionHelper::CanPromoteStructType in the jit, in particular if (StructHasCustomLayout(typeFlags) && ((typeFlags & CORINFO_FLG_CONTAINS_GC_PTR) == 0))

StructHasCustomLayout is another name for !ManagedSequential

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.

🕐

@MichalStrehovsky
Copy link
Member Author

Okay, so I looked into this in more detail and this is actually also changing how we lay out Span<T>.

Running the standard sequential field layout algorithm on Span will reorder the fields and I'm seeing the Length field getting moved to offset 8.

I'll look if I can get the padding ignored while keeping byref-like types blittable/managedsequential (however wrong that sounds).

@maryamariyan
Copy link
Member

Thank you for your contribution. As announced in dotnet/coreclr#27549 this repository will be moving to dotnet/runtime on November 13. If you would like to continue working on this PR after this date, the easiest way to move the change to dotnet/runtime is:

  1. In your coreclr repository clone, create patch by running git format-patch origin
  2. In your runtime repository clone, apply the patch by running git apply --directory src/coreclr <path to the patch created in step 1>

@sergiy-k sergiy-k added the post-consolidation PRs which will be hand ported to dotnet/runtime label Nov 7, 2019
@maryamariyan
Copy link
Member

Thank you for your contribution. As announced in #27549 the dotnet/runtime repository will be used going forward for changes to this code base. Closing this PR as no more changes will be accepted into master for this repository. If you’d like to continue working on this change please move it to dotnet/runtime.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-TypeSystem post-consolidation PRs which will be hand ported to dotnet/runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bad GC info generated for structures that contain Span field and small packing
5 participants