Repository navigation
First Class Structs: stop lying about underlying type. #1231
Description
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:
- teach rationalize how to work with these types;
- start generating
STORE_LCL/BLK/OBJin importer, like ingtNewBlkOpNodegentree.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.
- 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
- 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
Activity
CarolEidt commentedon Jan 1, 2020
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 commentedon Jan 2, 2020
Great!
Presumably you mean
ASGstruct trees with a call as RHS? OtherwiseASGstruct trees are definitely expected in morph.Currently I'm working on
ADDR, notASG. Eventually I'll work onASGremoval as well but I need to finishADDRfirst (well, at least the part related to local variables). In particular, I need struct typedLCL_FLDs and I need to get rid ofIsLocalAddrExpr(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 creatingSTORE_LCL|FLD_VARinLocalAddressVisitorfirst, 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.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:
fgMorphCopyBlockcan't promote, VN will produce a "new" VN, assertion prop will just kill all existing assertions related to the LHS etc.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
ASG. If codegen does not support struct typedSTORE_LCL_VARit's lowering's job to convert these toSTORE_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 commentedon Jan 2, 2020
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_VARwork with registers sized structs, without having to convert them toSTORE_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 theLCL_VARand using it inSTORE_LCL_VAR. It also disables reg size struct handling infgMorphOneAsgBlockOp, another form of lying that we should fix eventually.sandreenko commentedon Jan 2, 2020
The current way of representation of struct
ASGin importer involves many nodes likeBLK, ADDR, OBJ:we add conservative
Gflag onBLKin that method:runtime/src/coreclr/src/jit/simd.cpp
Lines 3199 to 3207 in 890b890
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 commentedon Jan 2, 2020
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 commentedon Jan 2, 2020
I have checked my notes and found that asserts were not in the rationalization phase, sorry for the confusion.
But rationalize was transforming
ASG structintoSTORE_BLK structwhen with fixup in importer we hadSTORE_LCL_FLD long.Details
IR without fixup after importation (new):after rationalize:
IR with fixup after importation (old):
after rationalize:
Is
STORE_BLKthe right node here? As I see from Struct “objects” as lvalues it is acceptabe, but don't we want to have a simpleSTORE_LCL_VARin this case?mikedn commentedon Jan 2, 2020
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.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 commentedon Jan 2, 2020
Agreed; we want to move toward using a simple
LCL_VARorSTORE_LCL_VARfor 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 commentedon Jan 2, 2020
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.
[-]First Class Structs: stop lying about underlaing type.[/-][+]First Class Structs: stop lying about underlying type.[/+]sandreenko commentedon Jan 11, 2020
a small update, I was able to compile and run
ConsoleMandel.ScalarFloatSinglethreadADTbenchmark with both mikedn's change forSTORE_LCL_VARand with Carol's change for enregistering SIMD8, I see the expected performance boost:dumps are attached, the difference that
V25 tmp11now can be enregistered.SIMDTestNew.txt
SIMDTestOld.txt
pri0 test results:
the changes are in that branch (but it is not ready for review).
Details
The are many problems, the main are:
then there is interaction with
LocalAddressVisitorthat transforms it into:and then we end up with
so we have
int = structand problems withgtNewTempAssign, that can't determinate if it needs bitcast (fromAllow TYP_SIMD8 to be enregisteredchange).impFixupStructReturnTypeand hits asserts when we inline, working on it now.CarolEidt commentedon Jan 11, 2020
This looks like great progress!
I think the right answer is to delay the introduction of the bitcast to
Lowering. There may be a reasonable interim solution, but it would require adding back ABI details into places like this, which would be nice to avoid.I've made some additional changes to my SIMD8 PR (#811) mostly to improve performance. I haven't done much testing on it at this stage, so it may actually be worse than before correctness-wise.
sandreenko commentedon Jan 21, 2020
a small update from the offline discussion with Carol:
We do not want to do retyping for
return structcase as well. It happens inimpFixupStructReturnTypeand that was missed in my branch.It creates many interesting cases, for example,
System.ValueTuple:Create():System.ValueTuplefrom SPC x64 checked:for such IL:
we are importing:
and then was retyped as:
and after lowering it looked like:
in the new version:
we have
LCL_VAR struct == CNS_INT int 0, so assertion propagation replaces[000004]with[000002], so we get:that we don't know how to generate (no struct handle), so temporary I block such assertion propagation but it looks like we are missing an optimization opportunity here.
mikedn commentedon Jan 21, 2020
FWIW using
CNS_INTlike this has always been a bad idea. It really should be always wrapped inINIT_VAL(which could also haveClassLayout*added to it) or, because default initialized structs values are pretty common, a new oper be created just for this (e.g.GT_INITOBJ).CarolEidt commentedon Jan 21, 2020
Agreed. This was something that was actual made worse by the first round of first class structs changes and I debated but decided not to add the extra node where "not needed" (though I've come to believe that it is, in fact, needed).
That seems like a promising idea; I presume that it would have store semantics, and look a lot like
GT_STORE_OBJbut with no source operand. Given that there's an IL instruction calledinitobjthat takes an arbitrary init value, I might call thisGT_ZEROOBJor something like that.mikedn commentedon Jan 21, 2020
Not really. I think that IL's
initobjsemantics is quite unfortunate and should not be replicated:C# code:
IL code:
ASM code:
While it's possible to improve the JIT to handle this better, things would have been easier if the IL was just:
That's
initblk,initobjalways zeroes out the destination location.sandreenko commentedon Jan 21, 2020
INIT_VAL right now doesn't have
ClassLayout*, does it? It can be easily added if we change its base type fromGenTreeOpto something new withClassLayout*field.However, from what I saw,
INIT_VALnowadays doesn't have an important semantic role, all phases just skip it and access its childer, so as I understand the only purpose of it is to block some optimizations (like Value numbering), and that is why we don't createINIT_VALfor0to keep these optimizations. Am I right or does it have another purpose?As I understand the proposed design is to create a new node for
initobjIL.What about IL like this:
nowadays we generate
ASG(BLOCK, 0), do we want to generateZEROOBJinstead?I like the idea, especially because it can be done in the master branch as a separate PR.
I don't fully understand procs and cons of it; are you expecting that we will be able to generate better code with this new node? Should it survive until lowering or later?
mikedn commentedon Jan 21, 2020
Yes, it's probably easy to add layout to it but it's not very useful ATM because
INIT_VALis not used in the most common case - zero initialization. It's only used when the initialization value is non-constant and those cases are practically non existent, at least in the FX libraries.Yes, that's pretty much why it exists. I think that the more precise reason is that without it the IR semantics might be a bit ambiguous - is
ASG(IND<struct>, IND<int>)filling the LHS with the byte value taken from the RHS or is it copying 4 bytes from the RHS? You can see this inOperIsCopyBlkOpandOperIsInitBlkOpwhere the init/copy distinction is based on the RHS being constant orINIT_VAL.Perhaps, I'm not sure how common
initblkis. I think that the C# compiler doesn't use it but others (e.g. VC++) do. Theinitblk/cpblksituation is unfortunately complicated:store(load)orstore(init)which makes sense only when the number of bytes is constant. When it's not things this load/store model seems pretty weird and unnecessary - you want a load/store model for the sake SSA, VN and others but at the same time you can't really do anything if the number of bytes isn't constant.ClassLayoutobjects without a class handle. I've done that in order to be able to move things forward but it seems weird to me and I think of it as a temporary solution. It may be better ifOBJhas layout andBLKonly has size, though this might complicate a bit code likeLowerBlockStore.ASGat import time, at least not easily. You'd just generateASG(LCL(0), ZEROOBJ)instead ofASG(LCL(0), CNS_INT(0)). Then some form of propagation would replace uses ofLCL(0)with aZEROOBJ, without worrying about generating some kind of invalid IR likeRETURN(CNS_INT(0)).I think that the main problem with adding something like
ZEROOBJis that it would be just an alternate representation ofINIT_VAL(CNS_INT(0)). Having 2 ways to represent the same thing in a compiler IR isn't exactly a bright idea so it may be better to just useINIT_VAL(CNS_INT(0)). The trouble with it is that the IR is bigger. Not a great idea in a JIT, especially when 99.9% of initializations are zero init.As for the main advantage - this is IMO pushing the JIR IR in the right direction by removing a case of context dependent node semantics. From
LCL_VARs nodes that are loads or stores depending on which side ofASGthey are, toSTRUCTtypedINDnodes that cannot be CSEd because you don't know the exact type and toCNS_INTnodes that aren't really integer constants, this kind of context dependency always causes problems.Well, it should avoid the issue you're having with
RETURN(CNS_INT(0))and assertion propagation.Maybe it could also be used to improve the codegen in the example I posted above but I haven't checked what's going on there. That's probably a rather different issue (e.g. it could be that because the temporary created for implicit byref args is address exposed assertion propagation doesn't touch it even though it could in this case).