CoreCLR support for InlineArrayAttribute. (struct layout part) #82744

VSadov · 2023-02-27T23:38:05Z

Implements support for InlineArrayAtribute as in #61135
Re: #81145

port relevant changes from the prototype
make the layout replication based on an attribute
add the actual attribute
write tests
friendly loader errors
optimized layout for "all-objects" case - will be a separate follow up change.

src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs

src/libraries/System.Private.CoreLib/src/System/ParamsArray.cs

VSadov · 2023-03-04T00:47:05Z

CC: @mangod9 @AaronRobinsonMSFT

VSadov · 2023-03-04T00:47:43Z

I think the PR is ready to be reviewed.

mangod9 · 2023-03-04T00:50:16Z

@davidwrighton as well.

VSadov · 2023-03-04T00:53:09Z

src/coreclr/inc/corinfo.h

@@ -852,7 +852,7 @@ enum CorInfoFlag
    CORINFO_FLG_ARRAY                 = 0x00080000, // class is an array class (initialized differently)
    CORINFO_FLG_OVERLAPPING_FIELDS    = 0x00100000, // struct or class has fields that overlap (aka union)
    CORINFO_FLG_INTERFACE             = 0x00200000, // it is an interface
-    CORINFO_FLG_DONT_DIG_FIELDS       = 0x00400000, // don't ask field info, AOT can't rely on it (used for types outside of AOT compilation version bubble)
+    CORINFO_FLG_DONT_DIG_FIELDS       = 0x00400000, // don't ask field info (used for types outside of AOT compilation version bubble and value arrays)


This flag was called CORINFO_FLG_DONT_PROMOTE at the time of the prototype. We do not want inline arrays to promote as indexing relies on contiguous layout.

I assume the flag still has the effect of suppressing field promotion.

We should double check with the JIT team - I would think we should be setting similar flags that we set for explicit layout structs and explicit layout structs don't set this one.

@dotnet/jit-contrib can someone from the JIT team take a look at this part?

CORINFO_FLG_DONT_DIG_FIELDS doesn't always suppress promotion (eg a single gc ref field struct will still get promoted).

I would introduce a new flag describing this, something like CORINFO_FLG_INDEXABLE_FIELDS, and update the jit to block promotion when it sees it. Looks like there is still one free bit?

Down the road we might be doing more flexible partial promotion and perhaps even handle cases like these where we know exactly which fields are being touched over some span of code.

update the jit to block promotion when it sees it

Is there an easy way to find all the places that need to care about the bit?

@VSadov I do think we need a JIT-EE GUID update here -- otherwise it can expose all sorts of latent issues for the JIT team once we get newer collections with this flag that older JITs do not handle.

Makes sense. Thanks!! I will change the guid in a follow up change.

In theory the presence of the attribute will imply the new runtime and prevent many versioning issues, but it is better to be sure.

how hard would it be to begin consuming the support?

Fairly easy. Roslyn will add a layer of convenience, but otherwise the feature can be used directly.
I’ve added some use cases in corlib and libraries as a part of the change.

The only usual caveat with using a feature this early is possibly having to change things if the feature changes based on feedback.

@jakobbotsch Sorry, did not notice your yesterdays comment.

In theory the presence of the attribute will imply the new runtime and prevent many versioning issues, but it is better to be sure.

The problem for the JIT team is that we disambiguate compatibility of SPMI collections via the JIT-EE GUID. We automatically collect every Sunday, so after that we will start seeing contexts with the new flag set and no indication that newer JITs are needed to properly handle this. That might not cause any problems except silent bad codegen for our SPMI runs, but it could also conceivably cause spurious assertion failures.

FWIW, @EgorBo just merged #83430 which includes a JIT-EE GUID bump, so you won't need to do that in a follow-up change after all.

Fairly easy. Roslyn will add a layer of convenience, but otherwise the feature can be used directly.
I’ve added some use cases in corlib and libraries as a part of the change.

I've looked at some of the examples, that looks fairly straightforward. I guess the main thing is how to get access to InlineArrayAttribute from the programs I'm generating.

VSadov · 2023-03-04T00:58:02Z

src/coreclr/vm/methodtablebuilder.cpp

