JIT: Support some assignment decomposition in physical promotion #85105

jakobbotsch · 2023-04-20T13:56:05Z

Add support for directly initializing and copying into replacements
instead of doing a struct local and read back. Physically promoted
struct locals used as sources are still handled conservatively (by first
writing them back to stack, then doing the copy).

For example, for a case like

void Foo()
{
    S s = _field;
    s.A = s.B + 3;
    Consume(s);
}

struct S
{
    public int A, B;
}

We see the following:

STMT00000 ( 0x000[E-] ... 0x006 )
               [000003] -A-XG------                         ▌  ASG       struct (copy)
               [000002] D------N---                         ├──▌  LCL_VAR   struct<Program+S, 8> V01 loc0
               [000001] ---XG------                         └──▌  FIELD     struct Program:_field
               [000000] -----------                            └──▌  LCL_VAR   ref    V00 this          (last use)
Processing block operation [000003] that involves replacements
New statement:
STMT00000 ( 0x000[E-] ... 0x006 )
               [000029] -A-XG------                         ▌  COMMA     int
               [000021] -A-XG------                         ├──▌  ASG       int
               [000015] D------N---                         │  ├──▌  LCL_VAR   int    V03 tmp1
               [000020] ---XG------                         │  └──▌  IND       int
               [000018] -----------                         │     └──▌  ADD       ref
               [000016] -----------                         │        ├──▌  LCL_VAR   ref    V00 this
               [000017] -----------                         │        └──▌  CNS_INT   long   8
               [000028] -A-XG------                         └──▌  ASG       int
               [000022] D------N---                            ├──▌  LCL_VAR   int    V04 tmp2
               [000027] ---XG------                            └──▌  IND       int
               [000025] -----------                               └──▌  ADD       ref
               [000023] -----------                                  ├──▌  LCL_VAR   ref    V00 this
               [000024] -----------                                  └──▌  CNS_INT   long   12

The logic is currently quite rudimentary when it comes to
holes/uncovered parts of the struct. For example, in the above case if
we add another unused field at the end of S then the result is:

STMT00000 ( 0x000[E-] ... 0x006 )
               [000003] -A-XG------                         ▌  ASG       struct (copy)
               [000002] D------N---                         ├──▌  LCL_VAR   struct<Program+S, 12> V01 loc0
               [000001] ---XG------                         └──▌  FIELD     struct Program:_field
               [000000] -----------                            └──▌  LCL_VAR   ref    V00 this          (last use)
Processing block operation [000003] that involves replacements
Struct operation is not fully covered by replaced fields. Keeping struct operation.
New statement:
STMT00000 ( 0x000[E-] ... 0x006 )
               [000030] -A-XG------                         ▌  COMMA     struct
               [000021] -A-XG------                         ├──▌  ASG       int
               [000015] D------N---                         │  ├──▌  LCL_VAR   int    V03 tmp1
               [000020] ---XG------                         │  └──▌  IND       int
               [000018] -----------                         │     └──▌  ADD       ref
               [000016] -----------                         │        ├──▌  LCL_VAR   ref    V00 this
               [000017] -----------                         │        └──▌  CNS_INT   long   8
               [000029] -A-XG------                         └──▌  COMMA     struct
               [000028] -A-XG------                            ├──▌  ASG       int
               [000022] D------N---                            │  ├──▌  LCL_VAR   int    V04 tmp2
               [000027] ---XG------                            │  └──▌  IND       int
               [000025] -----------                            │     └──▌  ADD       ref
               [000023] -----------                            │        ├──▌  LCL_VAR   ref    V00 this
               [000024] -----------                            │        └──▌  CNS_INT   long   12
               [000003] -A-XG------                            └──▌  ASG       struct (copy)
               [000002] D------N---                               ├──▌  LCL_VAR   struct<Program+S, 12> V01 loc0
               [000001] ---XG------                               └──▌  FIELD     struct Program:_field
               [000000] -----------                                  └──▌  LCL_VAR   ref    V00 this

