Rework JIT<->EE communication for struct promotion #87917

jakobbotsch · 2023-06-22T10:47:13Z

Rework the communication so that the JIT asks the EE for struct field related information relevant to promotion in a single JIT-EE call of a new function called getTypeLayout. Crossgen2 can then more easily provide the information that the JIT is allowed to depend on.

As a side effect, removes CORINFO_FLG_CUSTOMLAYOUT (which fixes #71711) and CORINFO_FLG_DONT_DIG_FIELDS.

Fix #85511
Fix #71711

This also makes physical promotion more precise with regards to padding that can be skipped as part of block operations, since we now recursively consider padding in sub structures as insignificant.

See my comment below for some more information.

Note: I previous had a PR that also enabled recursive struct promotion together with this, but I think that needs a separate investigation of the diffs to allow.

Rework the communication so that the JIT asks the EE for struct field related information relevant to promotion in a single JIT-EE call of a new function called getTypeLayout. Crossgen2 can then more easily provide the information that the JIT is allowed to depend on. As a side effect, removes CORINFO_FLG_CUSTOMLAYOUT (which fixes dotnet#71711) and CORINFO_FLG_DONT_DIG_FIELDS. Fix dotnet#85511 Fix dotnet#71711

ghost · 2023-06-22T10:47:25Z

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

Issue Details

Rework the communication so that the JIT asks the EE for struct field related information relevant to promotion in a single JIT-EE call of a new function called getTypeLayout. Crossgen2 can then more easily provide the information that the JIT is allowed to depend on.

As a side effect, removes CORINFO_FLG_CUSTOMLAYOUT (which fixes #71711) and CORINFO_FLG_DONT_DIG_FIELDS.

Fix #85511
Fix #71711

Author:	jakobbotsch
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

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

jakobbotsch · 2023-06-22T21:53:07Z

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress, runtime-jit-experimental, runtime-coreclr outerloop

azure-pipelines · 2023-06-22T21:53:32Z

Azure Pipelines successfully started running 4 pipeline(s).

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

Having these class handles floating around in the JIT is problematic during prejit where the JIT-EE calls allowed on "random" type handles (in this case the type of a field of a struct) is limited. Precursor to dotnet#87917

… in the recursive function

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

jakobbotsch · 2023-06-23T23:42:55Z

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress, runtime-coreclr outerloop

jakobbotsch · 2023-06-24T20:53:44Z

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress

azure-pipelines · 2023-06-24T20:53:56Z

Azure Pipelines successfully started running 2 pipeline(s).

Having these class handles floating around in the JIT is problematic during prejit where the JIT-EE calls allowed on "random" type handles (in this case the type of a field of a struct) is limited. Precursor to dotnet#87917

jakobbotsch · 2023-06-26T19:33:24Z

cc @dotnet/jit-contrib PTAL @AndyAyersMS (JIT side) @davidwrighton (VM/crossgen2 parts).

This passed outerloop and the jitstress pipelines above.

I've verified that this is no diff with jit-diff PMI for win-x64. I'll do some more validation for other platforms before merging this.

For CG2 it is a size regression overall over frameworks to enable this promotion, at least on win-x64. It seems primarily to be because promotion disables last-use copy omission for implicit byrefs. Since this optimization is new in .NET 8 it still means this is not a size-wise regression going from .NET 7 to .NET 8.
I plan to collect some more diffs for other platforms before merging this, but it should be good to review.

I'm including the changes from #87967 as part of this PR but expect those changes to merge separately before this. Those are JIT changes to make sure we are no longer passing type handles returned by CG2 for arbitrary fields around throughout the JIT -- there is a documented contract for how those type handles can be used now.

win-x64 CG2 diffs

Found 453 files with textual diffs.

Summary of Code Size diffs:
(Lower is better)

Total bytes of base: 38402462
Total bytes of diff: 38524554
Total bytes of delta: 122092 (0.32 % of base)
Total relative delta: NaN
    diff is a regression.
    relative diff is a regression.


Top file regressions (bytes):
       20988 : System.Security.Cryptography.dasm (3.19% of base)
       18253 : System.Private.Xml.dasm (0.69% of base)
       13602 : System.Text.Json.dasm (3.13% of base)
       10728 : Microsoft.CodeAnalysis.VisualBasic.dasm (0.22% of base)
       10546 : Microsoft.CodeAnalysis.CSharp.dasm (0.17% of base)
        6586 : System.Security.Cryptography.Pkcs.dasm (2.75% of base)
        6435 : System.Net.Http.dasm (1.28% of base)
        2793 : System.Security.Cryptography.Cose.dasm (5.96% of base)
        2553 : System.Private.DataContractSerialization.dasm (0.40% of base)
        2352 : System.Formats.Asn1.dasm (3.20% of base)
        2271 : System.Net.Sockets.dasm (1.50% of base)
        2147 : System.Formats.Tar.dasm (3.03% of base)
        2080 : System.Runtime.Numerics.dasm (2.06% of base)
        1894 : System.Net.Security.dasm (1.29% of base)
        1779 : System.Net.Quic.dasm (2.63% of base)
        1186 : Microsoft.CodeAnalysis.dasm (0.06% of base)
        1148 : Newtonsoft.Json.dasm (0.21% of base)
        1128 : System.Numerics.Tensors.dasm (4.05% of base)
        1115 : System.Data.Common.dasm (0.11% of base)
         905 : System.Private.Xml.Linq.dasm (0.83% of base)

Top file improvements (bytes):
       -1285 : System.Text.Encodings.Web.dasm (-4.54% of base)
        -600 : System.Text.Encoding.CodePages.dasm (-1.06% of base)
        -574 : System.Text.RegularExpressions.dasm (-0.20% of base)
        -517 : System.Threading.RateLimiting.dasm (-1.24% of base)
        -475 : xunit.runner.utility.netcoreapp10.dasm (-0.36% of base)
        -450 : System.DirectoryServices.AccountManagement.dasm (-0.20% of base)
        -428 : ILCompiler.Reflection.ReadyToRun.dasm (-0.34% of base)
        -350 : System.Reflection.MetadataLoadContext.dasm (-0.23% of base)
        -303 : System.Linq.Parallel.dasm (-0.11% of base)
        -296 : System.Web.HttpUtility.dasm (-2.72% of base)
        -289 : Microsoft.Diagnostics.Tracing.TraceEvent.dasm (-0.01% of base)
        -273 : Microsoft.Extensions.Caching.Memory.dasm (-1.67% of base)
        -262 : System.Configuration.ConfigurationManager.dasm (-0.09% of base)
        -236 : System.IO.Compression.dasm (-0.35% of base)
        -177 : System.Diagnostics.EventLog.dasm (-0.18% of base)
        -135 : System.Drawing.Primitives.dasm (-0.43% of base)
        -116 : Microsoft.Extensions.FileProviders.Physical.dasm (-0.79% of base)
        -114 : System.Threading.Tasks.Parallel.dasm (-0.45% of base)
        -103 : System.Speech.dasm (-0.03% of base)
         -94 : System.Management.dasm (-0.03% of base)

148 total files with Code Size differences (41 improved, 107 regressed), 129 unchanged.

Top method regressions (bytes):
        1462 (24.40% of base) : System.Net.Http.dasm - System.Net.Http.Headers.QPackStaticTable:.cctor()
         708 (63.05% of base) : System.Private.Xml.dasm - System.Xml.Serialization.CodeGenerator:.cctor()
         650 (20.09% of base) : System.Net.Http.dasm - System.Net.Http.Http2Connection+Http2Stream:.cctor()
         643 (12.33% of base) : System.Text.Json.dasm - System.Text.Json.JsonDocument:TryGetValue(int,byref):bool:this (15 methods)
         517 ( 9.37% of base) : Microsoft.CodeAnalysis.CSharp.dasm - Microsoft.CodeAnalysis.CSharp.CSharpDeclarationComputer:ComputeDeclarations(Microsoft.CodeAnalysis.SemanticModel,Microsoft.CodeAnalysis.ISymbol,Microsoft.CodeAnalysis.SyntaxNode,System.Func`3[Microsoft.CodeAnalysis.SyntaxNode,System.Nullable`1[int],bool],bool,Microsoft.CodeAnalysis.PooledObjects.ArrayBuilder`1[Microsoft.CodeAnalysis.DeclarationInfo],System.Nullable`1[int],System.Threading.CancellationToken)
         498 (66.49% of base) : System.Reflection.DispatchProxy.dasm - System.Reflection.DispatchProxyGenerator+ProxyBuilder:.cctor()
         407 (26.81% of base) : System.Private.Xml.dasm - System.Xml.Xsl.IlGen.XmlILOptimizerVisitor:FoldArithmetic(int,System.Xml.Xsl.Qil.QilLiteral,System.Xml.Xsl.Qil.QilLiteral):System.Xml.Xsl.Qil.QilNode:this
         371 ( 1.37% of base) : Microsoft.CodeAnalysis.CSharp.dasm - Microsoft.CodeAnalysis.CSharp.CSharpCommandLineParser:Parse(System.Collections.Generic.IEnumerable`1[System.String],System.String,System.String,System.String):Microsoft.CodeAnalysis.CSharp.CSharpCommandLineArguments:this
         360 ( 6.56% of base) : System.Private.Xml.dasm - System.Xml.XmlTextReaderImpl+<ParseXmlDeclarationAsync>d__505:MoveNext():this
         353 (20.84% of base) : System.Private.Xml.dasm - System.Xml.XmlDocument:.cctor()
         342 ( 9.20% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Microsoft.CodeAnalysis.VisualBasic.VisualBasicDeclarationComputer:ComputeDeclarationsCore(Microsoft.CodeAnalysis.SemanticModel,Microsoft.CodeAnalysis.SyntaxNode,System.Func`3[Microsoft.CodeAnalysis.SyntaxNode,System.Nullable`1[int],bool],bool,Microsoft.CodeAnalysis.PooledObjects.ArrayBuilder`1[Microsoft.CodeAnalysis.DeclarationInfo],System.Nullable`1[int],System.Threading.CancellationToken)
         336 (33.50% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.JsonSerializer:SerializeInternal(Newtonsoft.Json.JsonWriter,System.Object,System.Type):this
         330 ( 1.09% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Microsoft.CodeAnalysis.VisualBasic.VisualBasicCommandLineParser:Parse(System.Collections.Generic.IEnumerable`1[System.String],System.String,System.String,System.String):Microsoft.CodeAnalysis.VisualBasic.VisualBasicCommandLineArguments:this
         322 ( 5.84% of base) : System.Reflection.MetadataLoadContext.dasm - System.Reflection.TypeLoading.CoreTypeHelpers:GetFullName(int,byref,byref)
         321 (20.55% of base) : System.Data.Common.dasm - System.Data.Common.TimeSpanStorage:Aggregate(int[],int):System.Object:this
         313 (17.55% of base) : System.Text.Json.dasm - System.Text.Json.JsonSerializer:LookupProperty(System.Object,System.ReadOnlySpan`1[ubyte],byref,System.Text.Json.JsonSerializerOptions,byref,bool):System.Text.Json.Serialization.Metadata.JsonPropertyInfo
         311 ( 6.26% of base) : System.Private.Xml.dasm - System.Xml.DtdParser+<GetTokenAsync>d__173:MoveNext():this
         310 ( 9.36% of base) : Microsoft.CodeAnalysis.CSharp.dasm - Microsoft.CodeAnalysis.CSharp.SymbolDisplayVisitor:VisitMethod(Microsoft.CodeAnalysis.IMethodSymbol):this
         294 ( 5.21% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Microsoft.CodeAnalysis.VisualBasic.Syntax.InternalSyntax.Scanner:ScanNumericLiteral(Microsoft.CodeAnalysis.Syntax.InternalSyntax.SyntaxList`1[Microsoft.CodeAnalysis.VisualBasic.Syntax.InternalSyntax.VisualBasicSyntaxNode]):Microsoft.CodeAnalysis.VisualBasic.Syntax.InternalSyntax.SyntaxToken:this
         289 (15.59% of base) : System.Diagnostics.Process.dasm - System.Diagnostics.NtProcessManager:GetProcessInfos(System.Diagnostics.PerformanceCounterLib,int,int,System.ReadOnlySpan`1[ubyte]):System.Diagnostics.ProcessInfo[]

Top method improvements (bytes):
        -948 (-16.64% of base) : System.Text.RegularExpressions.dasm - System.Text.RegularExpressions.RegexWriter:EmitFragment(ubyte,System.Text.RegularExpressions.RegexNode,int):this
        -435 (-18.61% of base) : System.Text.Encoding.CodePages.dasm - System.Text.ISCIIEncoding:GetChars(ulong,int,ulong,int,System.Text.DecoderNLS):int:this
        -427 (-8.97% of base) : xunit.console.dasm - Xunit.ConsoleClient.CommandLine:Parse(System.Predicate`1[System.String]):Xunit.XunitProject:this
        -370 (-17.53% of base) : xunit.runner.utility.netcoreapp10.dasm - Xunit.ConfigReader_Json:Load(System.IO.Stream):Xunit.TestAssemblyConfiguration
        -351 (-39.00% of base) : System.Text.Encodings.Web.dasm - System.Text.Encodings.Web.DefaultJavaScriptEncoder+EscaperImplementation:<EncodeUtf16>g__TryEncodeScalarAsHex|5_0(System.Object,System.Text.Rune,System.Span`1[ushort]):int
        -344 (-6.01% of base) : System.Runtime.Numerics.dasm - System.Globalization.FormatProvider+Number:NumberToStringFormat(byref,byref,System.ReadOnlySpan`1[ushort],System.Globalization.NumberFormatInfo)
        -344 (-40.52% of base) : System.Text.Encodings.Web.dasm - System.Text.Encodings.Web.DefaultJavaScriptEncoder+EscaperImplementation:<EncodeUtf8>g__TryEncodeScalarAsHex|4_0(System.Object,System.Text.Rune,System.Span`1[ubyte]):int
        -321 (-19.42% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Microsoft.CodeAnalysis.VisualBasic.Symbols.ImplementsHelper:ValidateImplementedMember[System.__Canon](System.__Canon,System.__Canon,Microsoft.CodeAnalysis.VisualBasic.Syntax.QualifiedNameSyntax,Microsoft.CodeAnalysis.VisualBasic.Binder,Microsoft.CodeAnalysis.VisualBasic.BindingDiagnosticBag,Microsoft.CodeAnalysis.VisualBasic.Symbols.TypeSymbol,System.String,byref):System.__Canon
        -306 (-47.37% of base) : Microsoft.Extensions.Logging.Console.dasm - Microsoft.Extensions.Logging.Console.SimpleConsoleFormatter:GetLogLevelConsoleColors(int):Microsoft.Extensions.Logging.Console.SimpleConsoleFormatter+ConsoleColors:this
        -289 (-9.26% of base) : System.Private.Xml.dasm - System.Xml.XmlConvert:DecodeName(System.String):System.String
        -281 (-41.63% of base) : System.Text.Encodings.Web.dasm - System.Text.Encodings.Web.DefaultUrlEncoder+EscaperImplementation:EncodeUtf16(System.Text.Rune,System.Span`1[ushort]):int:this
        -275 (-42.90% of base) : System.Text.Encodings.Web.dasm - System.Text.Encodings.Web.DefaultUrlEncoder+EscaperImplementation:EncodeUtf8(System.Text.Rune,System.Span`1[ubyte]):int:this
        -275 (-17.87% of base) : System.Private.Uri.dasm - System.UriBuilder:ToString():System.String:this
        -267 (-24.61% of base) : System.IO.Compression.dasm - System.IO.Compression.Zip64ExtraField:TryGetZip64BlockFromGenericExtraField(System.IO.Compression.ZipGenericExtraField,bool,bool,bool,bool,byref):bool
        -253 (-5.10% of base) : Microsoft.CodeAnalysis.dasm - Microsoft.CodeAnalysis.GeneratorDriver:RunGeneratorsCore(Microsoft.CodeAnalysis.Compilation,Microsoft.CodeAnalysis.DiagnosticBag,System.Threading.CancellationToken):Microsoft.CodeAnalysis.GeneratorDriverState:this
        -247 (-5.67% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.JsonWriter:WriteValue(Newtonsoft.Json.JsonWriter,int,System.Object)
        -244 (-5.44% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.JsonWriter:WriteValueAsync(Newtonsoft.Json.JsonWriter,int,System.Object,System.Threading.CancellationToken):System.Threading.Tasks.Task
        -229 (-11.21% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.Schema.JsonSchemaGenerator:GenerateInternal(System.Type,int,bool):Newtonsoft.Json.Schema.JsonSchema:this
        -213 (-8.64% of base) : System.Private.Xml.dasm - System.Xml.Xsl.XPathConvert+BigNumber:DblToRgbFast(double,ubyte[],byref,byref):bool
        -193 (-6.78% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.Schema.JsonSchemaBuilder:ProcessSchemaProperties(Newtonsoft.Json.Linq.JObject):this

Top method regressions (percentages):
          51 (137.84% of base) : System.Net.Security.dasm - System.Net.Security.TlsFrameHelper:SkipOpaqueType1(System.ReadOnlySpan`1[ubyte]):System.ReadOnlySpan`1[ubyte]
          78 (91.76% of base) : System.Private.Xml.dasm - System.Xml.Schema.Compiler:IsValidOccurrenceRangeRestriction(System.Xml.Schema.XmlSchemaParticle,System.Xml.Schema.XmlSchemaParticle):bool
          78 (91.76% of base) : System.Private.Xml.dasm - System.Xml.Schema.SchemaCollectionCompiler:IsValidOccurrenceRangeRestriction(System.Xml.Schema.XmlSchemaParticle,System.Xml.Schema.XmlSchemaParticle):bool
         102 (79.69% of base) : xunit.console.dasm - Internal.Microsoft.Extensions.DependencyModel.CompilationOptions:.cctor()
         102 (79.69% of base) : Microsoft.Extensions.DependencyModel.dasm - Microsoft.Extensions.DependencyModel.CompilationOptions:.cctor()
          15 (78.95% of base) : System.IO.Compression.dasm - System.IO.Compression.ZipArchive:CreateEntry(System.String):System.IO.Compression.ZipArchiveEntry:this
          15 (78.95% of base) : System.IO.Compression.ZipFile.dasm - System.IO.Compression.ZipFileExtensions:CreateEntryFromFile(System.IO.Compression.ZipArchive,System.String,System.String):System.IO.Compression.ZipArchiveEntry
          54 (78.26% of base) : System.Numerics.Tensors.dasm - System.Numerics.Tensors.ArrayUtilities:GetIndex(int[],System.ReadOnlySpan`1[int],int):int
          13 (72.22% of base) : System.Text.Json.dasm - System.Text.Json.Nodes.JsonNode:op_Implicit(double):System.Text.Json.Nodes.JsonNode
          13 (72.22% of base) : System.Text.Json.dasm - System.Text.Json.Nodes.JsonNode:op_Implicit(float):System.Text.Json.Nodes.JsonNode
          13 (72.22% of base) : System.Text.Json.dasm - System.Text.Json.Nodes.JsonNode:op_Implicit(int):System.Text.Json.Nodes.JsonNode
          13 (72.22% of base) : System.Text.Json.dasm - System.Text.Json.Nodes.JsonNode:op_Implicit(long):System.Text.Json.Nodes.JsonNode
          13 (72.22% of base) : System.Text.Json.dasm - System.Text.Json.Nodes.JsonNode:op_Implicit(System.DateTimeOffset):System.Text.Json.Nodes.JsonNode
          13 (72.22% of base) : System.Text.Json.dasm - System.Text.Json.Nodes.JsonNode:op_Implicit(System.Decimal):System.Text.Json.Nodes.JsonNode
          13 (72.22% of base) : System.Text.Json.dasm - System.Text.Json.Nodes.JsonNode:op_Implicit(System.Guid):System.Text.Json.Nodes.JsonNode
          13 (72.22% of base) : System.Text.Json.dasm - System.Text.Json.Nodes.JsonNode:op_Implicit(System.Nullable`1[double]):System.Text.Json.Nodes.JsonNode
          13 (72.22% of base) : System.Text.Json.dasm - System.Text.Json.Nodes.JsonNode:op_Implicit(System.Nullable`1[long]):System.Text.Json.Nodes.JsonNode
          13 (72.22% of base) : System.Text.Json.dasm - System.Text.Json.Nodes.JsonNode:op_Implicit(System.Nullable`1[System.DateTime]):System.Text.Json.Nodes.JsonNode
          13 (72.22% of base) : System.Text.Json.dasm - System.Text.Json.Nodes.JsonNode:op_Implicit(System.Nullable`1[System.DateTimeOffset]):System.Text.Json.Nodes.JsonNode
          13 (72.22% of base) : System.Text.Json.dasm - System.Text.Json.Nodes.JsonNode:op_Implicit(System.Nullable`1[System.Decimal]):System.Text.Json.Nodes.JsonNode

Top method improvements (percentages):
         -14 (-77.78% of base) : xunit.runner.utility.netcoreapp10.dasm - Xunit.TestAssemblyConfiguration:get_MethodDisplayOptionsOrDefault():int:this
         -14 (-73.68% of base) : xunit.runner.utility.netcoreapp10.dasm - Xunit.TestAssemblyConfiguration:get_DiagnosticMessagesOrDefault():bool:this
         -14 (-73.68% of base) : xunit.runner.utility.netcoreapp10.dasm - Xunit.TestAssemblyConfiguration:get_InternalDiagnosticMessagesOrDefault():bool:this
         -14 (-73.68% of base) : xunit.runner.utility.netcoreapp10.dasm - Xunit.TestAssemblyConfiguration:get_ParallelizeAssemblyOrDefault():bool:this
         -14 (-73.68% of base) : xunit.runner.utility.netcoreapp10.dasm - Xunit.TestAssemblyConfiguration:get_StopOnFailOrDefault():bool:this
         -27 (-72.97% of base) : System.Diagnostics.DiagnosticSource.dasm - System.Diagnostics.Metrics.LastValue:Update(double):this
         -20 (-71.43% of base) : Microsoft.Extensions.FileProviders.Physical.dasm - Microsoft.Extensions.FileProviders.PhysicalFileProvider:set_UseActivePolling(bool):this
         -20 (-71.43% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.JsonSerializerSettings:set_DateFormatHandling(int):this
         -20 (-71.43% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.JsonSerializerSettings:set_DateParseHandling(int):this
         -20 (-71.43% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.JsonSerializerSettings:set_DateTimeZoneHandling(int):this
         -20 (-71.43% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.JsonSerializerSettings:set_FloatFormatHandling(int):this
         -20 (-71.43% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.JsonSerializerSettings:set_Formatting(int):this
         -40 (-65.57% of base) : System.Collections.Immutable.dasm - System.Collections.HashHelpers:get_Primes():System.ReadOnlySpan`1[int]
         -40 (-65.57% of base) : System.Configuration.ConfigurationManager.dasm - System.Configuration.XmlUtil:get_PositionOffset():System.ReadOnlySpan`1[int]
         -40 (-65.57% of base) : System.Reflection.Metadata.dasm - System.Reflection.Metadata.Ecma335.HasCustomAttributeTag:get_TagToTokenTypeArray():System.ReadOnlySpan`1[uint]
         -40 (-65.57% of base) : System.Reflection.Metadata.dasm - System.Reflection.Metadata.Ecma335.HasCustomDebugInformationTag:get_TagToTokenTypeArray():System.ReadOnlySpan`1[uint]
         -64 (-64.65% of base) : System.Private.DataContractSerialization.dasm - System.Runtime.Serialization.ObjectToIdCache:GetPrime(int):int
         -20 (-64.52% of base) : System.Composition.Convention.dasm - System.Composition.Convention.ImportConventionBuilder:AsMany(bool):System.Composition.Convention.ImportConventionBuilder:this
         -20 (-62.50% of base) : System.Composition.Convention.dasm - System.Composition.Convention.ImportConventionBuilder:AsMany():System.Composition.Convention.ImportConventionBuilder:this
         -17 (-60.71% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.JsonSerializerSettings:set_FloatParseHandling(int):this

9532 total methods with Code Size differences (3046 improved, 6486 regressed), 256068 unchanged.

jakobbotsch · 2023-06-26T19:39:34Z

src/coreclr/jit/gentree.cpp


                printf("\n");
                printf("                                                            ");
                printIndent(indentStack);
-                printf("    %-6s V%02u.%s (offs=0x%02x) -> ", varTypeName(fieldVarDsc->TypeGet()), tree->GetLclNum(),
-                       fieldName, fieldVarDsc->lvFldOffset);
+                printf("    %-6s %s -> ", varTypeName(fieldVarDsc->TypeGet()), fieldVarDsc->lvReason);


This is switched to use lvReason that already contains a description of promoted fields of similar nature, to avoid the use of the field above.

jakobbotsch · 2023-06-26T19:40:07Z

src/coreclr/jit/lclvars.cpp

+    // TODO-Quirk: The old logic disallowed promotion for "custom layout" HFAs.
+    // The equivalent check now is the following, but it is quite meaningless.
+    if (treeNodes[0].hasSignificantPadding && compiler->IsHfa(typeHnd))
    {
-        return CanConstructAndPromoteField(&structPromotionInfo);
+        return false;
    }


I'll remove this in a follow-up. It requires fixing a small bug in LSRA.

src/coreclr/jit/lclvars.cpp

src/coreclr/jit/codegencommon.cpp

…87967) Having these class handles floating around in the JIT is problematic during prejit where the JIT-EE calls allowed on "random" type handles (in this case the type of a field of a struct) is limited. Precursor to #87917

BruceForstall

A few comments

BruceForstall · 2023-06-26T21:58:32Z

src/coreclr/inc/corinfo.h

+    //
+    // Parameters:
+    //   typeHnd            - Handle of the type.
+    //   treeNodes          - [out] Pointer to tree node entries to write.


Technically [in, out]? Specifically, on entry it points to an array of CORINFO_TYPE_LAYOUT_NODE memory with *numTreeNodes elements.

I see in the JIT you call this with an arbitrary array of 256 elements, and bail on numbers above that.

If a caller wanted to allocate exactly the right number of nodes, what would they do? Perhaps they could
call this with treeNodes == nullptr and *numTreeNodes == 0 and on exit *numTreeNodes would equal the size needed? But that is semantically questionable since *numTreeNodes is "the number of entries written" which would have to be zero. So you'd have to iterate, sending larger and larger buffers to the API.

Should it be supported to get exactly the number needed in one call? Either here, or in a getTypeLayoutNodeCount API?

Technically [in, out]? Specifically, on entry it points to an array of CORINFO_TYPE_LAYOUT_NODE memory with *numTreeNodes elements.

I'll add a comment to this regard.

I see in the JIT you call this with an arbitrary array of 256 elements, and bail on numbers above that.

If a caller wanted to allocate exactly the right number of nodes, what would they do? Perhaps they could
call this with treeNodes == nullptr and *numTreeNodes == 0 and on exit *numTreeNodes would equal the size needed? But that is semantically questionable since *numTreeNodes is "the number of entries written" which would have to be zero. So you'd have to iterate, sending larger and larger buffers to the API.

Should it be supported to get exactly the number needed in one call? Either here, or in a getTypeLayoutNodeCount API?

I picked the number by measuring how many nodes the largest structs in all our SPMI collections have, doubling it, and rounding it up (IIRC it was between 80-100 nodes).

The 256 limit on the JIT side also serves to limit ourselves from pathological cases, because it is very easy (especially with upcoming inline arrays) to create structs with tons and tons of nodes in its layout. For physical promotion hitting this limit will just result in slightly conservative behavior, where we will believe padding to be significant. For large structs that are only going to be partially promoted that's not going to be harmful to CQ. For regular promotion we have a natural limit of 9 nodes due to its restrictions. So I would say that for now this is ok, and if we end up in a situation where it is too conservative in physical promotion we can look into improving it (IMO, a more preferable design in that situation might be callback based, e.g. walkTypeLayout -- but not sure we have much prior art to that).

For very large structs do you envision some way for the jit to indicate priority, eg from some prior analysis we see the field at offset 0xNNN is important and so we'd like to make sure the layout describes it explicitly, but that some other fields are not interesting and can just be treated as significant padding?

I suppose the API could take a range to return fields in, though then I think again the callback approach would be more flexible and with fewer downsides.

The actual "field type" parts are not used by physical promotion, so if regular promotion didn't exist this API would be quite a bit simpler (returning just a flat array of the significant segments). But for regular promotion it would technically be fine if it picked out a set of up to 4 fields it is interested in and only tried to promote these, though of course we would end up with dependent promotion a lot of the time.

src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp

BruceForstall · 2023-06-26T22:11:14Z

src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp

@@ -4735,6 +4735,86 @@ CORINFO_FIELD_HANDLE MethodContext::repGetFieldInClass(CORINFO_CLASS_HANDLE clsH
    return result;
 }

+void MethodContext::recGetTypeLayout(GetTypeLayoutResult result, CORINFO_CLASS_HANDLE typeHnd, CORINFO_TYPE_LAYOUT_NODE* nodes, size_t numNodes)


Is it confusing that this takes size_t numNodes instead of size_t* numNodes like the API itself? (although presumably it shouldn't alter the number)

Hmm, I initially had it as the pointer but realized this shouldn't be modified by the recording part, and changed it. I think I'll leave it like that.

src/coreclr/inc/corinfo.h

AndyAyersMS · 2023-06-26T22:58:32Z

src/coreclr/inc/corinfo.h

+    //
+    // Parameters:
+    //   typeHnd            - Handle of the type.
+    //   treeNodes          - [out] Pointer to tree node entries to write.


For very large structs do you envision some way for the jit to indicate priority, eg from some prior analysis we see the field at offset 0xNNN is important and so we'd like to make sure the layout describes it explicitly, but that some other fields are not interesting and can just be treated as significant padding?

src/coreclr/jit/lclvars.cpp

davidwrighton

I've reviewed the VM and CrossGen2/NativeAOT specific changes, and they look good to me. I have not reviewed any of the changes in the JIT itself.

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

Returning the input type handle back to the JIT is ok. That means the type handle is the direct type of an IL parameter, which we know is encodable.

jakobbotsch · 2023-06-27T12:55:41Z

I ran superpmi-collect for a special version of this branch that runs both promotions (to make sure all the data is available in the collections). Then I was able to use SPMI to get diffs for all other platforms. I checked disasm for win-x64, linux-x64, win-arm64, linux-arm64, and win-x86 and confirmed for all of these that this does not have any diffs in the non crossgen2 collections.

For the crossgen2 collections I will post the diffs, but first after I recollect them with one fix: I noticed that we stopped allowing normal promotion of vector structs since it bails out on the CG2 side. That promotion should be ok to do, so I have refined the check to allow it and will rerun diffs.

AndyAyersMS

Changes LGTM

jakobbotsch · 2023-06-27T19:18:10Z

Diffs. As mentioned above most of the regressions are from this disabling last-use copy omission optimization, and also some from #88086. Also some of the usual code size increases around promotion are seen, like block copies being turned into individual field copies which may take bigger code. We might consider separately if we should try to tune our promotion heuristics for CG2 since it seems like there may be some potential code size savings in doing so.

(Diffs may look small, but keep in mind these are diffs from a single collection)

ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jun 22, 2023

ghost assigned jakobbotsch Jun 22, 2023

jakobbotsch commented Jun 22, 2023

View reviewed changes

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

jakobbotsch added 5 commits June 22, 2023 13:06

Fix build

d571c73

Fix LA64/RISC-V build

5196cd5

Handle all SIMD/HW SIMD types

1955006

Remove some JITDUMP

7998e35

Small cleanup

e83d80b

davidwrighton reviewed Jun 22, 2023

View reviewed changes

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

jakobbotsch mentioned this pull request Jun 22, 2023

JIT: Generalized struct promotion #76928

Closed

40 tasks

This was referenced Jun 22, 2023

Test_EventSource_EtwManifestGeneration* tests failing in CI #48798

Closed

Work item tests crash on OSX x64 Debug #87287

Closed

LibraryImportGenerator.Unit.Tests crashed in CI #87951

Closed

jakobbotsch added 2 commits June 23, 2023 12:27

Address feedback

4e67323

Restore comment, fix a check

ed301b6

jakobbotsch mentioned this pull request Jun 23, 2023

JIT: Stop setting class handles for promoted parameter SIMD fields #87967

Merged

jakobbotsch added 5 commits June 23, 2023 16:03

Add getHfaType to the allowed list of calls

f07c36f

Fix build

ec44055

Fix SysV for wrapped SIMD fields

e75afd1

Avoid recursive fixups, switch to IsLayoutFixedInCurrentVersionBubble…

12c025d

… in the recursive function

davidwrighton reviewed Jun 23, 2023

View reviewed changes

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

Address feedback; remove an unused SPMI field

a1c01e3

jakobbotsch marked this pull request as ready for review June 26, 2023 19:33

jakobbotsch requested a review from MichalStrehovsky as a code owner June 26, 2023 19:33

jakobbotsch requested review from davidwrighton and AndyAyersMS June 26, 2023 19:33

jakobbotsch commented Jun 26, 2023

View reviewed changes

src/coreclr/jit/lclvars.cpp Outdated Show resolved Hide resolved

jakobbotsch commented Jun 26, 2023

View reviewed changes

src/coreclr/jit/codegencommon.cpp Outdated Show resolved Hide resolved

Merge branch 'main' of github.com:dotnet/runtime into fix-85511-2

673e94b

BruceForstall reviewed Jun 26, 2023

View reviewed changes

AndyAyersMS reviewed Jun 26, 2023

View reviewed changes

build-analysis bot mentioned this pull request Jun 27, 2023

Build on Windows Fails sometimes with fatal error C1090: PDB API call failed #48070

Open

davidwrighton approved these changes Jun 27, 2023

View reviewed changes

MichalStrehovsky reviewed Jun 27, 2023

View reviewed changes

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

jakobbotsch added 4 commits June 27, 2023 10:51

Address feedback

2143b89

Fix repGetTypeLayout partial results

09b9c4e

Avoid some regressions

c3218b3

Returning the input type handle back to the JIT is ok. That means the type handle is the direct type of an IL parameter, which we know is encodable.

Make a comment more precise

99f6744

AndyAyersMS approved these changes Jun 27, 2023

View reviewed changes

jakobbotsch merged commit 318c0e6 into dotnet:main Jun 27, 2023

jakobbotsch deleted the fix-85511-2 branch June 27, 2023 20:07

jakobbotsch mentioned this pull request Jun 30, 2023

Unify JIT interface getTypeLayout between VM and crossgen2; refine what is considered "significant padding" #88238

Merged

ghost locked as resolved and limited conversation to collaborators Jul 28, 2023

Rework JIT<->EE communication for struct promotion #87917

Rework JIT<->EE communication for struct promotion #87917

Uh oh!

Conversation

jakobbotsch commented Jun 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jun 22, 2023

Uh oh!

Uh oh!

jakobbotsch commented Jun 22, 2023

Uh oh!

azure-pipelines bot commented Jun 22, 2023

Uh oh!

Uh oh!

Uh oh!

jakobbotsch commented Jun 23, 2023

Uh oh!

jakobbotsch commented Jun 24, 2023

Uh oh!

azure-pipelines bot commented Jun 24, 2023

Uh oh!

jakobbotsch commented Jun 26, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

BruceForstall left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakobbotsch Jun 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

davidwrighton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jakobbotsch commented Jun 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AndyAyersMS left a comment

Choose a reason for hiding this comment

Uh oh!

jakobbotsch commented Jun 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jakobbotsch commented Jun 22, 2023 •

edited

Loading

jakobbotsch Jun 26, 2023 •

edited

Loading

jakobbotsch commented Jun 27, 2023 •

edited

Loading

jakobbotsch commented Jun 27, 2023 •

edited

Loading