First Class Structs: stop lying about underlying type.

[sandreenko]

I am trying to get rid of some pessimizations following first-class-structs design, cc @CarolEidt.

The main performance issue is struct returns that are wrapped as int or long types. In these cases even if we inline these calls we can't promote/enregister structs or their fields.

To stop retyping we need to stop calling impFixupCallStructReturn and fgFixupStructReturn, like this.

but then you will get errors in rationalize because it doesn't expect ASG struct trees.
I see two possible solutions:

1. teach rationalize how to work with these types;
2. start generating STORE_LCL/BLK/OBJ in importer, like in gtNewBlkOpNode gentree.cpp.

the second interferes with the work of deleting ASG from @mikedn, so that issue is created to discuss how we can make progress without creating merge conflicts. @mikedn how do you see ASG nodes replacement, are you planning to delete it completely, including importation phase?

An example how we keep struct type with the second approach

1. System.IO.FileSystem:FillAttributeInfo(System.String,byref,bool):int (MethodHash=333db0b0) (from VectorAdd_r.dll)

for such IL:

    [ 0]  10 (0x00a) call 0600018E
    [ 1]  15 (0x00f) stloc.1

the old generates:

STMT00004 (IL   ???...  ???)
    [000013] -AC---------              *  ASG       long  
    [000012] *-----------              +--*  IND       long  
    [000011] ------------              |  \--*  ADDR      byref 
    [000010] ------------              |     \--*  LCL_VAR   struct<System.IO.DisableMediaInsertionPrompt, 8> V04 loc1         
    [000009] --C---------              \--*  RET_EXPR  long  (inl return from call [000008])

then transforms it to:

N003 ( 18, 10) [000013] DACXG-------              *  STORE_LCL_FLD long   V04 loc1         [+0]
                                                  *    bool   V04._disableSuccess (offs=0x00) -> V15 tmp8         
                                                  *    int    V04._oldMode (offs=0x04) -> V16 tmp9  

and then generates:
mov qword ptr [V04 rbp-290H], rax

the new importer:

    [000012] -AC---------              *  STORE_LCL_VAR    struct<System.IO.DisableMediaInsertionPrompt, 8> V04 loc1         
    [000009] --C---------              \--*  RET_EXPR  struct(inl return from call [000008])

then:

N001 ( 14,  5) [000008] --CXG-------         t8 =    CALL      struct System.IO.DisableMediaInsertionPrompt.Create
                                                  /--*  t8     struct 
N003 ( 18,  8) [000012] DA-XG-------              *  STORE_LCL_VAR struct<System.IO.DisableMediaInsertionPrompt, 

then:
mov qword ptr [V04 rbp-29CH], rax

so we can keep STRUCT type and with changes in lowering can enregister them.

Other examples.

2. from VectorAddTest`1[Single][System.Single]:VectorAdd(float,float,float):int (MethodHash=051d27ab) (from VectorAdd_r.dll)

for such IL:

    [ 0]   1 (0x001) ldloca.s 0
    [ 1]   3 (0x003) ldarg.0
    [ 2]   4 (0x004) call 0A00002B

the old importer generates:

STMT00002 (IL 0x001...  ???)
               [000007] -A--G-------              *  ASG       simd32 (copy)
               [000006] ----G--N----              +--*  BLK       simd32<32>
               [000003] ------------              |  \--*  ADDR      byref 
               [000002] ------------              |     \--*  LCL_VAR   simd32<System.Numerics.Vector`1[Single]> V03 loc0         
               [000005] -------N----              \--*  SIMD      simd32 float init
               [000004] ------------                 \--*  LCL_VAR   float  V00 arg0 