In this case it would be significantly more efficient to copy only the
remainder, which is just a small part of the struct. However, in the
general case it is not easy to predict the most efficient way to do
this, and in some cases we cannot even represent the hole in JIT IR (if
it involves GC pointers), so I have left this for a future change for
now. Liveness should also be beneficial for that as there are many cases
where we would expect the remainder to be dead.

Add support for directly initializing and copying into replacements instead of doing a struct local and read back. Physically promoted struct locals used as sources are still handled conservatively (by first writing them back to stack, then doing the copy). For example, for a case like ``` void Foo() { S s = _field; s.A = s.B + 3; Consume(s); } struct S { public int A, B; } ``` We see the following: ``` STMT00000 ( 0x000[E-] ... 0x006 ) [000003] -A-XG------ ▌ ASG struct (copy) [000002] D------N--- ├──▌ LCL_VAR struct<Program+S, 8> V01 loc0 [000001] ---XG------ └──▌ FIELD struct Program:_field [000000] ----------- └──▌ LCL_VAR ref V00 this (last use) Processing block operation [000003] that involves replacements New statement: STMT00000 ( 0x000[E-] ... 0x006 ) [000029] -A-XG------ ▌ COMMA int [000021] -A-XG------ ├──▌ ASG int [000015] D------N--- │ ├──▌ LCL_VAR int V03 tmp1 [000020] ---XG------ │ └──▌ IND int [000018] ----------- │ └──▌ ADD ref [000016] ----------- │ ├──▌ LCL_VAR ref V00 this [000017] ----------- │ └──▌ CNS_INT long 8 [000028] -A-XG------ └──▌ ASG int [000022] D------N--- ├──▌ LCL_VAR int V04 tmp2 [000027] ---XG------ └──▌ IND int [000025] ----------- └──▌ ADD ref [000023] ----------- ├──▌ LCL_VAR ref V00 this [000024] ----------- └──▌ CNS_INT long 12 ``` The logic is currently quite rudimentary when it comes to holes/uncovered parts of the struct. For example, in the above case if we add another unused field at the end of S then the result is: ``` STMT00000 ( 0x000[E-] ... 0x006 ) [000003] -A-XG------ ▌ ASG struct (copy) [000002] D------N--- ├──▌ LCL_VAR struct<Program+S, 12> V01 loc0 [000001] ---XG------ └──▌ FIELD struct Program:_field [000000] ----------- └──▌ LCL_VAR ref V00 this (last use) Processing block operation [000003] that involves replacements Struct operation is not fully covered by replaced fields. Keeping struct operation. New statement: STMT00000 ( 0x000[E-] ... 0x006 ) [000030] -A-XG------ ▌ COMMA struct [000021] -A-XG------ ├──▌ ASG int [000015] D------N--- │ ├──▌ LCL_VAR int V03 tmp1 [000020] ---XG------ │ └──▌ IND int [000018] ----------- │ └──▌ ADD ref [000016] ----------- │ ├──▌ LCL_VAR ref V00 this [000017] ----------- │ └──▌ CNS_INT long 8 [000029] -A-XG------ └──▌ COMMA struct [000028] -A-XG------ ├──▌ ASG int [000022] D------N--- │ ├──▌ LCL_VAR int V04 tmp2 [000027] ---XG------ │ └──▌ IND int [000025] ----------- │ └──▌ ADD ref [000023] ----------- │ ├──▌ LCL_VAR ref V00 this [000024] ----------- │ └──▌ CNS_INT long 12 [000003] -A-XG------ └──▌ ASG struct (copy) [000002] D------N--- ├──▌ LCL_VAR struct<Program+S, 12> V01 loc0 [000001] ---XG------ └──▌ FIELD struct Program:_field [000000] ----------- └──▌ LCL_VAR ref V00 this ``` In this case it would be significantly more efficient to copy only the remainder, which is just a small part of the struct. However, in the general case it is not easy to predict the most efficient way to do this, and in some cases we cannot even represent the hole in JIT IR (if it involves GC pointers), so I have left this for a future change for now. Liveness should also be beneficial for that as there are many cases where we would expect the remainder to be dead.

ghost · 2023-04-20T13:56:17Z

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

Issue Details

