JIT: Generalize assignment decomposition in physical promotion #85323

jakobbotsch · 2023-04-25T14:25:28Z

Generalize assignment decomposition to handle arbitrary combinations of physically and regularly promoted structs. Do this by introducing a DecompositionPlan class that keeps track of the copies to do that involve replacement fields. The first step is then to fill out this plan. In the general case where both the source and destination are physically promoted this involves iterating the replacements in lockstep. For promotions that map exactly, a direct copy between their locals is queued into the plan; in other cases (e.g. partial overlap) it may involve writing the source back to the struct local.

The plan is used to generate the IR and to compute the remainder that needs to be handled after all replacements have been handled. Example:

Example

V03 promoted with 1 replacements
  [016..020) promoted as int V05
...
V04 promoted with 1 replacements
  [008..012) promoted as int V06
...
STMT00011 ( 0x00C[--] ... ??? )
               [000052] -A---------                         ▌  ASG       struct (copy)
               [000051] D------N---                         ├──▌  LCL_VAR   struct<S, 24> V04 tmp2         
               [000008] -----------                         └──▌  LCL_VAR   struct<S, 24> V03 tmp1          (last use)
Processing block operation [000052] that involves replacements
  V06 (V04.[008..012)) <- src+008
  dst+016 <- V05 (V03.[016..020))
  Remainder: [000..008) [012..016) [020..024)
  => remainder strategy: retain a full block op

Local V03 should not be enregistered because: was accessed as a local field

Local V04 should not be enregistered because: was accessed as a local field
New statement:
STMT00011 ( 0x00C[--] ... ??? )
               [000064] -A---------                         ▌  COMMA     void  
               [000056] -A---------                         ├──▌  ASG       struct (copy)
               [000051] D------N---                         │  ├──▌  LCL_VAR   struct<S, 24> V04 tmp2         
               [000008] -----------                         │  └──▌  LCL_VAR   struct<S, 24> V03 tmp1         
               [000063] -A---------                         └──▌  COMMA     void  
               [000059] -A---------                            ├──▌  ASG       int   
               [000057] D------N---                            │  ├──▌  LCL_VAR   int    V06 tmp4         
               [000058] -----------                            │  └──▌  LCL_FLD   int    V03 tmp1         [+8]
               [000062] -A---------                            └──▌  ASG       int   
               [000060] U------N---                               ├──▌  LCL_FLD   int    V04 tmp2         [+16]
               [000061] -----------                               └──▌  LCL_VAR   int    V05 tmp3

(Also unnest StatementList and rename it to DecompositionStatementList as the other name conflicted with another class.)

Generalize assignment decomposition to handle arbitrary combinations of physically promoted structs. Do this by introducing a DecompositionPlan class that keeps track of the copies to do that involve replacement fields. The first step is then to fill out this plan. In the general case where both the source and destination are physically promoted this involves iterating the replacements in lockstep. For promotions that map exactly, a direct copy between their locals is queued into the plan; in other cases (e.g. partial overlap) it may involve writing the source back to the struct local. The plan is used to generate the IR and to figure out whether the writes of all the replacement fields covers the destination such that the original struct copy can be omitted. Also unnest StatementList and rename it to DecompositionStatementList as the other name conflicted with another class.

ghost · 2023-04-25T14:25:41Z

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

Issue Details

Generalize assignment decomposition to handle arbitrary combinations of physically promoted structs. Do this by introducing a DecompositionPlan class that keeps track of the copies to do that involve replacement fields. The first step is then to fill out this plan. In the general case where both the source and destination are physically promoted this involves iterating the replacements in lockstep. For promotions that map exactly, a direct copy between their locals is queued into the plan; in other cases (e.g. partial overlap) it may involve writing the source back to the struct local.

The plan is used to generate the IR and to figure out whether the writes of all the replacement fields covers the destination such that the original struct copy can be omitted.

Also unnest StatementList and rename it to DecompositionStatementList as the other name conflicted with another class.

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

jakobbotsch · 2023-04-25T14:37:39Z

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

azure-pipelines · 2023-04-25T14:38:01Z

Azure Pipelines successfully started running 3 pipeline(s).

src/coreclr/jit/promotion.cpp

jakobbotsch · 2023-04-25T17:24:50Z

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

azure-pipelines · 2023-04-25T17:25:03Z

Azure Pipelines successfully started running 2 pipeline(s).

jakobbotsch · 2023-04-25T19:05:32Z

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

