RyuJIT: Eliminate unnecessary copies after struct fields are p-indep promoted in Frame:CreateResponseHeader(ref,bool):this

[sivarv]

Here is the IR and code generated:

N245 (  1,  1) [000466] ------------      t466 =    const     ref    null REG rcx $VN.Null

                                                 /--*  t466   ref    
N247 (  5,  4) [000468] DA--G-------             *  st.lclVar ref    V57 tmp48        d:3 rbx REG rbx RV

N249 (  3,  2) [000987] -------N----      t987 =    lclVar    ref    V56 tmp47        u:3 rdx (last use) REG rdx RV $2aa

                                                 /--*  t987   ref    
N251 (  7,  5) [000988] DA----------             *  st.lclVar ref    V54 tmp45        d:3 rdx REG rdx RV

N253 (  3,  2) [000990] -------N----      t990 =    lclVar    ref    V57 tmp48        u:3 rbx (last use) REG rbx RV $VN.Null

                                                 /--*  t990   ref    
N255 (  7,  5) [000991] DA----------             *  st.lclVar ref    V55 tmp46        d:3 rcx REG rcx RV

N257 (  3,  2) [000994] -------N----      t994 =    lclVar    ref    V54 tmp45        u:3 rdx (last use) REG rdx RV $2aa

                                                 /--*  t994   ref    
N259 (  7,  5) [000995] DA----------             *  st.lclVar ref    V58 tmp49        d:3 rdx REG rdx RV

N261 (  3,  2) [000997] -------N----      t997 =    lclVar    ref    V55 tmp46        u:3 rcx (last use) REG rcx RV $VN.Null

                                                 /--*  t997   ref    
N263 (  7,  5) [000998] DA----------             *  st.lclVar ref    V59 tmp50        d:3 rbx REG rbx RV

Here are the assignments:
V56 = indir(some const addr)
V57 = null   ; xor rbx, rbx
V54 = V56   ; both get rdx and hence no mov gets generated. V56 last use here
V55 = V57  ;  mov rcx, rbx ; V57 last use here
V58 = V54  ;  both get rdx and hence no mov gets generated. V54 last use here. V58 is used later
V59 = V55  ;  mov rbx, rcx ; V59 was assigned rbx. V59 is used later


33DB                 xor      rbx, rbx
488BCB               mov      rcx, rbx
488BD9               mov      rbx, rcx
48B9385618DF1C010000 mov      rcx, 0x11CDF185638

As you can see we have two separate chains of assignment

1. indir(some const addr) --> V56 --> V54 --> V58
2. null --> V57 --> V55 --> V59

LSRA got its preferencing right for first chain and there were no redundant moves.
In the second chain, it didn't get preferencing right and lead to redundant moves.

If RyuJIT did copy prop we could reduce both chains of assignment to

i) V58 = indir(some constant addr)
ii) V59 = null

In general it is good to avoid these redundant assignments by copy propagation. Prior to rationalization, these assignments were part of a GT_COMMA tree and that could be the reason why copy propagation might have not taken palce?

***** BB10, stmt 21
     (  5,  4) [000469] ------------             *  stmtExpr  void  (IL 0x085...  ???)
N001 (  1,  1) [000466] ------------             |  /--*  const     ref    null $VN.Null
N003 (  5,  4) [000468] -A--G---R---             \--*  =         ref    $VN.Null
N002 (  3,  2) [000467] D------N----                \--*  lclVar    ref    V57 tmp48        d:3 $VN.Null

***** BB10, stmt 22
     ( 14, 10) [000339] ------------             *  stmtExpr  void  (IL   ???...  ???)