Add support for directly initializing and copying into replacements
instead of doing a struct local and read back. Physically promoted
struct locals used as sources are still handled conservatively (by first
writing them back to stack, then doing the copy).

For example, for a case like

void Foo()
{
    S s = _field;
    s.A = s.B + 3;
    Consume(s);
}

struct S
{
    public int A, B;
}

We see the following:

STMT00000 ( 0x000[E-] ... 0x006 )
               [000003] -A-XG------                         ▌  ASG       struct (copy)
               [000002] D------N---                         ├──▌  LCL_VAR   struct<Program+S, 8> V01 loc0
               [000001] ---XG------                         └──▌  FIELD     struct Program:_field
               [000000] -----------                            └──▌  LCL_VAR   ref    V00 this          (last use)
Processing block operation [000003] that involves replacements
New statement:
STMT00000 ( 0x000[E-] ... 0x006 )
               [000029] -A-XG------                         ▌  COMMA     int
               [000021] -A-XG------                         ├──▌  ASG       int
               [000015] D------N---                         │  ├──▌  LCL_VAR   int    V03 tmp1
               [000020] ---XG------                         │  └──▌  IND       int
               [000018] -----------                         │     └──▌  ADD       ref
               [000016] -----------                         │        ├──▌  LCL_VAR   ref    V00 this
               [000017] -----------                         │        └──▌  CNS_INT   long   8
               [000028] -A-XG------                         └──▌  ASG       int
               [000022] D------N---                            ├──▌  LCL_VAR   int    V04 tmp2
               [000027] ---XG------                            └──▌  IND       int
               [000025] -----------                               └──▌  ADD       ref
               [000023] -----------                                  ├──▌  LCL_VAR   ref    V00 this
               [000024] -----------                                  └──▌  CNS_INT   long   12

The logic is currently quite rudimentary when it comes to
holes/uncovered parts of the struct. For example, in the above case if
we add another unused field at the end of S then the result is:

STMT00000 ( 0x000[E-] ... 0x006 )
               [000003] -A-XG------                         ▌  ASG       struct (copy)
               [000002] D------N---                         ├──▌  LCL_VAR   struct<Program+S, 12> V01 loc0
               [000001] ---XG------                         └──▌  FIELD     struct Program:_field
               [000000] -----------                            └──▌  LCL_VAR   ref    V00 this          (last use)
Processing block operation [000003] that involves replacements
Struct operation is not fully covered by replaced fields. Keeping struct operation.
New statement:
STMT00000 ( 0x000[E-] ... 0x006 )
               [000030] -A-XG------                         ▌  COMMA     struct
               [000021] -A-XG------                         ├──▌  ASG       int
               [000015] D------N---                         │  ├──▌  LCL_VAR   int    V03 tmp1
               [000020] ---XG------                         │  └──▌  IND       int
               [000018] -----------                         │     └──▌  ADD       ref
               [000016] -----------                         │        ├──▌  LCL_VAR   ref    V00 this
               [000017] -----------                         │        └──▌  CNS_INT   long   8
               [000029] -A-XG------                         └──▌  COMMA     struct
               [000028] -A-XG------                            ├──▌  ASG       int
               [000022] D------N---                            │  ├──▌  LCL_VAR   int    V04 tmp2
               [000027] ---XG------                            │  └──▌  IND       int
               [000025] -----------                            │     └──▌  ADD       ref
               [000023] -----------                            │        ├──▌  LCL_VAR   ref    V00 this
               [000024] -----------                            │        └──▌  CNS_INT   long   12
               [000003] -A-XG------                            └──▌  ASG       struct (copy)
               [000002] D------N---                               ├──▌  LCL_VAR   struct<Program+S, 12> V01 loc0
               [000001] ---XG------                               └──▌  FIELD     struct Program:_field
               [000000] -----------                                  └──▌  LCL_VAR   ref    V00 this

In this case it would be significantly more efficient to copy only the
remainder, which is just a small part of the struct. However, in the
general case it is not easy to predict the most efficient way to do
this, and in some cases we cannot even represent the hole in JIT IR (if
it involves GC pointers), so I have left this for a future change for
now. Liveness should also be beneficial for that as there are many cases
where we would expect the remainder to be dead.

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