azure-pipelines · 2023-04-25T19:05:48Z

Azure Pipelines successfully started running 2 pipeline(s).

jakobbotsch · 2023-04-25T21:54:36Z

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

azure-pipelines · 2023-04-25T21:54:50Z

Azure Pipelines successfully started running 2 pipeline(s).

The JIT has a flag GTF_ICON_INITCLASS that represents that accesses off that address are cctor dependent. Hoisting uses this to avoid hoisting cctor dependent indirections unless all cctors are also hoisted. However, local constant prop/VN-based constant prop do not handle this flag, so we could run into cases where addresses with GTF_ICON_INITCLASS were propagated and then subsequently hoisted incorrectly. This change moves the flag to an OperIsIndir() flag instead of being a flag on the constant. After some digging, I found that the original reason the flag was not an indir flag was simply that there were no more indir flags available, but we do have available flags today. This fix is much simpler than the alternatives which would be to teach VN/local copy prop to propagate this GTF_ICON_INITCLASS flag. Also remove GTF_FLD_INITCLASS which is never set.

jakobbotsch · 2023-04-26T14:22:27Z

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

azure-pipelines · 2023-04-26T14:22:46Z

Azure Pipelines successfully started running 3 pipeline(s).

azure-pipelines · 2023-05-02T19:48:07Z

Azure Pipelines successfully started running 3 pipeline(s).

…composition

jakobbotsch · 2023-05-04T11:16:12Z

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

azure-pipelines · 2023-05-04T11:16:29Z

Azure Pipelines successfully started running 3 pipeline(s).

jakobbotsch · 2023-05-04T12:12:30Z

src/coreclr/jit/promotiondecomposition.cpp

+                if (m_compiler->lvaGetDesc(entry.ToLclNum)->lvIsStructField)
+                    UpdateEarlyRefCount(m_compiler, dst);


This looks a bit odd right now because I'm trying to avoid adding any ref counts to the replacement locals created by this pass. So this logic tracks them correctly only for existing locals and relies on forward sub to give up on the physical promotion locals that have 0 ref count.

I will submit a separate PR after this one to correctly track the ref counts much more uniformly for all the IR created by this pass.

jakobbotsch · 2023-05-04T14:33:54Z

src/coreclr/jit/promotiondecomposition.cpp

@@ -24,13 +25,1181 @@ class DecompositionStatementList

        for (GenTree* cur = m_head->gtNext; cur != nullptr; cur = cur->gtNext)
        {
-            tree = comp->gtNewOperNode(GT_COMMA, tree->TypeGet(), cur, tree);
+            tree = comp->gtNewOperNode(GT_COMMA, TYP_VOID, cur, tree);


Physical promotion counterpart to #85320.

jakobbotsch · 2023-05-04T14:49:40Z

The failure is #83658.

cc @dotnet/jit-contrib PTAL @AndyAyersMS... if some of the structure looks a bit unfamiliar it's because I refactored it in #85728.

Diffs with physical promotion enabled.

Diffs with physical promotion enabled and old promotion disabled. I analyzed the regressions in #85323 (comment), there is still quite a lot of potential improvements to be had.

AndyAyersMS

LGTM overall

AndyAyersMS · 2023-05-05T15:39:12Z

src/coreclr/jit/promotiondecomposition.cpp

+                    primitiveType = TYP_INT;
+                    break;
+#endif
+                case TARGET_POINTER_SIZE:


I wonder if you should peel this case out and check for it up front, then just do power of 2 sized case analysis beyond that.

Does alignment matter if there are no GC refs?

I wonder if you should peel this case out and check for it up front, then just do power of 2 sized case analysis beyond that.

Sure, I can do that.

Does alignment matter if there are no GC refs?

AFAIK no, as of recently we do the same trick in block lowering for odd sizes by copying [start..start+8) and [end-8..end).

AndyAyersMS · 2023-05-05T15:44:25Z

src/coreclr/jit/promotiondecomposition.cpp

+        // If the remainder is a full block and is going to incur write barrier
+        // then avoid incurring multiple write barriers for each source
+        // replacement that is a GC pointer -- write them back to the struct
+        // first instead.


I think some of these strategy choices might benefit from having pseudo-IR "pictures" showing what we're trying to avoid/create.

Either here in comments or perhaps in some companion write-up.

Will add a comment on this specific optimization. For completeness:

// If the remainder is a full block and is going to incur write barrier // then avoid incurring multiple write barriers for each source // replacement that is a GC pointer -- write them back to the struct // first instead. That is, instead of: // // ▌ COMMA void // ├──▌ ASG struct (copy) <- write barrier // │ ├──▌ BLK struct<Program+S, 32> // │ │ └──▌ LCL_VAR byref V01 arg1 // │ └──▌ LCL_VAR struct<Program+S, 32> V00 arg0 // └──▌ COMMA void // ├──▌ ASG ref <- write barrier // │ ├──▌ IND ref // │ │ └──▌ ADD byref // │ │ ├──▌ LCL_VAR byref V01 arg1 // │ │ └──▌ CNS_INT long 8 // │ └──▌ LCL_VAR ref V05 tmp3 // └──▌ ASG ref <- write barrier // ├──▌ IND ref // │ └──▌ ADD byref // │ ├──▌ LCL_VAR byref V01 arg1 // │ └──▌ CNS_INT long 24 // └──▌ LCL_VAR ref V06 tmp4 // // Produce: // // ▌ COMMA void // ├──▌ ASG ref <- no write barrier // │ ├──▌ LCL_FLD ref V00 arg0 [+8] // │ └──▌ LCL_VAR ref V05 tmp3 // └──▌ COMMA void // ├──▌ ASG ref <- no write barrier // │ ├──▌ LCL_FLD ref V00 arg0 [+24] // │ └──▌ LCL_VAR ref V06 tmp4 // └──▌ ASG struct (copy) <- write barrier // ├──▌ BLK struct<Program+S, 32> // │ └──▌ LCL_VAR byref V01 arg1 (last use) // └──▌ LCL_VAR struct<Program+S, 32> V00 arg0 //

AndyAyersMS · 2023-05-05T15:48:52Z

src/coreclr/jit/promotiondecomposition.cpp

+                }
+            }
+
+            if (remainderStrategy.Type != RemainderStrategy::NoRemainder)