then rationalize changes it to:

    N001 (  3,  4) [000004] ------------         t4 =    LCL_VAR   float  V00 arg0         
                                                      /--*  t4     float  
    N002 (  4,  5) [000005] -------N----         t5 = *  SIMD      simd32 float init
                                                      /--*  t5     simd32 
    N004 (  8,  8) [000007] DA--G-------              *  STORE_LCL_VAR simd32<System.Numerics.Vector`1[Single]> V03 

and the final result is:

    Generating: N030 (  3,  4) [000004] ------------         t4 =    LCL_VAR   float  V00 arg0          mm0 REG mm0
    IN0005:        vmovss   xmm0, dword ptr [V00 rbp+10H]
                                                                  /--*  t4     float  
    Generating: N032 (  4,  5) [000005] -------N----         t5 = *  SIMD      simd32 float init REG mm0
    IN0006:        vbroadcastss ymm0, ymm0
                                                                  /--*  t5     simd32 
    Generating: N034 (  8,  8) [000007] DA--G-------              *  STORE_LCL_VAR simd32<System.Numerics.Vector`1[Single]> V03 loc0          NA REG NA
    IN0007:        vmovupd  ymmword ptr[V03 rbp-30H], ymm0
    Added IP mapping: 0x0009 STACK_EMPTY (G_M55380_IG04,ins#4,ofs#19)

the new importer generates:

    [000006] ------------              *  STORE_LCL_VAR simd32<System.Numerics.Vector`1[Single]> V03 loc0         
    [000005] -------N----                 *  SIMD      simd32 float init
    [000004] ------------                 \--*  LCL_VAR   float  V00 arg0 

and doesn't do any tranformations later.
in order to keep STORE_LCL_VAR add DA flags.

3. System.IO.FileSystem:FillAttributeInfo(System.String,byref,bool):int (MethodHash=333db0b0) (from VectorAdd_r.dll)

for such IL:

    [ 0]  98 (0x062) ldloca.s 2
    [ 1] 100 (0x064) initobj 0200000C

the old importer:

    [000102] IA----------              *  ASG       struct (init)
    [000099] D------N----              +--*  LCL_VAR   struct<WIN32_FIND_DATA, 592> V05 loc2         
    [000101] ------------              \--*  CNS_INT   int    0

then

N001 (  1,  1) [000101] ------------       t101 =    CNS_INT   int    0
N002 (  3,  2) [000099] D------N----        t99 =    LCL_VAR_ADDR byref  V05 loc2         
                                                  /--*  t99    byref  
                                                  +--*  t101   int    
               [000410] -A----------              *  STORE_BLK struct<592> (init)

the new importer generates:

    [000101] -A----------              *  STORE_BLK struct<592> (init)
    [000098] ------------              +--*  LCL_VAR_ADDR byref  V05 loc2         
    [000100] ------------              \--*  CNS_INT   int    0

4. VectorMathTests.Program:Main(System.String[]):int (MethodHash=2c61fab2) (from AbsSqrt_r.dll)

IL:

    [ 0]   1 (0x001) ldloca.s 0
    [ 1]   3 (0x003) ldc.r4 11.000000000000000
    [ 2]   8 (0x008) ldc.r4 13.000000000000000
    [ 3]  13 (0x00d) ldc.r4 8.0000000000000000
    [ 4]  18 (0x012) ldc.r4 4.0000000000000000
    [ 5]  23 (0x017) call 0A000004

old import:

    [000014] -A--G-------              *  ASG       simd16 (copy)
    [000013] ----G--N----              +--*  BLK       simd16<16>
    [000003] ------------              |  \--*  ADDR      byref 
    [000002] ------------              |     \--*  LCL_VAR   simd16<System.Numerics.Vector4> V01 loc0         
    [000012] -------N----              \--*  SIMD      simd16 float initN
    [000011] ------------                 \--*  LIST      float 
    [000004] ------------                    +--*  CNS_DBL   float  11.000000000000000
    [000010] ------------                    \--*  LIST      float 
    [000005] ------------                       +--*  CNS_DBL   float  13.000000000000000
    [000009] ------------                       \--*  LIST      float 
    [000006] ------------                          +--*  CNS_DBL   float  8.0000000000000000
    [000008] ------------                          \--*  LIST      float 
    [000007] ------------                             \--*  CNS_DBL   float  4.0000000000000000

after rationalize:

    N001 (  3,  4) [000004] ------------         t4 =    CNS_DBL   float  11.000000000000000
    N002 (  3,  4) [000005] ------------         t5 =    CNS_DBL   float  13.000000000000000
    N003 (  3,  4) [000006] ------------         t6 =    CNS_DBL   float  8.0000000000000000
    N004 (  3,  4) [000007] ------------         t7 =    CNS_DBL   float  4.0000000000000000
                                                      /--*  t4     float  
                                                      +--*  t5     float  
                                                      +--*  t6     float  
                                                      +--*  t7     float  
    N009 ( 16, 20) [000012] -------N----        t12 = *  SIMD      simd16 float initN
                                                      /--*  t12    simd16 
    N011 ( 20, 23) [000014] DA--G-------              *  STORE_LCL_VAR simd16<System.Numerics.Vector4> V01 loc0 

new import:

   [000013] DA----------              *  STORE_LCL_VAR simd16<System.Numerics.Vector4> V01 loc0         
   [000012] -------N----                 *  SIMD      simd16 float initN
   [000011] ------------                 \--*  LIST      float 
   [000004] ------------                    +--*  CNS_DBL   float  11.000000000000000
   [000010] ------------                    \--*  LIST      float 
   [000005] ------------                       +--*  CNS_DBL   float  13.000000000000000
   [000009] ------------                       \--*  LIST      float 
   [000006] ------------                          +--*  CNS_DBL   float  8.0000000000000000
   [000008] ------------                          \--*  LIST      float 
   [000007] ------------                             \--*  CNS_DBL   float  4.0000000000000000

category:cq
theme:structs
skill-level:expert
cost:large

[CarolEidt]

I think the first approach (teach rationalize how to deal with struct ASG trees) is the right one. Are there advantages to doing the second? I know that eventually we want to import assignments as stores, but I guess I don't understand the complexity of handling struct assignments in rationalizer.

[mikedn]

I am trying to get rid of some pessimizations following first-class-structs design,

Great!

but then you will get errors in rationalize because it doesn't expect ASG struct trees.

Presumably you mean ASG struct trees with a call as RHS? Otherwise ASG struct trees are definitely expected in morph.

the second interferes with the work of deleting ASG from @mikedn

Currently I'm working on ADDR, not ASG. Eventually I'll work on ASG removal as well but I need to finish ADDR first (well, at least the part related to local variables). In particular, I need struct typed LCL_FLDs and I need to get rid of IsLocalAddrExpr (or at least limit its use to non-tracked variables to avoid having to deal with indirect local stores in SSA). Also, I'm considering first creating STORE_LCL|FLD_VAR in LocalAddressVisitor first, just to limit the scope of change a bit. But this definitely has to be done in the importer sooner or later, it's a rather sad state of affairs that the IL has the correct model (load/store) and the importer has to replace that with the assignment model.

start generating STORE_LCL/BLK/OBJ in importer

That's tempting. But it may turn out to be a large amount of work, relative to the problem you're trying to solve. Nothing in the frontend understands these stores so you'll very likely need to patch up at least morph, assertion prop, SSA and VN. Granted, in the specific case of a call as RHS things should be simpler: fgMorphCopyBlock can't promote, VN will produce a "new" VN, assertion prop will just kill all existing assertions related to the LHS etc.

teach rationalize how to work with these types

I'm still not sure what exact problem you have found in rationalization. There's this bit of code:

runtime/src/coreclr/src/jit/rationalize.cpp

Lines 356 to 397 in c4f221c

	if ((location->TypeGet() == TYP_STRUCT) && !assignment->IsPhiDefn() && !value->IsMultiRegCall())
	{
	if ((location->OperGet() == GT_LCL_VAR))
	{
	// We need to construct a block node for the location.
	// Modify lcl to be the address form.
	location->SetOper(addrForm(locationOp));
	LclVarDsc* varDsc = &(comp->lvaTable[location->AsLclVarCommon()->GetLclNum()]);
	location->gtType = TYP_BYREF;
	GenTreeBlk* storeBlk = nullptr;
	unsigned int size = varDsc->lvExactSize;
	if (varDsc->HasGCPtr())
	{
	CORINFO_CLASS_HANDLE structHnd = varDsc->lvVerTypeInfo.GetClassHandle();
	GenTreeObj* objNode = comp->gtNewObjNode(structHnd, location);
	objNode->ChangeOper(GT_STORE_OBJ);
	objNode->SetData(value);
	storeBlk = objNode;
	}
	else
	{
	storeBlk = new (comp, GT_STORE_BLK)
	GenTreeBlk(GT_STORE_BLK, TYP_STRUCT, location, value, comp->typGetBlkLayout(size));
	}
	storeBlk->gtFlags |= GTF_ASG;
	storeBlk->gtFlags |= ((location->gtFlags | value->gtFlags) & GTF_ALL_EFFECT);
	GenTree* insertionPoint = location->gtNext;
	BlockRange().InsertBefore(insertionPoint, storeBlk);
	use.ReplaceWith(comp, storeBlk);
	BlockRange().Remove(assignment);
	JITDUMP("After transforming local struct assignment into a block op:\n");
	DISPTREERANGE(BlockRange(), use.Def());
	JITDUMP("\n");
	return;
	}
	else
	{
	assert(location->OperIsBlk());
	}
	}

that I'm not quite sure why it's in rationalization. Rationalization's role is to get rid of ASG. If codegen does not support struct typed STORE_LCL_VAR it's lowering's job to convert these to STORE_OBJ.
In any case, this rationalization code already ignores ASGs with a "multi reg call" as RHS so presumably you can easily extend this to "any reg call".

[mikedn]

I happen to have a little change I've done a while ago that may be of some help. It's not related to calls but it tries to make STORE_LCL_VAR work with registers sized structs, without having to convert them to STORE_OBJ. It's only an experiment right now but I think that's going in the write direction - better handling of register sized structs.

mikedn@81c5d91

It only handles the STORE_LCL_VAR(LCL_VAR) case now, by allocating a suitable register (GPR or XMM) to the LCL_VAR and using it in STORE_LCL_VAR. It also disables reg size struct handling in fgMorphOneAsgBlockOp, another form of lying that we should fix eventually.

[sandreenko]

I think the first approach (teach rationalize how to deal with struct ASG trees) is the right one. Are there advantages to doing the second? I know that eventually we want to import assignments as stores, but I guess I don't understand the complexity of handling struct assignments in rationalizer.

The current way of representation of struct ASG in importer involves many nodes like BLK, ADDR, OBJ:

               [000007] -A--G-------              *  ASG       simd32 (copy)
               [000006] ----G--N----              +--*  BLK       simd32<32>
               [000003] ------------              |  \--*  ADDR      byref 
               [000002] ------------              |     \--*  LCL_VAR   simd32<System.Numerics.Vector`1[Single]> V03 loc0  

we add conservative G flag on BLK in that method:

runtime/src/coreclr/src/jit/simd.cpp

Lines 3199 to 3207 in 890b890

	// At this point, we have a tree that we are going to store into a destination.
	// TODO-1stClassStructs: This should be a simple store or assignment, and should not require
	// GTF_ALL_EFFECT for the dest. This is currently emulating the previous behavior of
	// block ops.
	if (doCopyBlk)
	{
	GenTree* dest = new (this, GT_BLK)
	GenTreeBlk(GT_BLK, simdType, copyBlkDst, typGetBlkLayout(getSIMDTypeSizeInBytes(clsHnd)));
	dest->gtFlags |= GTF_GLOB_REF;

and it affects the later phases, probably, that can be fixed directly by deleting that setting as the comment says, but I think these nodes have negative impact on other phases before rationalize (like optOptimizeCSEs, optOptimizeValnumCSEs), am I wrong?

[CarolEidt]

The conservative handling of struct copies is something that needs to change. Some of these pessimizations were removed, only to be re-added later as an expedient, rather than fixing CSE. I'm not certain that's the case in this instance, but I think we should pursue this avenue.

[sandreenko]

but I guess I don't understand the complexity of handling struct assignments in rationalizer.

that I'm not quite sure why it's in rationalization. Rationalization's role is to get rid of ASG. If codegen does not support struct typed STORE_LCL_VAR it's lowering's job to convert these to STORE_OBJ.
In any case, this rationalization code already ignores ASGs with a "multi reg call" as RHS so presumably you can easily extend this to "any reg call".

I have checked my notes and found that asserts were not in the rationalization phase, sorry for the confusion.

But rationalize was transforming ASG struct into STORE_BLK struct when with fixup in importer we had STORE_LCL_FLD long.

IR without fixup after importation (new):

N003 ( 18,  8) [000012] -ACXG---R---              *  ASG       struct (copy) $VN.Void
N002 (  3,  2) [000010] D------N----              +--*  LCL_VAR   struct<System.IO.DisableMediaInsertionPrompt, 8>(P) V04 loc1         
                                                  +--*    bool   V04._disableSuccess (offs=0x00) -> V15 tmp8         
                                                  +--*    int    V04._oldMode (offs=0x04) -> V16 tmp9         
N001 ( 14,  5) [000008] --CXG-------              \--*  CALL      struct System.IO.DisableMediaInsertionPrompt.Create 

after rationalize:

N001 ( 14,  5) [000008] --CXG-------         t8 =    CALL      struct System.IO.DisableMediaInsertionPrompt.Create $240
N002 (  3,  2) [000010] D------N----        t10 =    LCL_VAR_ADDR byref  V04 loc1         
                                                  *    bool   V04._disableSuccess (offs=0x00) -> V15 tmp8         
                                                  *    int    V04._oldMode (offs=0x04) -> V16 tmp9         
                                                  /--*  t10    byref  
                                                  +--*  t8     struct 
               [000402] -ACXG-------              *  STORE_BLK struct<8> (copy)

IR with fixup after importation (old):

               [000013] -AC---------              *  ASG       long  
               [000012] *-----------              +--*  IND       long  
               [000011] ------------              |  \--*  ADDR      byref 
               [000010] ------------              |     \--*  LCL_VAR   struct<System.IO.DisableMediaInsertionPrompt, 8> V04 loc1         
               [000009] --C---------              \--*  RET_EXPR  long  (inl return from call [000008])

after rationalize:

N027 ( 14,  5) [000008] --CXG-------         t8 =    CALL      long   System.IO.DisableMediaInsertionPrompt.Create REG rax
                                                  /--*  t8     long   
N029 ( 18, 10) [000013] DA-XG-------              *  STORE_LCL_FLD long   V04 loc1         [+0] NA

Is STORE_BLK the right node here? As I see from Struct “objects” as lvalues it is acceptabe, but don't we want to have a simple STORE_LCL_VAR in this case?

[mikedn]

But rationalize was transforming ASG struct into STORE_BLK struct when with fixup in importer we had STORE_LCL_FLD long.

Yes, because copy/init block codegen does not support struct typed STORE_LCL|FLD_VAR. That's IMO something that should be improved, but it's probably a bit complicated due to the complexity of the struct copying/initialization code. The copy/init block codegen refactoring I did recently should help but it's not clear to me what benefit we would get from making such a change at this time.

Is STORE_BLK the right node here? As I see from Struct “objects” as lvalues it is acceptabe, but don't we want to have a simple STORE_LCL_VAR in this case?

IMO yes, it should be STORE_LCL_VAR. That's exactly what I did in the commit mentioned a few posts above but only in some limited circumstances.

[CarolEidt]

Agreed; we want to move toward using a simple LCL_VAR or STORE_LCL_VAR for struct typed locals in general, but for codegen, as @mikedn points out, this requires more changes for handling larger structs (i.e. those that require more than a single load or store).

[sandreenko]

Thanks, that was a very helpful discussion for me. I will stop doing changes before rationalizing (except deletion of retyping), adopt @mikedn's change and come back with results.