N004 (  3,  2) [000990] -------N----             |     /--*  lclVar    ref    V57 tmp48        u:3 (last use) $VN.Null
N006 (  7,  5) [000991] -A------R---             |  /--*  =         ref    $VN.Null
N005 (  3,  2) [000989] D------N----             |  |  \--*  lclVar    ref    V55 tmp46        d:3 $VN.Null
N007 ( 14, 10) [000992] -A----------             \--*  comma     void   $VN.Null
N001 (  3,  2) [000987] -------N----                |  /--*  lclVar    ref    V56 tmp47        u:3 (last use) $2aa
N003 (  7,  5) [000988] -A------R---                \--*  =         ref    $2aa
N002 (  3,  2) [000986] D------N----                   \--*  lclVar    ref    V54 tmp45        d:3 $2aa

***** BB10, stmt 23
     ( 14, 10) [000502] ------------             *  stmtExpr  void  (IL   ???...  ???)
N004 (  3,  2) [000997] -------N----             |     /--*  lclVar    ref    V55 tmp46        u:3 (last use) $VN.Null
N006 (  7,  5) [000998] -A------R---             |  /--*  =         ref    $VN.Null
N005 (  3,  2) [000996] D------N----             |  |  \--*  lclVar    ref    V59 tmp50        d:3 $VN.Null
N007 ( 14, 10) [000999] -A----------             \--*  comma     void   $VN.Null
N001 (  3,  2) [000994] -------N----                |  /--*  lclVar    ref    V54 tmp45        u:3 (last use) $2aa
N003 (  7,  5) [000995] -A------R---                \--*  =         ref    $2aa
N002 (  3,  2) [000993] D------N----                   \--*  lclVar    ref    V58 tmp49        d:3 $2aa

Similar redundant assignments/copies occur at 6 different places within the same method.

category:cq
theme:copy-prop
skill-level:expert
cost:medium

[JosephTremoulet]

Notice the (last use) annotations in the sources of those copies. It turns out optCopyProp refuses to extend lifetimes, for fear of breaking conventional SSA form in the face of pruned PHIs, so it doesn't get these cases.

[JosephTremoulet]

As for the assignment chains that begin with nullptr, I looked to see why constant prop wasn't kicking in. It turns out to be due to some of the GTF_DONT_CSE annotations that are temporarily in place as part of the 1st-class-struct work; commenting out a few of those (already marked with TODO-1stClassStructs comments) enables constant propagation and improves the resulting code sequences, so no new issues are needed to track them (and dotnet/coreclr#7677 may get them).

[JosephTremoulet]

I wonder if there's a reasonable way to adjust the analysis so that crossing a "last use" doesn't disqualify a candidate until the end of the block is reached, to at least get intra-block cases, since the concern is specifically about extending the lifetime across a pruned phi...

[JosephTremoulet]

@schellap, any thoughts on effective ways to extend copy prop to cover more (e.g. intra-block) lifetime-extension cases? I'm wondering if we could do something like have the liveness check use the overload of compUpdateLife that returns the set of last-use vars, and allow propagating uses of those until end-of-block (plus whatever logic would be needed to update the liveness information itself), maybe? Rumor has it you may have put some thought into this :).

[AndyAyersMS]

I suspect the motivating example here is lost, but the general issue still seems relevant, and worth at least a quick screening prototype -- how often do we not copy prop because we worry about over-extending a lifetime?

I could very well be overlooking something here, but it seems like we could freely forward substitute so long as:

• the current SSA def dominates the use (which is a hard requirement anyways) -- the def would reach the use if not killed.
• no other SSA def is dominated by the def and dominates the use -- the def is not killed.

Where we could extend dominates to cover statements in the natural way. Note this extension might be costly since we don't track where (in which statements) the defs actually appear today; we'd have to search for them. As an alternative we could be cautious when more than one of those three are in the same block and likely get many interesting cases with simpler criteria. Or likely defs are quite likely to happen at top level and we can do just a bit of searching and be conservative only for embedded defs.

This approach has the nice property that it does not depend on liveness, so if/when we extended a def's liveness as result of copy prop, we would not need to worry about liveness no longer being accurate.