+                }
+                else
+                {
+                    // TODO: diagnostics, repeat must be > 0


Do we want a special diagnostic messages for this or IDS_CLASSLOAD_GENERAL is good enough?

The bar for type loader error messages has been:

Good error message if it is something you can hit by writing C#.

Generic error message is ok if it is something that can be only hit by writing IL.

oh, made a similar comment above. If there is some likelihood where customers might hit such cases a specific message would be helpful.

Good error message if it is something you can hit by writing C#.

Yes, these can be hit, so need specific messages.

Good error message if it is something you can hit by writing C#.

Yes, these can be hit, so need specific messages.

Isn't C# going to have syntax for this? If so, it's likely they're going to prevent people placing this attribute on random stuff like they do for IsByRefLikeAttribute. Would want to double check, but it's not an unreasonable request to make.

I think for now we should assume that the attribute is valid for direct use. It is not as dangerous as IsByRefLike and is relatively easy to use correctly. There could be some use scenarios that are not covered by C# features.

If eventually we see that it is better to disallow, we can do that and make errors less friendly.
I think for the time being, while C# features are in design/prototyping/development, nicer errors would be a good thing.

src/coreclr/vm/methodtablebuilder.cpp

jkotas · 2023-03-04T01:01:16Z

src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs

+                long size = instanceByteSizeAndAlignment.Size.AsInt;
+                size *= repeat;
+
+                // limit the max size of array instance to 1MiB


Add same limit to the coreclr typeloader as well?

src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs

jkotas · 2023-03-04T01:07:12Z

src/libraries/System.Private.CoreLib/src/System/Reflection/MethodBase.cs

+            private object? _arg1;
+            private object? _arg2;
+            private object? _arg3;


Suggested change

private object? _arg1;

private object? _arg2;

private object? _arg3;

private T _arg1;

private T _arg2;

private T _arg3;

?

VSadov · 2023-03-12T18:24:27Z

/azp run runtime-extra-platforms

azure-pipelines · 2023-03-12T18:24:49Z

Azure Pipelines successfully started running 1 pipeline(s).

MichalStrehovsky · 2023-03-13T00:48:57Z

src/coreclr/tools/Common/TypeSystem/Common/MetadataType.cs

@@ -2,6 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.

 using System.Collections.Generic;
+using System.Diagnostics;


Unused using?

MichalStrehovsky · 2023-03-13T00:55:47Z

src/coreclr/tools/Common/TypeSystem/Common/MetadataType.cs

+        /// <summary>
+        /// Gets a value indicating whether this is an inline array type
+        /// </summary>
+        public virtual bool IsInlineArray


This should not be virtual. Things that are computed from flags are determined from flags (and fully inlineable).

MichalStrehovsky · 2023-03-13T00:58:54Z

src/coreclr/tools/Common/TypeSystem/Interop/IL/InlineArrayType.cs

@@ -91,6 +91,13 @@ public override bool IsSequentialLayout
            }
        }

+        public override bool IsInlineArray => Length > 0;


Let's not convert this to an inline array at this time. I'm concerned we have very little test coverage for this, at least until we can run the src/tests tree. We're not far from being able to do that.

This type is only used in marshalled interop and currently the only test coverage for marshalled interop we have is a smoke test under src/test/nativeaot, because we were very efficient in removing all built-in marshalling use within libraries.

Right. If there is little coverage, there is not a lot of point to make this change right now.

MichalStrehovsky · 2023-03-13T01:01:46Z

src/coreclr/tools/Common/TypeSystem/CodeGen/TypeDesc.CodeGen.cs

@@ -25,5 +25,11 @@ public partial class InstantiatedType
            if (_typeDef.IsIntrinsic)
                flags |= TypeFlags.IsIntrinsic;
        }
+
+        partial void AddComputedInlineArrayFlag(ref TypeFlags flags)


Technically this should go to a new file TypeDesc.Metadata.cs, but maybe this is fine.

Technically this should go to a new file TypeDesc.Metadata.cs, but maybe this is fine.

I will not change this part then.
Let me know if you rethink this.

AndyAyersMS

Jit changes look good.

jkotas

LGTM!

VSadov · 2023-03-16T21:45:26Z

Thanks!!

ghost assigned VSadov Feb 27, 2023

dotnet-issue-labeler bot added the area-crossgen2-coreclr label Feb 27, 2023

VSadov added area-TypeSystem-coreclr and removed area-crossgen2-coreclr labels Feb 28, 2023

VSadov force-pushed the valArr branch 2 times, most recently from df38f95 to 6e8c11b Compare February 28, 2023 07:21

jkotas reviewed Feb 28, 2023

View reviewed changes

src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs Outdated Show resolved Hide resolved

jkotas reviewed Feb 28, 2023

View reviewed changes

src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs Outdated Show resolved Hide resolved

This was referenced Feb 28, 2023

Test failure: System.Security.Cryptography.X509Certificates.Tests.CertificateCreation.CertificateRequestChainTests/CreateChain_Hybrid #25979

Closed

System.Security.Cryptography.X509Certificates.Tests.ChainTests.BuildInvalidSignatureTwice failure #65448

Closed

build-analysis bot mentioned this pull request Feb 28, 2023

Tracking issue for CI build timeouts #76454

Closed

jkotas reviewed Mar 1, 2023

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/ParamsArray.cs Show resolved Hide resolved

VSadov force-pushed the valArr branch from 553565a to 49ac2ea Compare March 1, 2023 11:30

VSadov force-pushed the valArr branch from 20b382b to ee94b5e Compare March 3, 2023 00:48

This was referenced Mar 3, 2023