Wonder if you should represent the number of additional address uses as part of the remainder strategy?

Say someday you decided to support two primitives... I suppose there are enough other changes needed that perhaps this one would not be overlooked.

Hmm, it would probably be reasonable to try to make this more polymorphic, but let me leave it as is for now and see how (if) it ends up abstracting/generalizing in the future to more sophisticated strategies.

AndyAyersMS · 2023-05-05T16:11:42Z

src/coreclr/jit/promotiondecomposition.cpp

+    }
+
+    //------------------------------------------------------------------------
+    // CreateInitValue:


Seems like this belongs over in gentree.cpp

Added a Compiler::gtNewConWithPattern and made regular block morphing use it too.

jakobbotsch · 2023-05-05T19:16:24Z

src/coreclr/jit/promotiondecomposition.cpp

+            if (addr->OperIsLocal() && (!m_dst->OperIs(GT_LCL_VAR, GT_LCL_FLD) ||
+                                        (addr->AsLclVarCommon()->GetLclNum() != m_dst->AsLclVarCommon()->GetLclNum())))


I think this is missing address exposed check, but it seems to be preexisting so let me fix it in a follow-up.

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Apr 25, 2023

ghost assigned jakobbotsch Apr 25, 2023

jakobbotsch commented Apr 25, 2023

View reviewed changes

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

jakobbotsch added 3 commits April 25, 2023 17:59

Handle no promoted field

09db21e

Add test for no promoted field

813fa83

Source may no longer be dying

5a372af

Fix a possible out of bounds in OverlappingReplacements

5e54863

jakobbotsch added 2 commits April 25, 2023 22:59

Remove leftover code

22aa3f7

Handle that source replacements may need a read back

87e55d6

Always read back partially overlapping destination

70defdc

This was referenced Apr 25, 2023

System.Text.RegularExpressions.Tests.AttRegexTests test fails in CI #75808

Closed

Tracking issue for CI build timeouts #76454

Closed

jakobbotsch mentioned this pull request Apr 26, 2023

JIT: Change GTF_ICON_INITCLASS -> GTF_IND_INITCLASS #85396

Merged

jakobbotsch added 2 commits April 26, 2023 15:52

Propagate indir flags

2b7440a

jakobbotsch mentioned this pull request Apr 26, 2023

Fix physical promotion scenario name and a couple of bugs #85343

Merged

jakobbotsch added 2 commits April 26, 2023 16:14

More correct propagation of flags

05c848c

Rename scenarios

