Skip to content

Commit 2e0033c

Browse files
jakobbotschjakobbotsch
authored
JIT: Support some assignment decomposition in physical promotion (#85105)
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.
1 parent 6abedb8 commit 2e0033c

File tree

9 files changed

+879
-49
lines changed

9 files changed

+879
-49
lines changed

‎src/coreclr/jit/compiler.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1063,6 +1063,7 @@ class LclVarDsc
10631063
unsigned short lvRefCnt(RefCountState state = RCS_NORMAL) const;
10641064
void incLvRefCnt(unsigned short delta, RefCountState state = RCS_NORMAL);
10651065
void setLvRefCnt(unsigned short newValue, RefCountState state = RCS_NORMAL);
1066+
void incLvRefCntSaturating(unsigned short delta, RefCountState state = RCS_NORMAL);
10661067

10671068
weight_t lvRefCntWtd(RefCountState state = RCS_NORMAL) const;
10681069
void incLvRefCntWtd(weight_t delta, RefCountState state = RCS_NORMAL);
@@ -2944,6 +2945,7 @@ class Compiler
29442945
static bool gtHasRef(GenTree* tree, unsigned lclNum);
29452946

29462947
bool gtHasLocalsWithAddrOp(GenTree* tree);
2948+
bool gtHasAddressExposedLocals(GenTree* tree);
29472949

29482950
unsigned gtSetCallArgsOrder(CallArgs* args, bool lateArgs, int* callCostEx, int* callCostSz);
29492951
unsigned gtSetMultiOpOrder(GenTreeMultiOp* multiOp);

‎src/coreclr/jit/compiler.hpp‎

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4361,10 +4361,9 @@ inline unsigned short LclVarDsc::lvRefCnt(RefCountState state) const
43614361
// Notes:
43624362
// It is currently the caller's responsibility to ensure this increment
43634363
// will not cause overflow.
4364-
4364+
//
43654365
inline void LclVarDsc::incLvRefCnt(unsigned short delta, RefCountState state)
43664366
{
4367-
43684367
#if defined(DEBUG)
43694368
assert(state != RCS_INVALID);
43704369
Compiler* compiler = JitTls::GetCompiler();
@@ -4376,6 +4375,25 @@ inline void LclVarDsc::incLvRefCnt(unsigned short delta, RefCountState state)
43764375
assert(m_lvRefCnt >= oldRefCnt);
43774376
}
43784377

4378+
//------------------------------------------------------------------------------
4379+
// incLvRefCntSaturating: increment reference count for this local var (with saturating semantics)
4380+
//
4381+
// Arguments:
4382+
// delta: the amount of the increment
4383+
// state: the requestor's expected ref count state; defaults to RCS_NORMAL
4384+
//
4385+
inline void LclVarDsc::incLvRefCntSaturating(unsigned short delta, RefCountState state)
4386+
{
4387+
#if defined(DEBUG)
4388+
assert(state != RCS_INVALID);
4389+
Compiler* compiler = JitTls::GetCompiler();
4390+
assert(compiler->lvaRefCountState == state);
4391+
#endif
4392+
4393+
int newRefCnt = m_lvRefCnt + delta;
4394+
m_lvRefCnt = static_cast<unsigned short>(min(USHRT_MAX, newRefCnt));
4395+
}
4396+
43794397
//------------------------------------------------------------------------------
43804398
// setLvRefCnt: set the reference count for this local var
43814399
//

‎src/coreclr/jit/gentree.cpp‎

Lines changed: 53 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2994,8 +2994,6 @@ bool Compiler::gtHasLocalsWithAddrOp(GenTree* tree)
29942994
DoLclVarsOnly = true,
29952995
};
29962996

2997-
bool HasAddrTakenLocal = false;
2998-
29992997
LocalsWithAddrOpVisitor(Compiler* comp) : GenTreeVisitor(comp)
30002998
{
30012999
}
@@ -3005,7 +3003,6 @@ bool Compiler::gtHasLocalsWithAddrOp(GenTree* tree)
30053003
LclVarDsc* varDsc = m_compiler->lvaGetDesc((*use)->AsLclVarCommon());
30063004
if (varDsc->lvHasLdAddrOp || varDsc->IsAddressExposed())
30073005
{
3008-
HasAddrTakenLocal = true;
30093006
return WALK_ABORT;
30103007
}
30113008

@@ -3014,8 +3011,48 @@ bool Compiler::gtHasLocalsWithAddrOp(GenTree* tree)
30143011
};
30153012

30163013
LocalsWithAddrOpVisitor visitor(this);
3017-
visitor.WalkTree(&tree, nullptr);
3018-
return visitor.HasAddrTakenLocal;
3014+
return visitor.WalkTree(&tree, nullptr) == WALK_ABORT;
3015+
}
3016+
3017+
//------------------------------------------------------------------------------
3018+
// gtHasAddressExposedLocal:
3019+
// Check if this tree contains locals with IsAddressExposed() flags set. Does
3020+
// a full tree walk.
3021+
//
3022+
// Paramters:
3023+
// tree - the tree
3024+
//
3025+
// Return Value:
3026+
// True if any sub tree is such a local.
3027+
//
3028+
bool Compiler::gtHasAddressExposedLocals(GenTree* tree)
3029+
{
3030+
struct Visitor : GenTreeVisitor<Visitor>
3031+
{
3032+
enum
3033+
{
3034+
DoPreOrder = true,
3035+
DoLclVarsOnly = true,
3036+
};
3037+
3038+
Visitor(Compiler* comp) : GenTreeVisitor(comp)
3039+
{
3040+
}
3041+
3042+
fgWalkResult PreOrderVisit(GenTree** use, GenTree* user)
3043+
{
3044+
LclVarDsc* varDsc = m_compiler->lvaGetDesc((*use)->AsLclVarCommon());
3045+
if (varDsc->IsAddressExposed())
3046+
{
3047+
return WALK_ABORT;
3048+
}
3049+
3050+
return WALK_CONTINUE;
3051+
}
3052+
};
3053+
3054+
Visitor visitor(this);
3055+
return visitor.WalkTree(&tree, nullptr) == WALK_ABORT;
30193056
}
30203057

30213058
#ifdef DEBUG
@@ -16329,7 +16366,17 @@ bool Compiler::gtSplitTree(
1632916366

1633016367
bool IsValue(const UseInfo& useInf)
1633116368
{
16332-
GenTree* node = (*useInf.Use)->gtEffectiveVal();
16369+
GenTree* node = *useInf.Use;
16370+
16371+
// Some places create void-typed commas that wrap actual values
16372+
// (e.g. VN-based dead store removal), so we need the double check
16373+
// here.
16374+
if (!node->IsValue())
16375+
{
16376+
return false;
16377+
}
16378+
16379+
node = node->gtEffectiveVal();
1633316380
if (!node->IsValue())
1633416381
{
1633516382
return false;

‎src/coreclr/jit/lclmorph.cpp‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1548,10 +1548,7 @@ class LocalAddressVisitor final : public GenTreeVisitor<LocalAddressVisitor>
15481548

15491549
// Note we don't need accurate counts when the values are large.
15501550
//
1551-
if (varDsc->lvRefCnt(RCS_EARLY) < USHRT_MAX)
1552-
{
1553-
varDsc->incLvRefCnt(1, RCS_EARLY);
1554-
}
1551+
varDsc->incLvRefCntSaturating(1, RCS_EARLY);
15551552

15561553
if (!m_compiler->lvaIsImplicitByRefLocal(lclNum))
15571554
{

0 commit comments

Comments
 (0)