Mac machines hitting "Provisionaing request delayed or failed" dotnet/arcade#12769

Closed

Alpine System.Net.Security.Tests failing because of "Cannot load library libgssapi_krb5.so.2" #82945

Closed

VSadov marked this pull request as ready for review March 4, 2023 00:47

VSadov requested a review from MichalStrehovsky as a code owner March 4, 2023 00:47

VSadov commented Mar 4, 2023

View reviewed changes

src/coreclr/vm/methodtablebuilder.cpp Outdated Show resolved Hide resolved

jkotas reviewed Mar 4, 2023

View reviewed changes

VSadov commented Mar 4, 2023

View reviewed changes

src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs Outdated Show resolved Hide resolved

jkotas reviewed Mar 4, 2023

View reviewed changes

VSadov added 13 commits March 10, 2023 15:18

more tests

1852993

limit the max size of array instance to 1MiB

295caf1

fix an assert in importercalls.cpp

e7738ea

"result" in GC layout should track the pointer count, not the size

065467b

error messages

62b20c8

fixed uses of "value array" in comments.

01e2e17

PR feedback on StackAllocedArguments

f844772

use the same size limit for inline arrays in the typeloader

1b5f608

more PR feedback

bf676e4

remove SetCannotBeBlittedByObjectCloner

7d42fbc

moving GetInlineArrayLength to MetadataType and related changes

7fff2be

use type.Context.Target.PointerSize

2d403ee

lost resources change

f6f6acc

VSadov force-pushed the valArr branch from 7e4d22c to f6f6acc Compare March 10, 2023 23:18

build-analysis bot mentioned this pull request Mar 11, 2023

[release/6.0] Doublelinklist GC failures on Mono #83245

Closed

fix for x86

d68cfa2

MichalStrehovsky reviewed Mar 13, 2023

View reviewed changes

VSadov added 2 commits March 13, 2023 10:34

Do not make InlineArrayType an inline array just yet.

ebc698b

CORINFO_FLG_INDEXABLE_FIELDS

9373c77

AndyAyersMS approved these changes Mar 13, 2023

View reviewed changes

build-analysis bot mentioned this pull request Mar 14, 2023

[jitstress] HardwareIntrinsics_ro fails with "process cannot access the file" error #83298

Closed

jkotas approved these changes Mar 16, 2023

View reviewed changes

VSadov merged commit a2b70f9 into dotnet:main Mar 16, 2023

VSadov deleted the valArr branch March 16, 2023 21:45

VSadov mentioned this pull request Mar 23, 2023

Using canonical All-pointers GC desc in more scenarios. #83800

Merged

AaronRobinsonMSFT mentioned this pull request Mar 28, 2023

[Tracking] CoreCLR support for InlineArrayAttribute #81145

Closed

3 tasks

ghost locked as resolved and limited conversation to collaborators Apr 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CoreCLR support for InlineArrayAttribute. (struct layout part) #82744

CoreCLR support for InlineArrayAttribute. (struct layout part) #82744

VSadov commented Feb 27, 2023 •

edited

Loading

VSadov commented Mar 4, 2023

VSadov commented Mar 4, 2023

mangod9 commented Mar 4, 2023

VSadov Mar 4, 2023

MichalStrehovsky Mar 6, 2023

VSadov Mar 11, 2023

AndyAyersMS Mar 13, 2023

VSadov Mar 13, 2023

jakobbotsch Mar 16, 2023

VSadov Mar 16, 2023

VSadov Mar 16, 2023

VSadov Mar 16, 2023

jakobbotsch Mar 17, 2023

VSadov Mar 4, 2023

jkotas Mar 4, 2023

mangod9 Mar 4, 2023

VSadov Mar 4, 2023

MichalStrehovsky Mar 6, 2023

VSadov Mar 8, 2023

jkotas Mar 4, 2023

jkotas Mar 4, 2023

VSadov commented Mar 12, 2023

azure-pipelines bot commented Mar 12, 2023

MichalStrehovsky Mar 13, 2023

MichalStrehovsky Mar 13, 2023

MichalStrehovsky Mar 13, 2023

VSadov Mar 13, 2023

MichalStrehovsky Mar 13, 2023

VSadov Mar 14, 2023

AndyAyersMS left a comment

jkotas left a comment

VSadov commented Mar 16, 2023

CoreCLR support for InlineArrayAttribute. (struct layout part) #82744

CoreCLR support for InlineArrayAttribute. (struct layout part) #82744

Conversation

VSadov commented Feb 27, 2023 • edited Loading

VSadov commented Mar 4, 2023

VSadov commented Mar 4, 2023

mangod9 commented Mar 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VSadov commented Mar 12, 2023

azure-pipelines bot commented Mar 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndyAyersMS left a comment

Choose a reason for hiding this comment

jkotas left a comment

Choose a reason for hiding this comment

VSadov commented Mar 16, 2023

VSadov commented Feb 27, 2023 •

edited

Loading