jakobbotsch · 2023-04-20T14:01:26Z

/azp run runtime-jit-experimental

azure-pipelines · 2023-04-20T14:01:36Z

Azure Pipelines successfully started running 1 pipeline(s).

jakobbotsch · 2023-04-20T17:12:30Z

/azp run runtime-jit-experimental

azure-pipelines · 2023-04-20T17:12:45Z

Azure Pipelines successfully started running 1 pipeline(s).

…ion-decompose-assignments

jakobbotsch · 2023-04-20T22:15:13Z

cc @dotnet/jit-contrib PTAL @AndyAyersMS

This passed jit-experimental.

Diffs with physical promotion.

Diffs without old promotion. Looks like there's a ton of generated regex code that has the same kind of regression in x86 that ends up disproportionally affecting the benchmarks diffs. Appears to be a case where the promoted fields end up without registers, and the moved def of the field appears to no longer be a candidate for having a single spill def location. I will investigate further tomorrow, but this PR should be good to review now (also need to address some formatting issues).

jakobbotsch · 2023-04-20T22:30:38Z

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

azure-pipelines · 2023-04-20T22:30:52Z

Azure Pipelines successfully started running 2 pipeline(s).

AndyAyersMS

Overall I am not clear on how the apparently flow-sensitive values of Replacement.NeedsReadBack and 'Replacement.NeedsWriteBack` are handled. Say a struct with promotions is assigned to differently in then/else and then some promoted piece is accessed below the join point, how does that access know if it needs to be read back?

I suppose the answer is that immediately after each assignment if a read back is needed then you just do it there instead of trying to defer, instead of trying to find "optimal" placement downstream somewhere...?

src/coreclr/jit/promotion.cpp

AndyAyersMS · 2023-04-23T16:13:12Z

src/coreclr/jit/promotion.cpp

+    // Parameters:
+    //   lclNum - the local
+    //
+    void IncrementRefCount(unsigned lclNum)


Share this with local address visitor?

Also the local address visitor's UpdateEarlyRefCount does things with weighted ref counts which would be difficult to duplicate here -- is there any adverse interaction caused by not updating them?

Share this with local address visitor?

Will do.

Also the local address visitor's UpdateEarlyRefCount does things with weighted ref counts which would be difficult to duplicate here -- is there any adverse interaction caused by not updating them?

The ref counts we are updating here are only for "existing" locals that appear under assignments, so I don't think the weighted ref counts are problematic.

The remainder of the pass is not handling the ref counts properly for the locals it introduces, I will submit a separate change to get that sorted out. There won't be an adverse effect of not updating the weighted ref count since we only use that to undo "old" promotion (the other use of them, for omitting some implicit byref copies, was replaced by liveness if you recall).

I introduced an LclVarDsc::incLvRefCntSaturating and used it from both. Didn't seem too easy to factor the rest of the promotion-related common logic due to the weighted ref-count logic you mentioned.

src/coreclr/jit/promotion.cpp

AndyAyersMS · 2023-04-23T16:26:31Z

src/coreclr/jit/promotion.cpp

