Skip to content

Commit e01d8f5

Browse files
jakobbotschjakobbotsch
authored
JIT: Refactor CSE candidate storage slightly (dotnet#116009)
CSE candidates had separate storage for the first candidate and subsequent candidates. This introduces some awkwardness when adding the second candidate to the list. Refactor things so that the first candidate is stored inline in `CSEdsc` as a normal list entry.
1 parent 49387ef commit e01d8f5

File tree

2 files changed

+101
-117
lines changed

2 files changed

+101
-117
lines changed

‎src/coreclr/jit/optcse.cpp‎

Lines changed: 99 additions & 111 deletions
Original file line numberDiff line numberDiff line change
@@ -298,8 +298,8 @@ bool Compiler::optCSE_canSwap(GenTree* op1, GenTree* op2)
298298
/* static */
299299
bool Compiler::optCSEcostCmpEx::operator()(const CSEdsc* dsc1, const CSEdsc* dsc2)
300300
{
301-
GenTree* exp1 = dsc1->csdTree;
302-
GenTree* exp2 = dsc2->csdTree;
301+
GenTree* exp1 = dsc1->csdTreeList.tslTree;
302+
GenTree* exp2 = dsc2->csdTreeList.tslTree;
303303

304304
auto expCost1 = exp1->GetCostEx();
305305
auto expCost2 = exp2->GetCostEx();
@@ -334,8 +334,8 @@ bool Compiler::optCSEcostCmpEx::operator()(const CSEdsc* dsc1, const CSEdsc* dsc
334334
/* static */
335335
bool Compiler::optCSEcostCmpSz::operator()(const CSEdsc* dsc1, const CSEdsc* dsc2)
336336
{
337-
GenTree* exp1 = dsc1->csdTree;
338-
GenTree* exp2 = dsc2->csdTree;
337+
GenTree* exp1 = dsc1->csdTreeList.tslTree;
338+
GenTree* exp2 = dsc2->csdTreeList.tslTree;
339339

340340
auto expCost1 = exp1->GetCostSz();
341341
auto expCost2 = exp2->GetCostSz();
@@ -434,7 +434,7 @@ void CSEdsc::ComputeNumLocals(Compiler* compiler)
434434
};
435435

436436
LocalCountingVisitor lcv(compiler);
437-
lcv.WalkTree(&csdTree, nullptr);
437+
lcv.WalkTree(&csdTreeList.tslTree, nullptr);
438438

439439
numDistinctLocals = lcv.m_count;
440440
numLocalOccurrences = lcv.m_occurrences;
@@ -615,99 +615,85 @@ unsigned Compiler::optValnumCSE_Index(GenTree* tree, Statement* stmt)
615615

616616
for (hashDsc = optCSEhash[hval]; hashDsc; hashDsc = hashDsc->csdNextInBucket)
617617
{
618-
if (hashDsc->csdHashKey == key)
618+
if (hashDsc->csdHashKey != key)
619619
{
620-
// Check for mismatched types on GT_CNS_INT nodes
621-
if ((tree->OperGet() == GT_CNS_INT) && (tree->TypeGet() != hashDsc->csdTree->TypeGet()))
622-
{
623-
continue;
624-
}
620+
continue;
621+
}
622+
623+
assert(hashDsc->csdTreeList.tslTree != nullptr);
625624

626-
treeStmtLst* newElem;
625+
// Check for mismatched types on GT_CNS_INT nodes
626+
if ((tree->OperGet() == GT_CNS_INT) && (tree->TypeGet() != hashDsc->csdTreeList.tslTree->TypeGet()))
627+
{
628+
continue;
629+
}
627630

628-
// Have we started the list of matching nodes?
631+
// Have we started the list of matching nodes?
629632

630-
if (hashDsc->csdTreeList == nullptr)
633+
if (hashDsc->csdTreeList.tslNext == nullptr)
634+
{
635+
// This is the second time we see this value. Handle cases
636+
// where the first value dominates the second one and we can
637+
// already prove that the first one is _not_ going to be a
638+
// valid def for the second one, due to the second one having
639+
// more exceptions. This happens for example in code like
640+
// CASTCLASS(x, y) where the "CASTCLASS" just adds exceptions
641+
// on top of "x". In those cases it is always better to let the
642+
// second value be the def.
643+
// It also happens for GT_COMMA, but that one is special cased
644+
// above; this handling is a less special-casey version of the
645+
// GT_COMMA handling above. However, it is quite limited since
646+
// it only handles the def/use being in the same block.
647+
if (compCurBB == hashDsc->csdTreeList.tslBlock)
631648
{
632-
// This is the second time we see this value. Handle cases
633-
// where the first value dominates the second one and we can
634-
// already prove that the first one is _not_ going to be a
635-
// valid def for the second one, due to the second one having
636-
// more exceptions. This happens for example in code like
637-
// CASTCLASS(x, y) where the "CASTCLASS" just adds exceptions
638-
// on top of "x". In those cases it is always better to let the
639-
// second value be the def.
640-
// It also happens for GT_COMMA, but that one is special cased
641-
// above; this handling is a less special-casey version of the
642-
// GT_COMMA handling above. However, it is quite limited since
643-
// it only handles the def/use being in the same block.
644-
if (compCurBB == hashDsc->csdBlock)
649+
GenTree* prevTree = hashDsc->csdTreeList.tslTree;
650+
ValueNum prevVnLib = prevTree->GetVN(VNK_Liberal);
651+
if (prevVnLib != vnLib)
645652
{
646-
GenTree* prevTree = hashDsc->csdTree;
647-
ValueNum prevVnLib = prevTree->GetVN(VNK_Liberal);
648-
if (prevVnLib != vnLib)
653+
ValueNum prevExceptionSet = vnStore->VNExceptionSet(prevVnLib);
654+
ValueNum curExceptionSet = vnStore->VNExceptionSet(vnLib);
655+
if ((prevExceptionSet != curExceptionSet) &&
656+
vnStore->VNExcIsSubset(curExceptionSet, prevExceptionSet))
649657
{
650-
ValueNum prevExceptionSet = vnStore->VNExceptionSet(prevVnLib);
651-
ValueNum curExceptionSet = vnStore->VNExceptionSet(vnLib);
652-
if ((prevExceptionSet != curExceptionSet) &&
653-
vnStore->VNExcIsSubset(curExceptionSet, prevExceptionSet))
654-
{
655-
JITDUMP("Skipping CSE candidate for tree [%06u]; tree [%06u] is a better candidate with "
656-
"more exceptions\n",
657-
prevTree->gtTreeID, tree->gtTreeID);
658-
prevTree->gtCSEnum = 0;
659-
hashDsc->csdStmt = stmt;
660-
hashDsc->csdTree = tree;
661-
tree->gtCSEnum = (signed char)hashDsc->csdIndex;
662-
return hashDsc->csdIndex;
663-
}
658+
JITDUMP("Skipping CSE candidate for tree [%06u]; tree [%06u] is a better candidate with "
659+
"more exceptions\n",
660+
prevTree->gtTreeID, tree->gtTreeID);
661+
prevTree->gtCSEnum = 0;
662+
hashDsc->csdTreeList.tslStmt = stmt;
663+
hashDsc->csdTreeList.tslTree = tree;
664+
tree->gtCSEnum = (signed char)hashDsc->csdIndex;
665+
return hashDsc->csdIndex;
664666
}
665667
}
666-
667-
// Create the new element based upon the matching hashDsc element.
668-
669-
newElem = new (this, CMK_TreeStatementList) treeStmtLst;
670-
671-
newElem->tslTree = hashDsc->csdTree;
672-
newElem->tslStmt = hashDsc->csdStmt;
673-
newElem->tslBlock = hashDsc->csdBlock;
674-
newElem->tslNext = nullptr;
675-
676-
/* Start the list with the first CSE candidate recorded */
677-
678-
hashDsc->csdTreeList = newElem;
679-
hashDsc->csdTreeLast = newElem;
680-
681-
hashDsc->csdIsSharedConst = isSharedConst;
682668
}
683669

684-
noway_assert(hashDsc->csdTreeList);
670+
hashDsc->csdIsSharedConst = isSharedConst;
671+
}
685672

686-
/* Append this expression to the end of the list */
673+
// Append this expression to the end of the list
687674

688-
newElem = new (this, CMK_TreeStatementList) treeStmtLst;
675+
treeStmtLst* newElem = new (this, CMK_TreeStatementList) treeStmtLst;
689676

690-
newElem->tslTree = tree;
691-
newElem->tslStmt = stmt;
692-
newElem->tslBlock = compCurBB;
693-
newElem->tslNext = nullptr;
677+
newElem->tslTree = tree;
678+
newElem->tslStmt = stmt;
679+
newElem->tslBlock = compCurBB;
680+
newElem->tslNext = nullptr;
694681

695-
hashDsc->csdTreeLast->tslNext = newElem;
696-
hashDsc->csdTreeLast = newElem;
682+
hashDsc->csdTreeLast->tslNext = newElem;
683+
hashDsc->csdTreeLast = newElem;
697684

698-
optDoCSE = true; // Found a duplicate CSE tree
685+
optDoCSE = true; // Found a duplicate CSE tree
699686

700-
/* Have we assigned a CSE index? */
701-
if (hashDsc->csdIndex == 0)
702-
{
703-
newCSE = true;
704-
break;
705-
}
706-
707-
assert(FitsIn<signed char>(hashDsc->csdIndex));
708-
tree->gtCSEnum = ((signed char)hashDsc->csdIndex);
709-
return hashDsc->csdIndex;
687+
/* Have we assigned a CSE index? */
688+
if (hashDsc->csdIndex == 0)
689+
{
690+
newCSE = true;
691+
break;
710692
}
693+
694+
assert(FitsIn<signed char>(hashDsc->csdIndex));
695+
tree->gtCSEnum = ((signed char)hashDsc->csdIndex);
696+
return hashDsc->csdIndex;
711697
}
712698

713699
if (!newCSE)
@@ -763,10 +749,12 @@ unsigned Compiler::optValnumCSE_Index(GenTree* tree, Statement* stmt)
763749
hashDsc->defExcSetPromise = vnStore->VNForEmptyExcSet();
764750
hashDsc->defExcSetCurrent = vnStore->VNForNull(); // uninit value
765751

766-
hashDsc->csdTree = tree;
767-
hashDsc->csdStmt = stmt;
768-
hashDsc->csdBlock = compCurBB;
769-
hashDsc->csdTreeList = nullptr;
752+
hashDsc->csdTreeList.tslTree = tree;
753+
hashDsc->csdTreeList.tslStmt = stmt;
754+
hashDsc->csdTreeList.tslBlock = compCurBB;
755+
hashDsc->csdTreeList.tslNext = nullptr;
756+
757+
hashDsc->csdTreeLast = &hashDsc->csdTreeList;
770758

771759
/* Append the entry to the hash bucket */
772760

@@ -801,11 +789,11 @@ unsigned Compiler::optValnumCSE_Index(GenTree* tree, Statement* stmt)
801789
hashDsc->csdIndex = CSEindex;
802790

803791
/* Update the gtCSEnum field in the original tree */
804-
noway_assert(hashDsc->csdTreeList->tslTree->gtCSEnum == 0);
792+
noway_assert(hashDsc->csdTreeList.tslTree->gtCSEnum == 0);
805793
assert(FitsIn<signed char>(CSEindex));
806794

807-
hashDsc->csdTreeList->tslTree->gtCSEnum = ((signed char)CSEindex);
808-
noway_assert(((unsigned)hashDsc->csdTreeList->tslTree->gtCSEnum) == CSEindex);
795+
hashDsc->csdTreeList.tslTree->gtCSEnum = ((signed char)CSEindex);
796+
noway_assert(((unsigned)hashDsc->csdTreeList.tslTree->gtCSEnum) == CSEindex);
809797

810798
tree->gtCSEnum = ((signed char)CSEindex);
811799

@@ -975,7 +963,7 @@ void Compiler::optValnumCSE_InitDataFlow()
975963
{
976964
CSEdsc* dsc = optCSEtab[inx];
977965
unsigned CSEindex = dsc->csdIndex;
978-
treeStmtLst* lst = dsc->csdTreeList;
966+
treeStmtLst* lst = &dsc->csdTreeList;
979967
noway_assert(lst);
980968

981969
while (lst != nullptr)
@@ -1101,13 +1089,13 @@ void Compiler::optValnumCSE_SetUpAsyncByrefKills()
11011089
CSEdsc* dsc = optCSEtab[inx - 1];
11021090
assert(dsc->csdIndex == inx);
11031091
bool isByRef = false;
1104-
if (dsc->csdTree->TypeIs(TYP_BYREF))
1092+
if (dsc->csdTreeList.tslTree->TypeIs(TYP_BYREF))
11051093
{
11061094
isByRef = true;
11071095
}
1108-
else if (dsc->csdTree->TypeIs(TYP_STRUCT))
1096+
else if (dsc->csdTreeList.tslTree->TypeIs(TYP_STRUCT))
11091097
{
1110-
ClassLayout* layout = dsc->csdTree->GetLayout(this);
1098+
ClassLayout* layout = dsc->csdTreeList.tslTree->GetLayout(this);
11111099
isByRef = layout->HasGCByRef();
11121100
}
11131101

@@ -2528,14 +2516,14 @@ void CSE_HeuristicParameterized::GetFeatures(CSEdsc* cse, double* features)
25282516
return;
25292517
}
25302518

2531-
const unsigned char costEx = cse->csdTree->GetCostEx();
2519+
const unsigned char costEx = cse->csdTreeList.tslTree->GetCostEx();
25322520
const double deMinimis = 1e-3;
25332521
const double deMinimusAdj = -log(deMinimis);
25342522

25352523
features[0] = costEx;
25362524
features[1] = deMinimusAdj + log(max(deMinimis, cse->csdUseWtCnt));
25372525
features[2] = deMinimusAdj + log(max(deMinimis, cse->csdDefWtCnt));
2538-
features[3] = cse->csdTree->GetCostSz();
2526+
features[3] = cse->csdTreeList.tslTree->GetCostSz();
25392527
features[4] = cse->csdUseCount;
25402528
features[5] = cse->csdDefCount;
25412529

@@ -2545,9 +2533,9 @@ void CSE_HeuristicParameterized::GetFeatures(CSEdsc* cse, double* features)
25452533
const bool isLiveAcrossCall = cse->csdLiveAcrossCall;
25462534

25472535
features[6] = booleanScale * isLiveAcrossCall;
2548-
features[7] = booleanScale * varTypeUsesIntReg(cse->csdTree->TypeGet());
2536+
features[7] = booleanScale * varTypeUsesIntReg(cse->csdTreeList.tslTree->TypeGet());
25492537

2550-
const bool isConstant = cse->csdTree->OperIsConst();
2538+
const bool isConstant = cse->csdTreeList.tslTree->OperIsConst();
25512539
const bool isSharedConstant = cse->csdIsSharedConst;
25522540

25532541
features[8] = booleanScale * (isConstant & !isSharedConstant);
@@ -2573,7 +2561,7 @@ void CSE_HeuristicParameterized::GetFeatures(CSEdsc* cse, double* features)
25732561
unsigned maxPostorderNum = 0;
25742562
BasicBlock* minPostorderBlock = nullptr;
25752563
BasicBlock* maxPostorderBlock = nullptr;
2576-
for (treeStmtLst* treeList = cse->csdTreeList; treeList != nullptr; treeList = treeList->tslNext)
2564+
for (treeStmtLst* treeList = &cse->csdTreeList; treeList != nullptr; treeList = treeList->tslNext)
25772565
{
25782566
BasicBlock* const treeBlock = treeList->tslBlock;
25792567
unsigned postorderNum = treeBlock->bbPostorderNum;
@@ -2602,12 +2590,12 @@ void CSE_HeuristicParameterized::GetFeatures(CSEdsc* cse, double* features)
26022590

26032591
// More
26042592
//
2605-
features[17] = booleanScale * ((cse->csdTree->gtFlags & GTF_CALL) != 0);
2593+
features[17] = booleanScale * ((cse->csdTreeList.tslTree->gtFlags & GTF_CALL) != 0);
26062594
features[18] = deMinimusAdj + log(max(deMinimis, cse->csdUseCount * cse->csdUseWtCnt));
26072595
features[19] = deMinimusAdj + log(max(deMinimis, cse->numLocalOccurrences * cse->csdUseWtCnt));
26082596
features[20] = booleanScale * ((double)(blockSpread) / numBBs);
26092597

2610-
const bool isContainable = cse->csdTree->OperIs(GT_ADD, GT_NOT, GT_MUL, GT_LSH);
2598+
const bool isContainable = cse->csdTreeList.tslTree->OperIs(GT_ADD, GT_NOT, GT_MUL, GT_LSH);
26112599
features[21] = booleanScale * isContainable;
26122600
features[22] = booleanScale * (isContainable && isLowCost);
26132601

@@ -3172,7 +3160,7 @@ void CSE_HeuristicRLHook::GetFeatures(CSEdsc* cse, int* features)
31723160
unsigned maxPostorderNum = 0;
31733161
BasicBlock* minPostorderBlock = nullptr;
31743162
BasicBlock* maxPostorderBlock = nullptr;
3175-
for (treeStmtLst* treeList = cse->csdTreeList; treeList != nullptr; treeList = treeList->tslNext)
3163+
for (treeStmtLst* treeList = &cse->csdTreeList; treeList != nullptr; treeList = treeList->tslNext)
31763164
{
31773165
BasicBlock* const treeBlock = treeList->tslBlock;
31783166
unsigned postorderNum = treeBlock->bbPostorderNum;
@@ -3232,13 +3220,13 @@ void CSE_HeuristicRLHook::GetFeatures(CSEdsc* cse, int* features)
32323220
features[i++] = type;
32333221
features[i++] = cse->IsViable() ? 1 : 0;
32343222
features[i++] = cse->csdLiveAcrossCall ? 1 : 0;
3235-
features[i++] = cse->csdTree->OperIsConst() ? 1 : 0;
3223+
features[i++] = cse->csdTreeList.tslTree->OperIsConst() ? 1 : 0;
32363224
features[i++] = cse->csdIsSharedConst ? 1 : 0;
32373225
features[i++] = isMakeCse ? 1 : 0;
3238-
features[i++] = ((cse->csdTree->gtFlags & GTF_CALL) != 0) ? 1 : 0;
3239-
features[i++] = cse->csdTree->OperIs(GT_ADD, GT_NOT, GT_MUL, GT_LSH) ? 1 : 0;
3240-
features[i++] = cse->csdTree->GetCostEx();
3241-
features[i++] = cse->csdTree->GetCostSz();
3226+
features[i++] = ((cse->csdTreeList.tslTree->gtFlags & GTF_CALL) != 0) ? 1 : 0;
3227+
features[i++] = cse->csdTreeList.tslTree->OperIs(GT_ADD, GT_NOT, GT_MUL, GT_LSH) ? 1 : 0;
3228+
features[i++] = cse->csdTreeList.tslTree->GetCostEx();
3229+
features[i++] = cse->csdTreeList.tslTree->GetCostSz();
32423230
features[i++] = cse->csdUseCount;
32433231
features[i++] = cse->csdDefCount;
32443232
features[i++] = (int)cse->csdUseWtCnt;
@@ -4271,7 +4259,7 @@ void CSE_Heuristic::SortCandidates()
42714259
for (unsigned cnt = 0; cnt < m_pCompiler->optCSECandidateCount; cnt++)
42724260
{
42734261
CSEdsc* dsc = sortTab[cnt];
4274-
GenTree* expr = dsc->csdTree;
4262+
GenTree* expr = dsc->csdTreeList.tslTree;
42754263

42764264
weight_t def;
42774265
weight_t use;
@@ -4281,13 +4269,13 @@ void CSE_Heuristic::SortCandidates()
42814269
{
42824270
def = dsc->csdDefCount; // def count
42834271
use = dsc->csdUseCount; // use count (excluding the implicit uses at defs)
4284-
cost = dsc->csdTree->GetCostSz();
4272+
cost = dsc->csdTreeList.tslTree->GetCostSz();
42854273
}
42864274
else
42874275
{
42884276
def = dsc->csdDefWtCnt; // weighted def count
42894277
use = dsc->csdUseWtCnt; // weighted use count (excluding the implicit uses at defs)
4290-
cost = dsc->csdTree->GetCostEx();
4278+
cost = dsc->csdTreeList.tslTree->GetCostEx();
42914279
}
42924280

42934281
if (!Compiler::Is_Shared_Const_CSE(dsc->csdHashKey))
@@ -4840,7 +4828,7 @@ void CSE_HeuristicCommon::PerformCSE(CSE_Candidate* successfulCandidate)
48404828
ValueNum bestVN = ValueNumStore::NoVN;
48414829
bool bestIsDef = false;
48424830
ssize_t bestConstValue = 0;
4843-
treeStmtLst* lst = dsc->csdTreeList;
4831+
treeStmtLst* lst = &dsc->csdTreeList;
48444832

48454833
while (lst != nullptr)
48464834
{
@@ -4931,7 +4919,7 @@ void CSE_HeuristicCommon::PerformCSE(CSE_Candidate* successfulCandidate)
49314919
}
49324920
else // !isSharedConst
49334921
{
4934-
lst = dsc->csdTreeList;
4922+
lst = &dsc->csdTreeList;
49354923
GenTree* firstTree = lst->tslTree;
49364924
printf("In %s, CSE (oper = %s, type = %s) has differing VNs: ", m_pCompiler->info.compFullName,
49374925
GenTree::OpName(firstTree->OperGet()), varTypeName(firstTree->TypeGet()));
@@ -4956,7 +4944,7 @@ void CSE_HeuristicCommon::PerformCSE(CSE_Candidate* successfulCandidate)
49564944
ArrayStack<UseDefLocation> defUses(m_pCompiler->getAllocator(CMK_CSE));
49574945

49584946
// First process the defs.
4959-
for (lst = dsc->csdTreeList; lst != nullptr; lst = lst->tslNext)
4947+
for (lst = &dsc->csdTreeList; lst != nullptr; lst = lst->tslNext)
49604948
{
49614949
GenTree* const exp = lst->tslTree;
49624950
Statement* const stmt = lst->tslStmt;
@@ -5065,7 +5053,7 @@ void CSE_HeuristicCommon::PerformCSE(CSE_Candidate* successfulCandidate)
50655053
}
50665054

50675055
// Now process the actual uses.
5068-
for (lst = dsc->csdTreeList; lst != nullptr; lst = lst->tslNext)
5056+
for (lst = &dsc->csdTreeList; lst != nullptr; lst = lst->tslNext)
50695057
{
50705058
GenTree* const exp = lst->tslTree;
50715059
Statement* const stmt = lst->tslStmt;

0 commit comments

Comments
 (0)