58c9d82

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 7 commits May 3, 2023 19:09

Nest some classes to see if it makes github diff work properly

5ad9291

What if I remove this

c7254bf

Extract to a new file

6f782dd

A few fixes

0888806

Remove unnecessary change with GT_FIELD removal

9641dbe

Merge branch 'main' of github.com:dotnet/runtime into more-general-de…

7d96e83

…composition

Fix after merge

bd2021d

build-analysis bot mentioned this pull request May 3, 2023

Test_EventSource_EtwManifestGeneration* tests failing in CI #48798

Closed

jakobbotsch added 3 commits May 4, 2023 12:17

Merge branch 'main' of github.com:dotnet/runtime into more-general-de…

5929d19

…composition

Fix after merge

39428e4

Clean up a bit

c7a9f8d

jakobbotsch commented May 4, 2023

View reviewed changes

jakobbotsch marked this pull request as ready for review May 4, 2023 14:49

jakobbotsch requested a review from AndyAyersMS May 4, 2023 14:49

AndyAyersMS approved these changes May 5, 2023

View reviewed changes

jakobbotsch commented May 5, 2023

View reviewed changes

jakobbotsch added 2 commits May 5, 2023 21:46

Address feedback; factor some code

b346598

Fix bad factoring

d431901

build-analysis bot mentioned this pull request May 5, 2023

IOException running NuGet-Migrations during tests in dotnet CLI first run #80619

Closed

jakobbotsch merged commit 40cad3c into dotnet:main May 6, 2023

jakobbotsch deleted the more-general-decomposition branch May 6, 2023 10:06

ghost locked as resolved and limited conversation to collaborators Jun 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JIT: Generalize assignment decomposition in physical promotion #85323

JIT: Generalize assignment decomposition in physical promotion #85323

jakobbotsch commented Apr 25, 2023 •

edited

Loading

ghost commented Apr 25, 2023

jakobbotsch commented Apr 25, 2023

azure-pipelines bot commented Apr 25, 2023

jakobbotsch commented Apr 25, 2023

azure-pipelines bot commented Apr 25, 2023

jakobbotsch commented Apr 25, 2023

azure-pipelines bot commented Apr 25, 2023

jakobbotsch commented Apr 25, 2023

azure-pipelines bot commented Apr 25, 2023

jakobbotsch commented Apr 26, 2023

azure-pipelines bot commented Apr 26, 2023

azure-pipelines bot commented May 2, 2023

jakobbotsch commented May 4, 2023

azure-pipelines bot commented May 4, 2023

jakobbotsch May 4, 2023

jakobbotsch May 4, 2023

jakobbotsch commented May 4, 2023

AndyAyersMS left a comment

AndyAyersMS May 5, 2023

jakobbotsch May 5, 2023

AndyAyersMS May 5, 2023

jakobbotsch May 5, 2023

AndyAyersMS May 5, 2023

jakobbotsch May 5, 2023

AndyAyersMS May 5, 2023

jakobbotsch May 5, 2023

jakobbotsch May 5, 2023

		if (m_compiler->lvaGetDesc(entry.ToLclNum)->lvIsStructField)
		UpdateEarlyRefCount(m_compiler, dst);

		if (addr->OperIsLocal() && (!m_dst->OperIs(GT_LCL_VAR, GT_LCL_FLD) \|\|
		(addr->AsLclVarCommon()->GetLclNum() != m_dst->AsLclVarCommon()->GetLclNum())))

JIT: Generalize assignment decomposition in physical promotion #85323

JIT: Generalize assignment decomposition in physical promotion #85323

Conversation

jakobbotsch commented Apr 25, 2023 • edited Loading

ghost commented Apr 25, 2023

jakobbotsch commented Apr 25, 2023

azure-pipelines bot commented Apr 25, 2023

jakobbotsch commented Apr 25, 2023

azure-pipelines bot commented Apr 25, 2023

jakobbotsch commented Apr 25, 2023

azure-pipelines bot commented Apr 25, 2023

jakobbotsch commented Apr 25, 2023

azure-pipelines bot commented Apr 25, 2023

jakobbotsch commented Apr 26, 2023

azure-pipelines bot commented Apr 26, 2023

azure-pipelines bot commented May 2, 2023

jakobbotsch commented May 4, 2023

azure-pipelines bot commented May 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakobbotsch commented May 4, 2023

AndyAyersMS left a 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

jakobbotsch commented Apr 25, 2023 •

edited

Loading