Rework JIT<->EE communication for struct promotion; add support for general recursive struct promotion #85569

jakobbotsch · 2023-04-30T15:50:23Z

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 flattenType. 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_INDEXABLE_FIELDS (the latter by adding support to inline arrays in this new function). Last but not least, this adds support for general recursive struct promotion provided the field limit of 4 is not hit (for now, let's see if there are any more fundamental issues with this).

Fix #85511
Fix #71711
Fix #7576

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 'flattenType'. As a side effect, removes CORINFO_FLG_CUSTOMLAYOUT and CORINFO_FLG_INDEXABLE_FIELDS (the latter by adding support to inline arrays in this new function). Also, last but not least, this adds support for general recursive struct promotion provided the field limit of 4 is not hit (for now, let's see if there are any more fundamental issues with this). Fix dotnet#85511 Fix dotnet#71711

ghost · 2023-04-30T15:50:37Z

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 flattenType. 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_INDEXABLE_FIELDS (the latter by adding support to inline arrays in this new function). Last but not least, this adds support for general recursive struct promotion provided the field limit of 4 is not hit (for now, let's see if there are any more fundamental issues with this).

Fix #85511
Fix #71711
Fix #7576

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

jakobbotsch · 2023-04-30T18:29:05Z

src/coreclr/jit/lclvars.cpp

-    if (varDsc->lvIsStructField)
-    {
-        LclVarDsc* parentvarDsc = lvaGetDesc(varDsc->lvParentLcl);
-#if !defined(TARGET_64BIT)
-        if (varTypeIsLong(parentvarDsc))
-        {
-            bool isLo = (lclNum == parentvarDsc->lvFieldLclStart);
-            printf(" V%02u.%s(offs=0x%02x)", varDsc->lvParentLcl, isLo ? "lo" : "hi", isLo ? 0 : genTypeSize(TYP_INT));
-        }
-        else
-#endif // !defined(TARGET_64BIT)
-        {
-            CORINFO_CLASS_HANDLE typeHnd = parentvarDsc->GetLayout()->GetClassHandle();
-            CORINFO_FIELD_HANDLE fldHnd  = info.compCompHnd->getFieldInClass(typeHnd, varDsc->lvFldOrdinal);
-
-            char buffer[128];
-            printf(" V%02u.%s(offs=0x%02x)", varDsc->lvParentLcl, eeGetFieldName(fldHnd, false, buffer, sizeof(buffer)),
-                   varDsc->lvFldOffset);
-
-            lvaPromotionType promotionType = lvaGetPromotionType(parentvarDsc);
-            switch (promotionType)
-            {
-                case PROMOTION_TYPE_NONE:
-                    printf(" P-NONE");
-                    break;
-                case PROMOTION_TYPE_DEPENDENT:
-                    printf(" P-DEP");
-                    break;
-                case PROMOTION_TYPE_INDEPENDENT:
-                    printf(" P-INDEP");
-                    break;
-            }
-        }
-    }


LclVarDsc::lvReason printed below already contains this information, so we were printing it twice (and this code wouldn't work anymore, since lvFldOrdinal cannot be used to map fields of the parent local in the general case).

jakobbotsch · 2023-05-01T14:05:09Z

The promotion of inline arrays does not really work well because local morph does not recognize the accesses to other elements based on the first element:

LocalAddressVisitor visiting statement:
STMT00008 ( INL01 @ 0x010[E-] ... ??? ) <- INLRT @ 0x000[E-]
               [000034] -ACXG------                         ▌  ASG       ref   
               [000033] --CXG--N---                         ├──▌  IND       ref   
               [000054] -----------                         │  └──▌  ADD       byref 
               [000050] -----------                         │     ├──▌  FIELD_ADDR byref  System.ThreeObjects:Arg0
               [000049] -----------                         │     │  └──▌  LCL_ADDR  byref  V06 loc0         [+0]
                                                            │     │  └──▌    ref    field V06.Arg0 (fldOffset=0x0) -> V10 tmp3         
                                                            │     │  └──▌    ref    field V06.Arg0 (fldOffset=0x8) -> V11 tmp4         
                                                            │     │  └──▌    ref    field V06.Arg0 (fldOffset=0x10) -> V12 tmp5         
               [000053] -----------                         │     └──▌  CNS_INT   long   8
               [000002] -----------                         └──▌  LCL_VAR   ref    V04 arg4         
Replacing the field in promoted struct with local var V10

Local V10 should not be enregistered because: it is address exposed

Local V11 should not be enregistered because: it is address exposed

Local V12 should not be enregistered because: it is address exposed

Local V06 should not be enregistered because: it is address exposed
LocalAddressVisitor modified statement:
STMT00008 ( INL01 @ 0x010[E-] ... ??? ) <- INLRT @ 0x000[E-]
               [000034] -ACXG------                         ▌  ASG       ref   
               [000033] --CXG--N---                         ├──▌  IND       ref   
               [000054] -----------                         │  └──▌  ADD       byref 
               [000093] -----------                         │     ├──▌  LCL_ADDR  long   V10 tmp3         [+0]
               [000094] -----------                         │     └──▌  CNS_INT   long   8
               [000002] -----------                         └──▌  LCL_VAR   ref    V04 arg4

That leads to significantly worse codegen than without promotion (this example is from System.Text.StringBuilder:AppendFormat(System.IFormatProvider,System.String,System.Object,System.Object,System.Object)).. Likely need to teach local morph to be smarter.

tannergooding · 2023-05-01T17:00:04Z

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

-                significantPadding = true;
+                if (!type.ContainsGCPointers && !type.IsByRefLike)
+                {
+                    significantPadding = true;


Does this pessimize cases that don't have padding?

Say for example you've defined a union { float f; int32_t i; } and therefore it is explicit, but the two structures are "perfectly overlapping".

-- JW as a potential "future" improvement, not something to be handled in this PR or anything

No, since we don't support promotion of structs with overlapping fields. But if we did still no as the JIT only pays attention to significantPadding when there are holes in the struct.

ghost · 2023-06-03T23:01:44Z

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

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

ghost assigned jakobbotsch Apr 30, 2023

Fix a few bugs

e7bfbbd

jakobbotsch commented Apr 30, 2023

View reviewed changes

jakobbotsch added 4 commits April 30, 2023 23:00

SPMI support

1d7cde5

SSA fix

cbba7e0

Retain liberal handling of explicit layout structs with GC pointers

273f432

Handle native value types

35016da

runfoapp bot mentioned this pull request May 1, 2023

Long Running Test: Interop/MonoAPI/MonoMono/PInvokeDetach/PInvokeDetach.sh #73040

Closed

Work around new write barriers after promoting InlineArray

bcb95eb

tannergooding reviewed May 1, 2023

View reviewed changes

jakobbotsch added 4 commits May 2, 2023 01:18

Promote NFloat, Vector64

3aea901

Tweak local morph

1756b03

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

daa4d35

Fix after merge

c7833b3

jakobbotsch mentioned this pull request May 2, 2023

JIT: Allow forward sub to reorder trees throwing the same single exception #85647

Merged

jakobbotsch added 4 commits May 2, 2023 15:19

JIT: Allow forward sub to reorder trees throwing same exceptions

cadc3b9

Fix a bug

7e03161

Add some logging

0dd2a89

Run jit-format; clean up a bit

64da446

jakobbotsch mentioned this pull request May 2, 2023

JIT: Fix liveness for partial defs of parent locals #85654

Merged

This was referenced May 2, 2023

Failures in System.Net.Mail.Tests.SmtpClientTest tests #85637

Closed

System.IO.Tests.RandomAccess_NoBuffering.ReadUsingSingleBuffer timing out #85659

Closed

jakobbotsch added 2 commits May 4, 2023 19:58

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

3585ed7

Fix after merge

dac6e91

jakobbotsch closed this May 4, 2023

jakobbotsch reopened this May 4, 2023

ghost closed this Jun 3, 2023

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

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Rework JIT<->EE communication for struct promotion; add support for general recursive struct promotion #85569

Rework JIT<->EE communication for struct promotion; add support for general recursive struct promotion #85569

Uh oh!

jakobbotsch commented Apr 30, 2023 •

edited

Loading

Uh oh!

ghost commented Apr 30, 2023

Uh oh!

jakobbotsch Apr 30, 2023 •

edited

Loading

Uh oh!

jakobbotsch commented May 1, 2023 •

edited

Loading

Uh oh!

tannergooding May 1, 2023

Uh oh!

jakobbotsch May 1, 2023

Uh oh!

ghost commented Jun 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Rework JIT<->EE communication for struct promotion; add support for general recursive struct promotion #85569

Rework JIT<->EE communication for struct promotion; add support for general recursive struct promotion #85569

Uh oh!

Conversation

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

Uh oh!

Uh oh!

ghost commented Apr 30, 2023

Uh oh!

jakobbotsch Apr 30, 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!

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

Uh oh!

Uh oh!

tannergooding May 1, 2023

Choose a reason for hiding this comment

Uh oh!

jakobbotsch May 1, 2023

Choose a reason for hiding this comment

Uh oh!

ghost commented Jun 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jakobbotsch commented Apr 30, 2023 •

edited

Loading

jakobbotsch Apr 30, 2023 •

edited

Loading

jakobbotsch commented May 1, 2023 •

edited

Loading