+            {
+                JITDUMP("Copy [%06u] is between two physically promoted locals with replacements\n",
+                        Compiler::dspTreeID(asg));
+                JITDUMP("*** Conservative: Phys<->phys copies not yet supported; inserting conservative write-back\n");


Is this case uncommon or something you want to work on later?

Would it be interesting to check for self assignment or do we just not see those?

I want to handle sources in an upcoming change (I have most of this code written). Will keep in mind to add a check for self assignments then.

AndyAyersMS · 2023-04-23T16:36:33Z

src/coreclr/jit/promotion.cpp

+                if (!covered)
+                {
+                    JITDUMP("Struct operation is not fully covered by replaced fields. Keeping struct operation.\n");
+                    result.AddStatement(asg);


I think I am confused here -- it seems like result already accumulated IR for the handled cases and now we're appending a full copy, but some of the copied bits may be stale if we didn't write them back to the source -- but we know not to read from the corresponding bits in the destination?

If so maybe expand on the comment below to say "For handled replacements, we must not read back..."

The copied bits won't be stale here since this PR always (conservatively) writes the source replacements back to its struct local to begin with, but yes, for the replacements that were handled we won't read them back from the struct local due to the loop below, so it wouldn't matter even if they were.

Once we get to handling sources this "covering" copy will need to happen before the copying of the source replacements since otherwise we would overwrite the fresh bits (from the source's replacements) with stale bits (from the source's struct local).

jakobbotsch · 2023-04-23T17:07:40Z

Overall I am not clear on how the apparently flow-sensitive values of Replacement.NeedsReadBack and 'Replacement.NeedsWriteBack` are handled. Say a struct with promotions is assigned to differently in then/else and then some promoted piece is accessed below the join point, how does that access know if it needs to be read back?

I suppose the answer is that immediately after each assignment if a read back is needed then you just do it there instead of trying to defer, instead of trying to find "optimal" placement downstream somewhere...?

Currently the invariant is that at the entrance to all BBs the replacement locals always contain the freshest value. So there is "resolution" done at the end of each BB to make that so if NeedsReadBack is true:

runtime/src/coreclr/jit/promotion.cpp

Lines 1146 to 1170 in 2c9e483

    
           for (unsigned i = 0; i < numLocals; i++) 
        
           { 
        
               if (replacements[i] == nullptr) 
        
               { 
        
                   continue; 
        
               } 
        
               for (Replacement& rep : *replacements[i]) 
        
               { 
        
                   assert(!rep.NeedsReadBack || !rep.NeedsWriteBack); 
        
                   if (rep.NeedsReadBack) 
        
                   { 
        
                       JITDUMP("Reading back replacement V%02u.[%03u..%03u) -> V%02u at the end of " FMT_BB "\n", i, 
        
                               rep.Offset, rep.Offset + genTypeSize(rep.AccessType), rep.LclNum, bb->bbNum); 
        
                       GenTree* dst = m_compiler->gtNewLclvNode(rep.LclNum, rep.AccessType); 
        
                       GenTree* src = m_compiler->gtNewLclFldNode(i, rep.AccessType, rep.Offset); 
        
                       GenTree* asg = m_compiler->gtNewAssignNode(dst, src); 
        
                       m_compiler->fgInsertStmtNearEnd(bb, m_compiler->fgNewStmtFromTree(asg)); 
        
                       rep.NeedsReadBack = false; 
        
                   } 
        
                   rep.NeedsWriteBack = true; 
        
               } 
        
           }

There's definitely more clever stuff we can do to optimize the insertion of the read backs/write backs.

jakobbotsch · 2023-04-24T11:34:22Z

/azp run runtime-coreclr superpmi-replay

azure-pipelines · 2023-04-24T11:34:36Z

Azure Pipelines successfully started running 1 pipeline(s).

jakobbotsch · 2023-04-24T13:34:26Z

Looks like there's a ton of generated regex code that has the same kind of regression in x86 that ends up disproportionally affecting the benchmarks diffs. Appears to be a case where the promoted fields end up without registers, and the moved def of the field appears to no longer be a candidate for having a single spill def location.

#85251 is an attempt at fixing the problem and resolves the regression for the collection:

[15:31:32] 10,193 contexts with diffs (9,045 improvements, 522 regressions, 626 same size)
[15:31:32]   -46,981/+7,885 bytes

…ion-decompose-assignments

jakobbotsch · 2023-04-25T13:57:24Z

New superpmi-diffs runs with #85251.

Diffs with physical promotion

Diffs with physical promotion without regular promotion

jakobbotsch added 6 commits April 20, 2023 15:18

Add some tests

563a858

Fix a bug

2ff0212

Run jit-format

11dc7da

Cleanup

26bd089

Proper address type

5d2a80e

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 20, 2023

ghost assigned jakobbotsch Apr 20, 2023

Fix non-windows build

833738a

jakobbotsch marked this pull request as ready for review April 20, 2023 21:28

jakobbotsch added 2 commits April 20, 2023 23:29

Run jit-format

551f367

Merge branch 'main' of github.com:dotnet/runtime into physical-promot…

128f2e7

…ion-decompose-assignments

jakobbotsch requested a review from AndyAyersMS April 20, 2023 22:15

Clean up

4153186

build-analysis bot mentioned this pull request Apr 21, 2023

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

Closed

jakobbotsch added 3 commits April 21, 2023 11:22

Expand gtSplitTree's IsValue check

e0fe581

Ensure some tests run

863e293

Fix some missing read backs

20af8ee

AndyAyersMS reviewed Apr 23, 2023

View reviewed changes

Address feedback

12ba7f5

jakobbotsch mentioned this pull request Apr 24, 2023

JIT: Allow spill-at-single-def for pure defs #85251

Merged

AndyAyersMS approved these changes Apr 24, 2023

View reviewed changes

Merge branch 'main' of github.com:dotnet/runtime into physical-promot…

13b373c

…ion-decompose-assignments

jakobbotsch mentioned this pull request Apr 25, 2023

Promote (scalar replace) structs with more than 4 fields #6534

Closed

jakobbotsch merged commit 2e0033c into dotnet:main Apr 25, 2023

jakobbotsch deleted the physical-promotion-decompose-assignments branch April 25, 2023 13:57

This was referenced Apr 26, 2023

Assertion failed 'block->lastNode()->OperGet() == GT_RETFILT' during 'Generate code' (IL size 1480; hash 0x46e9aa75; FullOpts) #85088

Closed

JIT: Generalized struct promotion #76928

Closed

ghost locked as resolved and limited conversation to collaborators May 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JIT: Support some assignment decomposition in physical promotion #85105

JIT: Support some assignment decomposition in physical promotion #85105

jakobbotsch commented Apr 20, 2023

ghost commented Apr 20, 2023

jakobbotsch commented Apr 20, 2023

azure-pipelines bot commented Apr 20, 2023

jakobbotsch commented Apr 20, 2023

azure-pipelines bot commented Apr 20, 2023

jakobbotsch commented Apr 20, 2023

jakobbotsch commented Apr 20, 2023

azure-pipelines bot commented Apr 20, 2023

AndyAyersMS left a comment

AndyAyersMS Apr 23, 2023

jakobbotsch Apr 23, 2023

jakobbotsch Apr 23, 2023

AndyAyersMS Apr 23, 2023

jakobbotsch Apr 23, 2023

AndyAyersMS Apr 23, 2023

jakobbotsch Apr 23, 2023

jakobbotsch commented Apr 23, 2023

jakobbotsch commented Apr 24, 2023

azure-pipelines bot commented Apr 24, 2023

jakobbotsch commented Apr 24, 2023

jakobbotsch commented Apr 25, 2023

JIT: Support some assignment decomposition in physical promotion #85105

JIT: Support some assignment decomposition in physical promotion #85105

Conversation

jakobbotsch commented Apr 20, 2023

ghost commented Apr 20, 2023

jakobbotsch commented Apr 20, 2023

azure-pipelines bot commented Apr 20, 2023

jakobbotsch commented Apr 20, 2023

azure-pipelines bot commented Apr 20, 2023

jakobbotsch commented Apr 20, 2023

jakobbotsch commented Apr 20, 2023

azure-pipelines bot commented Apr 20, 2023

AndyAyersMS left a comment

Choose a reason for hiding this comment

AndyAyersMS Apr 23, 2023

Choose a reason for hiding this comment

jakobbotsch Apr 23, 2023

Choose a reason for hiding this comment

jakobbotsch Apr 23, 2023

Choose a reason for hiding this comment

AndyAyersMS Apr 23, 2023

Choose a reason for hiding this comment

jakobbotsch Apr 23, 2023

Choose a reason for hiding this comment

AndyAyersMS Apr 23, 2023

Choose a reason for hiding this comment

jakobbotsch Apr 23, 2023

Choose a reason for hiding this comment

jakobbotsch commented Apr 23, 2023

jakobbotsch commented Apr 24, 2023

azure-pipelines bot commented Apr 24, 2023

jakobbotsch commented Apr 24, 2023

jakobbotsch commented Apr 25, 2023