JIT: Put all CSEs into SSA - #106637

Merged
jakobbotsch merged 29 commits into
dotnet:mainfrom
jakobbotsch:ssa-updating
Nov 7, 2024
Merged

JIT: Put all CSEs into SSA#106637
jakobbotsch merged 29 commits into
dotnet:mainfrom
jakobbotsch:ssa-updating

Conversation

@jakobbotsch

@jakobbotschjakobbotsch commented Aug 19, 2024•
edited
Loading

Copy link
Copy Markdown
Member

This adds an SSA updater that can incrementally put locals that were added to the IR into SSA form and starts using this infrastructure from CSE. Previously only single-def CSEs were put into SSA, which can cause e.g. IV opts to miss out on them.

The SSA updater requires all uses and definitions to be supplied. Based on the definitions it is then possible to compute the candidate blocks for phi nodes in the usual way as the iterated dominance frontier. Since we do not have liveness for the local we do not insert the phi definitions eagerly; instead, we recursively compute the reaching def for each use by walking its dominators.
Walking the dominators we either expect to find a real definition, or to hit a block that is part of the iterated dominance frontier of the definitions, in which case we know we have a live phi definition that we can insert.

Inserting phi definitions needs to recursively do the same thing for the phi arguments. To make this faster we could memoize the reaching SSA numbers for each block, though this is not currently done.

This also requires computing the DFS tree, dominators, and dominance frontiers after RBO (which invalidates those). Since the DFS tree and dominator tree is usually computed anyway by IV opts, this is not that costly.
The cost is further reduced by avoiding any of these computations in the single-def case (which CSE already handled). This special case is also moved into the SSA updater.

Finally, since IV opts need liveness to work, the SSA inserter also computes liveness for the inserted locals. This is done by marking all paths from each use back to its reaching definition as having that local live-in.

Fix#109412

@jakobbotsch
JIT: Put all CSEs into SSA
ddf2b02
This adds an SSA updater that can incrementally put locals that were
added to the IR into SSA form and starts using this infrastructure from
CSE. It updates CSE to use this SSA updater to put all CSEs into SSA.
Previously only single-def CSEs were put into SSA, which can cause e.g.
IV opts to miss out on them.
The SSA updater requires all uses and definitions to be supplied. Based
on the definitions it is then possible to compute the candidate blocks
for phi nodes in the usual way as the iterated dominance frontier. Since
we do not have liveness for the local we do not insert the phi
definitions eagerly; instead, we recursively compute the reaching def
for each use by walking its dominators.
Walking the dominators we either expect to find a real definition, or to
hit a block that is part of the iterated dominance frontier of the
definitions, in which case we know we have a live phi definition that we
can insert.
Inserting phi definitions needs to recursively do the same thing for the
phi arguments. To make this faster we could memoize the reaching SSA
numbers for each block, though this is not currently done.
This also requires computing the DFS tree, dominators, and dominance
frontiers after RBO (which invalidates those). The DFS tree and
dominators should be reusable by IV opts in most cases, though currently
we invalidate them after CSE because assertion prop sometimes will
change the flow graph and does not know how to invalidate the DFS tree.
This can also be fixed to improve throughput.
We currently do not get much use out of this because IV opts also need
liveness for the IVs. More specifically it needs to know whether the IVs
are live out of the loop; it should be possible to keep some breadcrumbs
around for inserted locals to cheaply know this, I think.
@ghostghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 19, 2024
@jakobbotsch

This comment was marked as outdated.

Sign in to view
This was referenced Aug 19, 2024
Closed
Closed
@jakobbotsch

jakobbotsch commented Aug 20, 2024

Copy link
Copy Markdown
MemberAuthor

TP diffs for benchmarks.run_pgo now look like:

Base:120833007777, Diff:120935018667, +0.0844%317810259:NA:25.72%:+0.2630%:public: void __cdecl FlowGraphDominanceFrontiers::ComputeIteratedDominanceFrontier(struct BasicBlock*, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>*) 162099515:NA:13.12%:+0.1342%:public: static classFlowGraphDominanceFrontiers* __cdecl FlowGraphDominanceFrontiers::Build(classFlowGraphDominatorTree*) 39047720:NA:3.16%:+0.0323%:private: static void __cdecl SsaBuilder::AddNewPhiArg(classCompiler*, struct BasicBlock*, struct Statement*, struct GenTreePhi*, unsigned int, unsigned int, struct BasicBlock*) 38346141:NA:3.10%:+0.0317%:public: bool __cdecl Compiler::fgMorphBlockStmt(struct BasicBlock*, struct Statement*, bool) 18905583:+52.64%:1.53%:+0.0156%:public: virtual void __cdecl CSE_HeuristicCommon::PerformCSE(classCSE_Candidate*) 13835238:NA:1.12%:+0.0114%:private: static struct Statement* __cdecl SsaBuilder::InsertPhi(classCompiler*, struct BasicBlock*, unsigned int) 8753827:+0.85%:0.71%:+0.0072%:public: __cdecl `public: unsigned int __cdecl Compiler::fgRunDfs<class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_1>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_2>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_3>, 0>(class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_1>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_2>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_3>)'::`2'::<lambda_1>::operator()(struct BasicBlock*) const
7221209:+0.28%:0.58%:+0.0060%:public: void * __cdecl ArenaAllocator::allocateMemory(unsigned __int64) 7119522:+2.54%:0.58%:+0.0059%:public: static classFlowGraphDominatorTree* __cdecl FlowGraphDominatorTree::Build(classFlowGraphDfsTree const *) 6499831:+7.25%:0.53%:+0.0054%:private: bool __cdecl jitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>::ensure_capacity(unsigned __int64) 4424130:+12.56%:0.36%:+0.0037%:private: void __cdecl JitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, struct FlowEdge*, classCompAllocator, classJitHashTableBehavior>::Grow(void) 3705888:NA:0.30%:+0.0031%:public: static void __cdecl SsaBuilder::InsertInSsa(classCompiler*, unsigned int, classArrayStack<struct UseDefLocation> &, classArrayStack<struct UseDefLocation> &) 3361499:NA:0.27%:+0.0028%:private: struct UseDefLocation __cdecl IncrementalSsaBuilder::FindOrCreateReachingDef(struct UseDefLocation const &) 2892505:+0.42%:0.23%:+0.0024%:public: unsigned short __cdecl Compiler::optAddAssertion(struct Compiler::AssertionDsc*) 2651134:+0.69%:0.21%:+0.0022%:BasicBlock::VisitAllSuccs<`AllSuccessorEnumerator::AllSuccessorEnumerator'::`2'::<lambda_1>>2535856:+4.24%:0.21%:+0.0021%:public: struct FlowEdge* __cdecl Compiler::BlockPredsWithEH(struct BasicBlock*) 1743270:NA:0.14%:+0.0014%:public: void __cdecl IncrementalSsaBuilder::Insert(void) 1693606:+0.58%:0.14%:+0.0014%:public: void __cdecl RangeCheck::MergeEdgeAssertions(unsigned int, unsigned __int64 *const &, struct Range*) 1583199:NA:0.13%:+0.0013%:private: bool __cdecl IncrementalSsaBuilder::FindReachingDefInBlock(struct UseDefLocation const &, struct BasicBlock*, struct UseDefLocation*) 1557652:+1.89%:0.13%:+0.0013%:public: void __cdecl Compiler::fgValueNumberPhiDef(struct GenTreeLclVar*, struct BasicBlock*, bool) 1349037:+0.75%:0.11%:+0.0011%:VisitEHSuccs<0,`AllSuccessorEnumerator::AllSuccessorEnumerator'::`2'::<lambda_1> >1261895:+0.82%:0.10%:+0.0010%: `Compiler::optReachable'::`12'::<lambda_1>::operator() -5032126:-0.49%:0.41%:-0.0042%: memset -26356661:-50.03%:2.13%:-0.0218%:private: void __cdecl SsaBuilder::AddPhiArg(struct BasicBlock*, struct Statement*, struct GenTreePhi*, unsigned int, unsigned int, struct BasicBlock*) -30940686:-15.02%:2.50%:-0.0256%:private: void __cdecl SsaBuilder::InsertPhiFunctions(void) -36618036:-100.00%:2.96%:-0.0303%:public: bool __cdecl Compiler::fgMorphBlockStmt(struct BasicBlock*, struct Statement*) -139925973:-100.00%:11.32%:-0.1158%:private: void __cdecl SsaBuilder::ComputeDominanceFrontiers(struct BasicBlock**, int, classJitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>, classCompAllocator, classJitHashTableBehavior>*) -322436319:-100.00%:26.09%:-0.2668%:private: void __cdecl SsaBuilder::ComputeIteratedDominanceFrontier(struct BasicBlock*, classJitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>, classCompAllocator, classJitHashTableBehavior> const *, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>*)

So looks small enough that we can take this change without much more consideration.

One thing is whether we need to memoize reaching defs in some way to avoid quadratic (or even potentially exponential) behavior. Need to think about that a bit.

@jakobbotschjakobbotsch added this to the 10.0.0 milestone Aug 20, 2024
@build-analysisbuild-analysisBot mentioned this pull request Aug 20, 2024
Closed
@jakobbotsch

jakobbotsch commented Aug 21, 2024

Copy link
Copy Markdown
MemberAuthor

benchmarks.run_pgo seems to be one of the least affected collections. The most affected one is benchmarks.run. With some manual reconciliation of renamed functions in the diff, the detailed TP diff for it looks like:

Base:40469192366, Diff:40551805661, +0.2041%16221652:+33.98%:13.58%:+0.0401%:private: void __cdecl SsaBuilder::ComputeDominanceFrontiers(struct BasicBlock**, int, classJitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>, classCompAllocator, classJitHashTableBehavior>*) 14025772:+48.54%:11.74%:+0.0347%:public: virtual void __cdecl CSE_HeuristicCommon::PerformCSE(classCSE_Candidate*) 11802368:NA:9.88%:+0.0292%:private: static void __cdecl SsaBuilder::AddNewPhiArg(classCompiler*, struct BasicBlock*, struct Statement*, struct GenTreePhi*, unsigned int, unsigned int, struct BasicBlock*) 6227765:+2.88%:5.21%:+0.0154%:public: __cdecl `public: unsigned int __cdecl Compiler::fgRunDfs<class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_1>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_2>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_3>, 0>(class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_1>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_2>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_3>)'::`2'::<lambda_1>::operator()(struct BasicBlock*) const
4914583:+0.60%:4.11%:+0.0121%:public: void * __cdecl ArenaAllocator::allocateMemory(unsigned __int64) 4651869:+6.52%:3.89%:+0.0115%:public: static classFlowGraphDominatorTree* __cdecl FlowGraphDominatorTree::Build(classFlowGraphDfsTree const *) 4369172:+17.68%:3.66%:+0.0108%:private: bool __cdecl jitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>::ensure_capacity(unsigned __int64) 3469086:NA:2.90%:+0.0086%:private: static struct Statement* __cdecl SsaBuilder::InsertPhi(classCompiler*, struct BasicBlock*, unsigned int) 2793726:NA:2.34%:+0.0069%:public: static void __cdecl SsaBuilder::InsertInSsa(classCompiler*, unsigned int, classArrayStack<struct UseDefLocation> &, classArrayStack<struct UseDefLocation> &) 2747882:NA:2.30%:+0.0068%:private: struct UseDefLocation __cdecl IncrementalSsaBuilder::FindOrCreateReachingDef(struct UseDefLocation const &) 2745600:+25.93%:2.30%:+0.0068%:private: void __cdecl JitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, struct FlowEdge*, classCompAllocator, classJitHashTableBehavior>::Grow(void) 2538778:+2.19%:2.13%:+0.0063%:public: void __cdecl RangeCheck::MergeEdgeAssertions(unsigned int, unsigned __int64 *const &, struct Range*) 2129497:+12.45%:1.78%:+0.0053%:public: struct FlowEdge* __cdecl Compiler::BlockPredsWithEH(struct BasicBlock*) 1869746:+2.27%:1.57%:+0.0046%:BasicBlock::VisitAllSuccs<`AllSuccessorEnumerator::AllSuccessorEnumerator'::`2'::<lambda_1>>1822996:NA:1.53%:+0.0045%:private: bool __cdecl IncrementalSsaBuilder::FindReachingDefInBlock(struct UseDefLocation const &, struct BasicBlock*, struct UseDefLocation*) 1790224:NA:1.50%:+0.0044%:public: void __cdecl IncrementalSsaBuilder::Insert(void) 1522909:+0.56%:1.28%:+0.0038%:public: unsigned short __cdecl Compiler::optAddAssertion(struct Compiler::AssertionDsc*) 1018306:+2.48%:0.85%:+0.0025%:VisitEHSuccs<0,`AllSuccessorEnumerator::AllSuccessorEnumerator'::`2'::<lambda_1> >891863:+2.70%:0.75%:+0.0022%:public: struct Range __cdecl RangeCheck::ComputeRange(struct BasicBlock*, struct GenTree*, bool) 790041:+4.09%:0.66%:+0.0020%:public: bool __cdecl Compiler::fgMorphBlockStmt(struct BasicBlock*, struct Statement*) 717239:+6.03%:0.60%:+0.0018%:public: struct FlowEdge* __cdecl Compiler::BlockDominancePreds(struct BasicBlock*) 595655:+0.18%:0.50%:+0.0015%: memset 573612:+2.48%:0.48%:+0.0014%:public: void __cdecl Compiler::fgValueNumberPhiDef(struct GenTreeLclVar*, struct BasicBlock*, bool) 551275:+31.48%:0.46%:+0.0014%:public: struct EHblkDsc* __cdecl Compiler::ehGetBlockExnFlowDsc(struct BasicBlock*) 383941:+0.44%:0.32%:+0.0009%:public: void __cdecl Compiler::compInit(classArenaAllocator*, struct CORINFO_METHOD_STRUCT_*, classICorJitInfo*, struct CORINFO_METHOD_INFO*, struct InlineInfo*) 374654:+2.29%:0.31%:+0.0009%:public: struct Scev* __cdecl ScalarEvolutionContext::Simplify(struct Scev*, struct SimplificationAssumptions const &) 367471:+0.29%:0.31%:+0.0009%:private: void __cdecl SsaBuilder::BlockRenameVariables(struct BasicBlock*) 312764:+2.04%:0.26%:+0.0008%:public: bool __cdecl JitHashTable<struct GenTree *, struct JitPtrKeyFuncs<struct GenTree>, struct Scev*, classCompAllocator, classJitHashTableBehavior>::Set(struct GenTree*, struct Scev*, enumJitHashTable<struct GenTree *, struct JitPtrKeyFuncs<struct GenTree>, struct Scev*, classCompAllocator, classJitHashTableBehavior>::SetKind) 304582:+0.07%:0.26%:+0.0008%:public: struct GenTree* __cdecl Compiler::optAssertionProp_LclVar(unsigned __int64 *const &, struct GenTreeLclVarCommon*, struct Statement*) 297432:+4.41%:0.25%:+0.0007%:public: enumRelopEvaluationResult __cdecl ScalarEvolutionContext::EvaluateRelop(unsigned int) 281935:+0.55%:0.24%:+0.0007%:private: void __cdecl JitHashTable<struct CORINFO_FIELD_STRUCT_*, struct JitPtrKeyFuncs<struct CORINFO_FIELD_STRUCT_>, classFieldSeq, classCompAllocator, classJitHashTableBehavior>::Grow(void) 254204:+2.33%:0.21%:+0.0006%:public: static void __cdecl BitSetOps<unsigned __int64 *, 1, struct BitVecTraits *, struct BitVecTraits>::ClearD(struct BitVecTraits*, unsigned __int64 *&) 221830:+1.38%:0.19%:+0.0005%:private: void __cdecl SsaDefArray<class LclSsaVarDsc>::GrowArray(classCompAllocator) 220372:+0.14%:0.18%:+0.0005%:public: enumPhaseStatus __cdecl Compiler::optAssertionPropMain(void) 197369:+3.85%:0.17%:+0.0005%:public: bool __cdecl RangeCheck::ComputeDoesOverflow(struct BasicBlock*, struct GenTree*, struct Range const &) 194313:+0.26%:0.16%:+0.0005%:public: void __cdecl Compiler::optImpliedAssertions(unsigned short, unsigned __int64 *&) 186496:+2.80%:0.16%:+0.0005%:public: struct Range __cdecl RangeCheck::GetRange(struct BasicBlock*, struct GenTree*, bool) 178680:+0.22%:0.15%:+0.0004%:private: void __cdecl SsaBuilder::ComputeIteratedDominanceFrontier(struct BasicBlock*, classJitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>, classCompAllocator, classJitHashTableBehavior> const *, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>*) 175067:+0.24%:0.15%:+0.0004%:public: void __cdecl DataFlow::ForwardAnalysis<class AssertionPropFlowCallback>(classAssertionPropFlowCallback&) 174825:+0.88%:0.15%:+0.0004%:public: void __cdecl ValueNumStore::GetConstantBoundInfo(unsigned int, struct ValueNumStore::ConstantBoundInfo*) 172806:+0.31%:0.14%:+0.0004%:protected: void __cdecl Compiler::compCompile(void **, unsigned int *, classJitFlags*) 172736:+2.73%:0.14%:+0.0004%:public: void __cdecl RangeCheck::MergeEdgeAssertions(struct GenTreeLclVarCommon*, unsigned __int64 *const &, struct Range*) 164132:+3.16%:0.14%:+0.0004%:public: static struct Range __cdecl RangeOps::Merge(struct Range const &, struct Range const &, bool) 162139:+3.63%:0.14%:+0.0004%:public: struct Range __cdecl RangeCheck::ComputeRangeForBinOp(struct BasicBlock*, struct GenTreeOp*, bool) 155473:+0.24%:0.13%:+0.0004%:public: void __cdecl Compiler::optImpliedByTypeOfAssertions(unsigned __int64 *&) 150968:+0.33%:0.13%:+0.0004%:BasicBlock::VisitAllSuccs<`Compiler::optReachable'::`12'::<lambda_1>>147163:+0.33%:0.12%:+0.0004%:public: unsigned short __cdecl Compiler::optGlobalAssertionIsEqualOrNotEqualZero(unsigned __int64 *const &, struct GenTree*) 146382:+0.76%:0.12%:+0.0004%:public: enumPhaseStatus __cdecl Compiler::optVNBasedDeadStoreRemoval(void) 139613:+0.29%:0.12%:+0.0003%: `Compiler::optReachable'::`12'::<lambda_1>::operator() 138149:+0.06%:0.12%:+0.0003%:public: unsigned int __cdecl ValueNumStore::VNForFunc(enumvar_types, enumVNFunc, unsigned int, unsigned int) 136733:+0.65%:0.11%:+0.0003%: `LoopLocalOccurrences::GetOrCreateMap'::`2'::<lambda_1>::operator() 130909:+0.04%:0.11%:+0.0003%:public: unsigned int * __cdecl JitHashTable<struct ValueNumStore::VNDefFuncApp<2>, struct ValueNumStore::VNDefFuncAppKeyFuncs<2>, unsigned int, classCompAllocator, classJitHashTableBehavior>::LookupPointerOrAdd(struct ValueNumStore::VNDefFuncApp<2>, unsigned int) 124058:+0.16%:0.10%:+0.0003%:public: static void __cdecl BitSetOps<unsigned __int64 *, 1, struct BitVecTraits *, struct BitVecTraits>::IntersectionD(struct BitVecTraits*, unsigned __int64 *&, unsigned __int64 *) -374413:-3.47%:0.31%:-0.0009%:public: enumPhaseStatus __cdecl Compiler::fgSsaBuild(void) -7939897:-26.44%:6.65%:-0.0196%:private: void __cdecl SsaBuilder::AddPhiArg(struct BasicBlock*, struct Statement*, struct GenTreePhi*, unsigned int, unsigned int, struct BasicBlock*) -9406160:-14.99%:7.88%:-0.0232%:private: void __cdecl SsaBuilder::InsertPhiFunctions(void)

@build-analysisbuild-analysisBot mentioned this pull request Aug 23, 2024
Closed
@dotnet-policy-servicedotnet-policy-serviceBot removed this from the 10.0.0 milestone Sep 22, 2024
@jakobbotschjakobbotsch reopened this Oct 7, 2024
@jakobbotschjakobbotsch mentioned this pull request Oct 31, 2024
Closed
@jakobbotsch

jakobbotsch commented Oct 31, 2024

Copy link
Copy Markdown
MemberAuthor

This seems to also break strength reduction sometimes, need to investigate that.

image

@jakobbotschjakobbotsch mentioned this pull request Nov 4, 2024
Merged
@jakobbotschjakobbotsch mentioned this pull request Nov 6, 2024
Closed
@jakobbotsch

jakobbotsch commented Nov 6, 2024

Copy link
Copy Markdown
MemberAuthor

This seems to also break strength reduction sometimes, need to investigate that.

The problem here was the fact that I added the lvTracked check in optLocalHasNonLoopUses. I did that separately in #109505, to take those regressions separately (this was a correctness fix). To resolve those regressions I have then added the necessary liveness computation to this PR: that is, when inserting a local into SSA we now also compute the set of blocks in which that local is live-in, which is then stored in a side table. The cost of that computation does not look to be too bad -- overall TP cost of SSA insertion + liveness computation is around 0.2% for win-x64, with some collections being outliers in both directions. I think that cost is acceptable given that this gets us a fair amount of new strength reductions.

Still have some other regressions to investigate.

@jakobbotsch

jakobbotsch commented Nov 6, 2024

Copy link
Copy Markdown
MemberAuthor

cc @dotnet/jit-contrib PTAL @AndyAyersMS -- this now computes liveness for SSA-inserted locals, see my comment above. Can you review those new parts?

Diffs.

I went through a bunch of ASP.NET regressions and they fell into two categories:

  • Some regressions are because of new eliminated comparisons in assertion prop, which can lead to diferent register allocation due to fewer uses of those locals, and this can end up being a regression overall
  • Other regressions happen when IVs created by IV opts do not get a register allocated (nothing new here)

Sometimes strength reduction is also a size regression while being a perfscore regression, but there is nothing new there.

Some stats for number of strength reductions in win-x64 collections with this PR:
aspnet: 1710 -> 1819 (+6.4%)
benchmarks.run_pgo: 2177 -> 2252 (+3.4%)
libraries_tests.run: 10483 -> 11115 (+6.0%) with 74 new misses
realworld: 471 -> 523 (+11%)

@jakobbotsch
jakobbotsch marked this pull request as ready for review November 6, 2024 16:32
@jakobbotsch

jakobbotsch commented Nov 6, 2024

Copy link
Copy Markdown
MemberAuthor

This seems to be more expensive for arm64, likely because there are more CSEs that are now having their liveness computed.
One thing is that we only very rarely need that liveness, for IV opts only, so if we could only compute it at that point it would be better. But it would require us to keep the list of reaching defs/uses up to date between CSE and IV opts, while currently we only need to keep the liveness information up to date. Probably not impossible, but seems impractical given that assertion prop runs there. An alternative would be to find some way to predict whether we'll need the liveness, but I also didn't see a simple way of doing that from CSE.

AndyAyersMS
AndyAyersMS approved these changes Nov 6, 2024
View reviewed changes
Comment threadsrc/coreclr/jit/ssabuilder.cpp
m_queue.Reset();
m_queue.Push(use.Block);

while (!m_queue.Empty())

@AndyAyersMSAndyAyersMSNov 6, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a check that you actually do find the def block, and/or don't try to search past the first block? (that is, def dominates use)

@jakobbotschjakobbotschNov 7, 2024

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we necessarily find the reaching def (we might hit a block marked live-in already due to another use), but makes sense to add an assert that we don't get to the first block. I added that assert into AddInsertedSsaLiveIn since it never makes sense to mark these added locals as live into the first BB.

@build-analysisbuild-analysisBot mentioned this pull request Nov 7, 2024
Open
3 tasks
@jakobbotsch
jakobbotsch merged commit 9c1f53e into dotnet:mainNov 7, 2024
@jakobbotsch
jakobbotsch deleted the ssa-updating branch November 7, 2024 10:18
This was referenced Nov 8, 2024
Closed
Closed
@DrewScogginsDrewScoggins mentioned this pull request Nov 14, 2024
Closed
@AndyAyersMSAndyAyersMS mentioned this pull request Nov 21, 2024
Closed
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Dec 8, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@AndyAyersMSAndyAyersMSAndyAyersMS approved these changes

Assignees

@jakobbotschjakobbotsch

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

JIT: Cloned loop not fully strength reduced on arm64

2 participants

@jakobbotsch@AndyAyersMS
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
JIT: Put all CSEs into SSA - #106637
Merged
jakobbotsch merged 29 commits into
dotnet:mainfrom 
jakobbotsch:ssa-updating
Nov 7, 2024
Merged
JIT: Put all CSEs into SSA#106637
jakobbotsch merged 29 commits into
dotnet:mainfrom 
jakobbotsch:ssa-updating
Conversation
@jakobbotsch
@jakobbotschjakobbotsch
commented
Aug 19, 2024•

edited

Loading

Copy link


Copy Markdown

Member
This adds an SSA updater that can incrementally put locals that were added to the IR into SSA form and starts using this infrastructure from CSE. Previously only single-def CSEs were put into SSA, which can cause e.g. IV opts to miss out on them.
The SSA updater requires all uses and definitions to be supplied. Based on the definitions it is then possible to compute the candidate blocks for phi nodes in the usual way as the iterated dominance frontier. Since we do not have liveness for the local we do not insert the phi definitions eagerly; instead, we recursively compute the reaching def for each use by walking its dominators.

Walking the dominators we either expect to find a real definition, or to hit a block that is part of the iterated dominance frontier of the definitions, in which case we know we have a live phi definition that we can insert.
Inserting phi definitions needs to recursively do the same thing for the phi arguments. To make this faster we could memoize the reaching SSA numbers for each block, though this is not currently done.
This also requires computing the DFS tree, dominators, and dominance frontiers after RBO (which invalidates those). Since the DFS tree and dominator tree is usually computed anyway by IV opts, this is not that costly.

The cost is further reduced by avoiding any of these computations in the single-def case (which CSE already handled). This special case is also moved into the SSA updater.
Finally, since IV opts need liveness to work, the SSA inserter also computes liveness for the inserted locals. This is done by marking all paths from each use back to its reaching definition as having that local live-in.
Fix#109412
@jakobbotsch
JIT: Put all CSEs into SSA
ddf2b02
This adds an SSA updater that can incrementally put locals that were
added to the IR into SSA form and starts using this infrastructure from
CSE. It updates CSE to use this SSA updater to put all CSEs into SSA.
Previously only single-def CSEs were put into SSA, which can cause e.g.
IV opts to miss out on them.
The SSA updater requires all uses and definitions to be supplied. Based
on the definitions it is then possible to compute the candidate blocks
for phi nodes in the usual way as the iterated dominance frontier. Since
we do not have liveness for the local we do not insert the phi
definitions eagerly; instead, we recursively compute the reaching def
for each use by walking its dominators.
Walking the dominators we either expect to find a real definition, or to
hit a block that is part of the iterated dominance frontier of the
definitions, in which case we know we have a live phi definition that we
can insert.
Inserting phi definitions needs to recursively do the same thing for the
phi arguments. To make this faster we could memoize the reaching SSA
numbers for each block, though this is not currently done.
This also requires computing the DFS tree, dominators, and dominance
frontiers after RBO (which invalidates those). The DFS tree and
dominators should be reusable by IV opts in most cases, though currently
we invalidate them after CSE because assertion prop sometimes will
change the flow graph and does not know how to invalidate the DFS tree.
This can also be fixed to improve throughput.
We currently do not get much use out of this because IV opts also need
liveness for the IVs. More specifically it needs to know whether the IVs
are live out of the loop; it should be possible to keep some breadcrumbs
around for inserted locals to cheaply know this, I think.
@ghostghost
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
Aug 19, 2024
@jakobbotsch

This comment was marked as outdated.

Sign in to view

This was referenced Aug 19, 2024

Closed


Closed

@jakobbotsch
jakobbotsch
commented
Aug 20, 2024

Copy link


Copy Markdown

MemberAuthor
TP diffs for benchmarks.run_pgo now look like:
Base:120833007777, Diff:120935018667, +0.0844%317810259:NA:25.72%:+0.2630%:public: void __cdecl FlowGraphDominanceFrontiers::ComputeIteratedDominanceFrontier(struct BasicBlock*, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>*) 162099515:NA:13.12%:+0.1342%:public: static classFlowGraphDominanceFrontiers* __cdecl FlowGraphDominanceFrontiers::Build(classFlowGraphDominatorTree*) 39047720:NA:3.16%:+0.0323%:private: static void __cdecl SsaBuilder::AddNewPhiArg(classCompiler*, struct BasicBlock*, struct Statement*, struct GenTreePhi*, unsigned int, unsigned int, struct BasicBlock*) 38346141:NA:3.10%:+0.0317%:public: bool __cdecl Compiler::fgMorphBlockStmt(struct BasicBlock*, struct Statement*, bool) 18905583:+52.64%:1.53%:+0.0156%:public: virtual void __cdecl CSE_HeuristicCommon::PerformCSE(classCSE_Candidate*) 13835238:NA:1.12%:+0.0114%:private: static struct Statement* __cdecl SsaBuilder::InsertPhi(classCompiler*, struct BasicBlock*, unsigned int) 8753827:+0.85%:0.71%:+0.0072%:public: __cdecl `public: unsigned int __cdecl Compiler::fgRunDfs<class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_1>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_2>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_3>, 0>(class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_1>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_2>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_3>)'::`2'::<lambda_1>::operator()(struct BasicBlock*) const
7221209:+0.28%:0.58%:+0.0060%:public: void * __cdecl ArenaAllocator::allocateMemory(unsigned __int64) 7119522:+2.54%:0.58%:+0.0059%:public: static classFlowGraphDominatorTree* __cdecl FlowGraphDominatorTree::Build(classFlowGraphDfsTree const *) 6499831:+7.25%:0.53%:+0.0054%:private: bool __cdecl jitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>::ensure_capacity(unsigned __int64) 4424130:+12.56%:0.36%:+0.0037%:private: void __cdecl JitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, struct FlowEdge*, classCompAllocator, classJitHashTableBehavior>::Grow(void) 3705888:NA:0.30%:+0.0031%:public: static void __cdecl SsaBuilder::InsertInSsa(classCompiler*, unsigned int, classArrayStack<struct UseDefLocation> &, classArrayStack<struct UseDefLocation> &) 3361499:NA:0.27%:+0.0028%:private: struct UseDefLocation __cdecl IncrementalSsaBuilder::FindOrCreateReachingDef(struct UseDefLocation const &) 2892505:+0.42%:0.23%:+0.0024%:public: unsigned short __cdecl Compiler::optAddAssertion(struct Compiler::AssertionDsc*) 2651134:+0.69%:0.21%:+0.0022%:BasicBlock::VisitAllSuccs<`AllSuccessorEnumerator::AllSuccessorEnumerator'::`2'::<lambda_1>>2535856:+4.24%:0.21%:+0.0021%:public: struct FlowEdge* __cdecl Compiler::BlockPredsWithEH(struct BasicBlock*) 1743270:NA:0.14%:+0.0014%:public: void __cdecl IncrementalSsaBuilder::Insert(void) 1693606:+0.58%:0.14%:+0.0014%:public: void __cdecl RangeCheck::MergeEdgeAssertions(unsigned int, unsigned __int64 *const &, struct Range*) 1583199:NA:0.13%:+0.0013%:private: bool __cdecl IncrementalSsaBuilder::FindReachingDefInBlock(struct UseDefLocation const &, struct BasicBlock*, struct UseDefLocation*) 1557652:+1.89%:0.13%:+0.0013%:public: void __cdecl Compiler::fgValueNumberPhiDef(struct GenTreeLclVar*, struct BasicBlock*, bool) 1349037:+0.75%:0.11%:+0.0011%:VisitEHSuccs<0,`AllSuccessorEnumerator::AllSuccessorEnumerator'::`2'::<lambda_1> >1261895:+0.82%:0.10%:+0.0010%: `Compiler::optReachable'::`12'::<lambda_1>::operator() -5032126:-0.49%:0.41%:-0.0042%: memset -26356661:-50.03%:2.13%:-0.0218%:private: void __cdecl SsaBuilder::AddPhiArg(struct BasicBlock*, struct Statement*, struct GenTreePhi*, unsigned int, unsigned int, struct BasicBlock*) -30940686:-15.02%:2.50%:-0.0256%:private: void __cdecl SsaBuilder::InsertPhiFunctions(void) -36618036:-100.00%:2.96%:-0.0303%:public: bool __cdecl Compiler::fgMorphBlockStmt(struct BasicBlock*, struct Statement*) -139925973:-100.00%:11.32%:-0.1158%:private: void __cdecl SsaBuilder::ComputeDominanceFrontiers(struct BasicBlock**, int, classJitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>, classCompAllocator, classJitHashTableBehavior>*) -322436319:-100.00%:26.09%:-0.2668%:private: void __cdecl SsaBuilder::ComputeIteratedDominanceFrontier(struct BasicBlock*, classJitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>, classCompAllocator, classJitHashTableBehavior> const *, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>*) 
So looks small enough that we can take this change without much more consideration.
One thing is whether we need to memoize reaching defs in some way to avoid quadratic (or even potentially exponential) behavior. Need to think about that a bit.
@jakobbotschjakobbotsch
added this to the 10.0.0 milestone
Aug 20, 2024
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 20, 2024

Closed

@jakobbotsch
jakobbotsch
commented
Aug 21, 2024

Copy link


Copy Markdown

MemberAuthor
benchmarks.run_pgo seems to be one of the least affected collections. The most affected one is benchmarks.run. With some manual reconciliation of renamed functions in the diff, the detailed TP diff for it looks like:
Base:40469192366, Diff:40551805661, +0.2041%16221652:+33.98%:13.58%:+0.0401%:private: void __cdecl SsaBuilder::ComputeDominanceFrontiers(struct BasicBlock**, int, classJitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>, classCompAllocator, classJitHashTableBehavior>*) 14025772:+48.54%:11.74%:+0.0347%:public: virtual void __cdecl CSE_HeuristicCommon::PerformCSE(classCSE_Candidate*) 11802368:NA:9.88%:+0.0292%:private: static void __cdecl SsaBuilder::AddNewPhiArg(classCompiler*, struct BasicBlock*, struct Statement*, struct GenTreePhi*, unsigned int, unsigned int, struct BasicBlock*) 6227765:+2.88%:5.21%:+0.0154%:public: __cdecl `public: unsigned int __cdecl Compiler::fgRunDfs<class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_1>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_2>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_3>, 0>(class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_1>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_2>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_3>)'::`2'::<lambda_1>::operator()(struct BasicBlock*) const
4914583:+0.60%:4.11%:+0.0121%:public: void * __cdecl ArenaAllocator::allocateMemory(unsigned __int64) 4651869:+6.52%:3.89%:+0.0115%:public: static classFlowGraphDominatorTree* __cdecl FlowGraphDominatorTree::Build(classFlowGraphDfsTree const *) 4369172:+17.68%:3.66%:+0.0108%:private: bool __cdecl jitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>::ensure_capacity(unsigned __int64) 3469086:NA:2.90%:+0.0086%:private: static struct Statement* __cdecl SsaBuilder::InsertPhi(classCompiler*, struct BasicBlock*, unsigned int) 2793726:NA:2.34%:+0.0069%:public: static void __cdecl SsaBuilder::InsertInSsa(classCompiler*, unsigned int, classArrayStack<struct UseDefLocation> &, classArrayStack<struct UseDefLocation> &) 2747882:NA:2.30%:+0.0068%:private: struct UseDefLocation __cdecl IncrementalSsaBuilder::FindOrCreateReachingDef(struct UseDefLocation const &) 2745600:+25.93%:2.30%:+0.0068%:private: void __cdecl JitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, struct FlowEdge*, classCompAllocator, classJitHashTableBehavior>::Grow(void) 2538778:+2.19%:2.13%:+0.0063%:public: void __cdecl RangeCheck::MergeEdgeAssertions(unsigned int, unsigned __int64 *const &, struct Range*) 2129497:+12.45%:1.78%:+0.0053%:public: struct FlowEdge* __cdecl Compiler::BlockPredsWithEH(struct BasicBlock*) 1869746:+2.27%:1.57%:+0.0046%:BasicBlock::VisitAllSuccs<`AllSuccessorEnumerator::AllSuccessorEnumerator'::`2'::<lambda_1>>1822996:NA:1.53%:+0.0045%:private: bool __cdecl IncrementalSsaBuilder::FindReachingDefInBlock(struct UseDefLocation const &, struct BasicBlock*, struct UseDefLocation*) 1790224:NA:1.50%:+0.0044%:public: void __cdecl IncrementalSsaBuilder::Insert(void) 1522909:+0.56%:1.28%:+0.0038%:public: unsigned short __cdecl Compiler::optAddAssertion(struct Compiler::AssertionDsc*) 1018306:+2.48%:0.85%:+0.0025%:VisitEHSuccs<0,`AllSuccessorEnumerator::AllSuccessorEnumerator'::`2'::<lambda_1> >891863:+2.70%:0.75%:+0.0022%:public: struct Range __cdecl RangeCheck::ComputeRange(struct BasicBlock*, struct GenTree*, bool) 790041:+4.09%:0.66%:+0.0020%:public: bool __cdecl Compiler::fgMorphBlockStmt(struct BasicBlock*, struct Statement*) 717239:+6.03%:0.60%:+0.0018%:public: struct FlowEdge* __cdecl Compiler::BlockDominancePreds(struct BasicBlock*) 595655:+0.18%:0.50%:+0.0015%: memset 573612:+2.48%:0.48%:+0.0014%:public: void __cdecl Compiler::fgValueNumberPhiDef(struct GenTreeLclVar*, struct BasicBlock*, bool) 551275:+31.48%:0.46%:+0.0014%:public: struct EHblkDsc* __cdecl Compiler::ehGetBlockExnFlowDsc(struct BasicBlock*) 383941:+0.44%:0.32%:+0.0009%:public: void __cdecl Compiler::compInit(classArenaAllocator*, struct CORINFO_METHOD_STRUCT_*, classICorJitInfo*, struct CORINFO_METHOD_INFO*, struct InlineInfo*) 374654:+2.29%:0.31%:+0.0009%:public: struct Scev* __cdecl ScalarEvolutionContext::Simplify(struct Scev*, struct SimplificationAssumptions const &) 367471:+0.29%:0.31%:+0.0009%:private: void __cdecl SsaBuilder::BlockRenameVariables(struct BasicBlock*) 312764:+2.04%:0.26%:+0.0008%:public: bool __cdecl JitHashTable<struct GenTree *, struct JitPtrKeyFuncs<struct GenTree>, struct Scev*, classCompAllocator, classJitHashTableBehavior>::Set(struct GenTree*, struct Scev*, enumJitHashTable<struct GenTree *, struct JitPtrKeyFuncs<struct GenTree>, struct Scev*, classCompAllocator, classJitHashTableBehavior>::SetKind) 304582:+0.07%:0.26%:+0.0008%:public: struct GenTree* __cdecl Compiler::optAssertionProp_LclVar(unsigned __int64 *const &, struct GenTreeLclVarCommon*, struct Statement*) 297432:+4.41%:0.25%:+0.0007%:public: enumRelopEvaluationResult __cdecl ScalarEvolutionContext::EvaluateRelop(unsigned int) 281935:+0.55%:0.24%:+0.0007%:private: void __cdecl JitHashTable<struct CORINFO_FIELD_STRUCT_*, struct JitPtrKeyFuncs<struct CORINFO_FIELD_STRUCT_>, classFieldSeq, classCompAllocator, classJitHashTableBehavior>::Grow(void) 254204:+2.33%:0.21%:+0.0006%:public: static void __cdecl BitSetOps<unsigned __int64 *, 1, struct BitVecTraits *, struct BitVecTraits>::ClearD(struct BitVecTraits*, unsigned __int64 *&) 221830:+1.38%:0.19%:+0.0005%:private: void __cdecl SsaDefArray<class LclSsaVarDsc>::GrowArray(classCompAllocator) 220372:+0.14%:0.18%:+0.0005%:public: enumPhaseStatus __cdecl Compiler::optAssertionPropMain(void) 197369:+3.85%:0.17%:+0.0005%:public: bool __cdecl RangeCheck::ComputeDoesOverflow(struct BasicBlock*, struct GenTree*, struct Range const &) 194313:+0.26%:0.16%:+0.0005%:public: void __cdecl Compiler::optImpliedAssertions(unsigned short, unsigned __int64 *&) 186496:+2.80%:0.16%:+0.0005%:public: struct Range __cdecl RangeCheck::GetRange(struct BasicBlock*, struct GenTree*, bool) 178680:+0.22%:0.15%:+0.0004%:private: void __cdecl SsaBuilder::ComputeIteratedDominanceFrontier(struct BasicBlock*, classJitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>, classCompAllocator, classJitHashTableBehavior> const *, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>*) 175067:+0.24%:0.15%:+0.0004%:public: void __cdecl DataFlow::ForwardAnalysis<class AssertionPropFlowCallback>(classAssertionPropFlowCallback&) 174825:+0.88%:0.15%:+0.0004%:public: void __cdecl ValueNumStore::GetConstantBoundInfo(unsigned int, struct ValueNumStore::ConstantBoundInfo*) 172806:+0.31%:0.14%:+0.0004%:protected: void __cdecl Compiler::compCompile(void **, unsigned int *, classJitFlags*) 172736:+2.73%:0.14%:+0.0004%:public: void __cdecl RangeCheck::MergeEdgeAssertions(struct GenTreeLclVarCommon*, unsigned __int64 *const &, struct Range*) 164132:+3.16%:0.14%:+0.0004%:public: static struct Range __cdecl RangeOps::Merge(struct Range const &, struct Range const &, bool) 162139:+3.63%:0.14%:+0.0004%:public: struct Range __cdecl RangeCheck::ComputeRangeForBinOp(struct BasicBlock*, struct GenTreeOp*, bool) 155473:+0.24%:0.13%:+0.0004%:public: void __cdecl Compiler::optImpliedByTypeOfAssertions(unsigned __int64 *&) 150968:+0.33%:0.13%:+0.0004%:BasicBlock::VisitAllSuccs<`Compiler::optReachable'::`12'::<lambda_1>>147163:+0.33%:0.12%:+0.0004%:public: unsigned short __cdecl Compiler::optGlobalAssertionIsEqualOrNotEqualZero(unsigned __int64 *const &, struct GenTree*) 146382:+0.76%:0.12%:+0.0004%:public: enumPhaseStatus __cdecl Compiler::optVNBasedDeadStoreRemoval(void) 139613:+0.29%:0.12%:+0.0003%: `Compiler::optReachable'::`12'::<lambda_1>::operator() 138149:+0.06%:0.12%:+0.0003%:public: unsigned int __cdecl ValueNumStore::VNForFunc(enumvar_types, enumVNFunc, unsigned int, unsigned int) 136733:+0.65%:0.11%:+0.0003%: `LoopLocalOccurrences::GetOrCreateMap'::`2'::<lambda_1>::operator() 130909:+0.04%:0.11%:+0.0003%:public: unsigned int * __cdecl JitHashTable<struct ValueNumStore::VNDefFuncApp<2>, struct ValueNumStore::VNDefFuncAppKeyFuncs<2>, unsigned int, classCompAllocator, classJitHashTableBehavior>::LookupPointerOrAdd(struct ValueNumStore::VNDefFuncApp<2>, unsigned int) 124058:+0.16%:0.10%:+0.0003%:public: static void __cdecl BitSetOps<unsigned __int64 *, 1, struct BitVecTraits *, struct BitVecTraits>::IntersectionD(struct BitVecTraits*, unsigned __int64 *&, unsigned __int64 *) -374413:-3.47%:0.31%:-0.0009%:public: enumPhaseStatus __cdecl Compiler::fgSsaBuild(void) -7939897:-26.44%:6.65%:-0.0196%:private: void __cdecl SsaBuilder::AddPhiArg(struct BasicBlock*, struct Statement*, struct GenTreePhi*, unsigned int, unsigned int, struct BasicBlock*) -9406160:-14.99%:7.88%:-0.0232%:private: void __cdecl SsaBuilder::InsertPhiFunctions(void) 
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 23, 2024

Closed

@dotnet-policy-servicedotnet-policy-serviceBot
removed this from the 10.0.0 milestone
Sep 22, 2024
@jakobbotschjakobbotsch
reopened this
Oct 7, 2024
@jakobbotschjakobbotsch
mentioned this pull request
Oct 31, 2024

Closed

@jakobbotsch
jakobbotsch
commented
Oct 31, 2024

Copy link


Copy Markdown

MemberAuthor
This seems to also break strength reduction sometimes, need to investigate that.
image
@jakobbotschjakobbotsch
mentioned this pull request
Nov 4, 2024
 Merged

@jakobbotschjakobbotsch
mentioned this pull request
Nov 6, 2024
 Closed

@jakobbotsch
jakobbotsch
commented
Nov 6, 2024

Copy link


Copy Markdown

MemberAuthor
This seems to also break strength reduction sometimes, need to investigate that.
The problem here was the fact that I added the lvTracked check in optLocalHasNonLoopUses. I did that separately in #109505, to take those regressions separately (this was a correctness fix). To resolve those regressions I have then added the necessary liveness computation to this PR: that is, when inserting a local into SSA we now also compute the set of blocks in which that local is live-in, which is then stored in a side table. The cost of that computation does not look to be too bad -- overall TP cost of SSA insertion + liveness computation is around 0.2% for win-x64, with some collections being outliers in both directions. I think that cost is acceptable given that this gets us a fair amount of new strength reductions.
Still have some other regressions to investigate.
@jakobbotsch
jakobbotsch
commented
Nov 6, 2024

Copy link


Copy Markdown

MemberAuthor
cc @dotnet/jit-contrib PTAL @AndyAyersMS -- this now computes liveness for SSA-inserted locals, see my comment above. Can you review those new parts?
Diffs.
I went through a bunch of ASP.NET regressions and they fell into two categories:
  • Some regressions are because of new eliminated comparisons in assertion prop, which can lead to diferent register allocation due to fewer uses of those locals, and this can end up being a regression overall
  • Other regressions happen when IVs created by IV opts do not get a register allocated (nothing new here)
Sometimes strength reduction is also a size regression while being a perfscore regression, but there is nothing new there.
Some stats for number of strength reductions in win-x64 collections with this PR:

aspnet: 1710 -> 1819 (+6.4%)

benchmarks.run_pgo: 2177 -> 2252 (+3.4%)

libraries_tests.run: 10483 -> 11115 (+6.0%) with 74 new misses

realworld: 471 -> 523 (+11%)
@jakobbotsch
jakobbotsch
marked this pull request as ready for review
November 6, 2024 16:32
@jakobbotsch
jakobbotsch
commented
Nov 6, 2024

Copy link


Copy Markdown

MemberAuthor
This seems to be more expensive for arm64, likely because there are more CSEs that are now having their liveness computed.

One thing is that we only very rarely need that liveness, for IV opts only, so if we could only compute it at that point it would be better. But it would require us to keep the list of reaching defs/uses up to date between CSE and IV opts, while currently we only need to keep the liveness information up to date. Probably not impossible, but seems impractical given that assertion prop runs there. An alternative would be to find some way to predict whether we'll need the liveness, but I also didn't see a simple way of doing that from CSE.
AndyAyersMS
AndyAyersMS
approved these changes
Nov 6, 2024
View reviewed changes
Comment threadsrc/coreclr/jit/ssabuilder.cpp
 m_queue.Reset();
 m_queue.Push(use.Block);

 while (!m_queue.Empty())
@AndyAyersMSAndyAyersMSNov 6, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a check that you actually do find the def block, and/or don't try to search past the first block? (that is, def dominates use)
@jakobbotschjakobbotschNov 7, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we necessarily find the reaching def (we might hit a block marked live-in already due to another use), but makes sense to add an assert that we don't get to the first block. I added that assert into AddInsertedSsaLiveIn since it never makes sense to mark these added locals as live into the first BB.
@build-analysisbuild-analysisBot
mentioned this pull request
Nov 7, 2024

Open

3 tasks
@jakobbotsch
jakobbotsch
merged commit 9c1f53e
into
dotnet:mainNov 7, 2024
@jakobbotsch
jakobbotsch
deleted the

ssa-updating

branch
November 7, 2024 10:18

This was referenced Nov 8, 2024

Closed


Closed

@DrewScogginsDrewScoggins
mentioned this pull request
Nov 14, 2024

Closed

@AndyAyersMSAndyAyersMS
mentioned this pull request
Nov 21, 2024

Closed

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Dec 8, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@AndyAyersMSAndyAyersMSAndyAyersMS approved these changes

Assignees

@jakobbotschjakobbotsch

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

JIT: Cloned loop not fully strength reduced on arm64


2 participants

@jakobbotsch@AndyAyersMS

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
JIT: Put all CSEs into SSA - #106637
Merged
jakobbotsch merged 29 commits into
dotnet:mainfrom 
jakobbotsch:ssa-updating
Nov 7, 2024
Merged
JIT: Put all CSEs into SSA#106637
jakobbotsch merged 29 commits into
dotnet:mainfrom 
jakobbotsch:ssa-updating
Conversation
@jakobbotsch
@jakobbotschjakobbotsch
commented
Aug 19, 2024•

edited

Loading

Copy link


Copy Markdown

Member
This adds an SSA updater that can incrementally put locals that were added to the IR into SSA form and starts using this infrastructure from CSE. Previously only single-def CSEs were put into SSA, which can cause e.g. IV opts to miss out on them.
The SSA updater requires all uses and definitions to be supplied. Based on the definitions it is then possible to compute the candidate blocks for phi nodes in the usual way as the iterated dominance frontier. Since we do not have liveness for the local we do not insert the phi definitions eagerly; instead, we recursively compute the reaching def for each use by walking its dominators.

Walking the dominators we either expect to find a real definition, or to hit a block that is part of the iterated dominance frontier of the definitions, in which case we know we have a live phi definition that we can insert.
Inserting phi definitions needs to recursively do the same thing for the phi arguments. To make this faster we could memoize the reaching SSA numbers for each block, though this is not currently done.
This also requires computing the DFS tree, dominators, and dominance frontiers after RBO (which invalidates those). Since the DFS tree and dominator tree is usually computed anyway by IV opts, this is not that costly.

The cost is further reduced by avoiding any of these computations in the single-def case (which CSE already handled). This special case is also moved into the SSA updater.
Finally, since IV opts need liveness to work, the SSA inserter also computes liveness for the inserted locals. This is done by marking all paths from each use back to its reaching definition as having that local live-in.
Fix#109412
@jakobbotsch
JIT: Put all CSEs into SSA
ddf2b02
This adds an SSA updater that can incrementally put locals that were
added to the IR into SSA form and starts using this infrastructure from
CSE. It updates CSE to use this SSA updater to put all CSEs into SSA.
Previously only single-def CSEs were put into SSA, which can cause e.g.
IV opts to miss out on them.
The SSA updater requires all uses and definitions to be supplied. Based
on the definitions it is then possible to compute the candidate blocks
for phi nodes in the usual way as the iterated dominance frontier. Since
we do not have liveness for the local we do not insert the phi
definitions eagerly; instead, we recursively compute the reaching def
for each use by walking its dominators.
Walking the dominators we either expect to find a real definition, or to
hit a block that is part of the iterated dominance frontier of the
definitions, in which case we know we have a live phi definition that we
can insert.
Inserting phi definitions needs to recursively do the same thing for the
phi arguments. To make this faster we could memoize the reaching SSA
numbers for each block, though this is not currently done.
This also requires computing the DFS tree, dominators, and dominance
frontiers after RBO (which invalidates those). The DFS tree and
dominators should be reusable by IV opts in most cases, though currently
we invalidate them after CSE because assertion prop sometimes will
change the flow graph and does not know how to invalidate the DFS tree.
This can also be fixed to improve throughput.
We currently do not get much use out of this because IV opts also need
liveness for the IVs. More specifically it needs to know whether the IVs
are live out of the loop; it should be possible to keep some breadcrumbs
around for inserted locals to cheaply know this, I think.
@ghostghost
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
Aug 19, 2024
@jakobbotsch

This comment was marked as outdated.

Sign in to view

This was referenced Aug 19, 2024

Closed


Closed

@jakobbotsch
jakobbotsch
commented
Aug 20, 2024

Copy link


Copy Markdown

MemberAuthor
TP diffs for benchmarks.run_pgo now look like:
Base:120833007777, Diff:120935018667, +0.0844%317810259:NA:25.72%:+0.2630%:public: void __cdecl FlowGraphDominanceFrontiers::ComputeIteratedDominanceFrontier(struct BasicBlock*, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>*) 162099515:NA:13.12%:+0.1342%:public: static classFlowGraphDominanceFrontiers* __cdecl FlowGraphDominanceFrontiers::Build(classFlowGraphDominatorTree*) 39047720:NA:3.16%:+0.0323%:private: static void __cdecl SsaBuilder::AddNewPhiArg(classCompiler*, struct BasicBlock*, struct Statement*, struct GenTreePhi*, unsigned int, unsigned int, struct BasicBlock*) 38346141:NA:3.10%:+0.0317%:public: bool __cdecl Compiler::fgMorphBlockStmt(struct BasicBlock*, struct Statement*, bool) 18905583:+52.64%:1.53%:+0.0156%:public: virtual void __cdecl CSE_HeuristicCommon::PerformCSE(classCSE_Candidate*) 13835238:NA:1.12%:+0.0114%:private: static struct Statement* __cdecl SsaBuilder::InsertPhi(classCompiler*, struct BasicBlock*, unsigned int) 8753827:+0.85%:0.71%:+0.0072%:public: __cdecl `public: unsigned int __cdecl Compiler::fgRunDfs<class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_1>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_2>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_3>, 0>(class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_1>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_2>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_3>)'::`2'::<lambda_1>::operator()(struct BasicBlock*) const
7221209:+0.28%:0.58%:+0.0060%:public: void * __cdecl ArenaAllocator::allocateMemory(unsigned __int64) 7119522:+2.54%:0.58%:+0.0059%:public: static classFlowGraphDominatorTree* __cdecl FlowGraphDominatorTree::Build(classFlowGraphDfsTree const *) 6499831:+7.25%:0.53%:+0.0054%:private: bool __cdecl jitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>::ensure_capacity(unsigned __int64) 4424130:+12.56%:0.36%:+0.0037%:private: void __cdecl JitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, struct FlowEdge*, classCompAllocator, classJitHashTableBehavior>::Grow(void) 3705888:NA:0.30%:+0.0031%:public: static void __cdecl SsaBuilder::InsertInSsa(classCompiler*, unsigned int, classArrayStack<struct UseDefLocation> &, classArrayStack<struct UseDefLocation> &) 3361499:NA:0.27%:+0.0028%:private: struct UseDefLocation __cdecl IncrementalSsaBuilder::FindOrCreateReachingDef(struct UseDefLocation const &) 2892505:+0.42%:0.23%:+0.0024%:public: unsigned short __cdecl Compiler::optAddAssertion(struct Compiler::AssertionDsc*) 2651134:+0.69%:0.21%:+0.0022%:BasicBlock::VisitAllSuccs<`AllSuccessorEnumerator::AllSuccessorEnumerator'::`2'::<lambda_1>>2535856:+4.24%:0.21%:+0.0021%:public: struct FlowEdge* __cdecl Compiler::BlockPredsWithEH(struct BasicBlock*) 1743270:NA:0.14%:+0.0014%:public: void __cdecl IncrementalSsaBuilder::Insert(void) 1693606:+0.58%:0.14%:+0.0014%:public: void __cdecl RangeCheck::MergeEdgeAssertions(unsigned int, unsigned __int64 *const &, struct Range*) 1583199:NA:0.13%:+0.0013%:private: bool __cdecl IncrementalSsaBuilder::FindReachingDefInBlock(struct UseDefLocation const &, struct BasicBlock*, struct UseDefLocation*) 1557652:+1.89%:0.13%:+0.0013%:public: void __cdecl Compiler::fgValueNumberPhiDef(struct GenTreeLclVar*, struct BasicBlock*, bool) 1349037:+0.75%:0.11%:+0.0011%:VisitEHSuccs<0,`AllSuccessorEnumerator::AllSuccessorEnumerator'::`2'::<lambda_1> >1261895:+0.82%:0.10%:+0.0010%: `Compiler::optReachable'::`12'::<lambda_1>::operator() -5032126:-0.49%:0.41%:-0.0042%: memset -26356661:-50.03%:2.13%:-0.0218%:private: void __cdecl SsaBuilder::AddPhiArg(struct BasicBlock*, struct Statement*, struct GenTreePhi*, unsigned int, unsigned int, struct BasicBlock*) -30940686:-15.02%:2.50%:-0.0256%:private: void __cdecl SsaBuilder::InsertPhiFunctions(void) -36618036:-100.00%:2.96%:-0.0303%:public: bool __cdecl Compiler::fgMorphBlockStmt(struct BasicBlock*, struct Statement*) -139925973:-100.00%:11.32%:-0.1158%:private: void __cdecl SsaBuilder::ComputeDominanceFrontiers(struct BasicBlock**, int, classJitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>, classCompAllocator, classJitHashTableBehavior>*) -322436319:-100.00%:26.09%:-0.2668%:private: void __cdecl SsaBuilder::ComputeIteratedDominanceFrontier(struct BasicBlock*, classJitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>, classCompAllocator, classJitHashTableBehavior> const *, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>*) 
So looks small enough that we can take this change without much more consideration.
One thing is whether we need to memoize reaching defs in some way to avoid quadratic (or even potentially exponential) behavior. Need to think about that a bit.
@jakobbotschjakobbotsch
added this to the 10.0.0 milestone
Aug 20, 2024
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 20, 2024

Closed

@jakobbotsch
jakobbotsch
commented
Aug 21, 2024

Copy link


Copy Markdown

MemberAuthor
benchmarks.run_pgo seems to be one of the least affected collections. The most affected one is benchmarks.run. With some manual reconciliation of renamed functions in the diff, the detailed TP diff for it looks like:
Base:40469192366, Diff:40551805661, +0.2041%16221652:+33.98%:13.58%:+0.0401%:private: void __cdecl SsaBuilder::ComputeDominanceFrontiers(struct BasicBlock**, int, classJitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>, classCompAllocator, classJitHashTableBehavior>*) 14025772:+48.54%:11.74%:+0.0347%:public: virtual void __cdecl CSE_HeuristicCommon::PerformCSE(classCSE_Candidate*) 11802368:NA:9.88%:+0.0292%:private: static void __cdecl SsaBuilder::AddNewPhiArg(classCompiler*, struct BasicBlock*, struct Statement*, struct GenTreePhi*, unsigned int, unsigned int, struct BasicBlock*) 6227765:+2.88%:5.21%:+0.0154%:public: __cdecl `public: unsigned int __cdecl Compiler::fgRunDfs<class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_1>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_2>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_3>, 0>(class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_1>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_2>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_3>)'::`2'::<lambda_1>::operator()(struct BasicBlock*) const
4914583:+0.60%:4.11%:+0.0121%:public: void * __cdecl ArenaAllocator::allocateMemory(unsigned __int64) 4651869:+6.52%:3.89%:+0.0115%:public: static classFlowGraphDominatorTree* __cdecl FlowGraphDominatorTree::Build(classFlowGraphDfsTree const *) 4369172:+17.68%:3.66%:+0.0108%:private: bool __cdecl jitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>::ensure_capacity(unsigned __int64) 3469086:NA:2.90%:+0.0086%:private: static struct Statement* __cdecl SsaBuilder::InsertPhi(classCompiler*, struct BasicBlock*, unsigned int) 2793726:NA:2.34%:+0.0069%:public: static void __cdecl SsaBuilder::InsertInSsa(classCompiler*, unsigned int, classArrayStack<struct UseDefLocation> &, classArrayStack<struct UseDefLocation> &) 2747882:NA:2.30%:+0.0068%:private: struct UseDefLocation __cdecl IncrementalSsaBuilder::FindOrCreateReachingDef(struct UseDefLocation const &) 2745600:+25.93%:2.30%:+0.0068%:private: void __cdecl JitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, struct FlowEdge*, classCompAllocator, classJitHashTableBehavior>::Grow(void) 2538778:+2.19%:2.13%:+0.0063%:public: void __cdecl RangeCheck::MergeEdgeAssertions(unsigned int, unsigned __int64 *const &, struct Range*) 2129497:+12.45%:1.78%:+0.0053%:public: struct FlowEdge* __cdecl Compiler::BlockPredsWithEH(struct BasicBlock*) 1869746:+2.27%:1.57%:+0.0046%:BasicBlock::VisitAllSuccs<`AllSuccessorEnumerator::AllSuccessorEnumerator'::`2'::<lambda_1>>1822996:NA:1.53%:+0.0045%:private: bool __cdecl IncrementalSsaBuilder::FindReachingDefInBlock(struct UseDefLocation const &, struct BasicBlock*, struct UseDefLocation*) 1790224:NA:1.50%:+0.0044%:public: void __cdecl IncrementalSsaBuilder::Insert(void) 1522909:+0.56%:1.28%:+0.0038%:public: unsigned short __cdecl Compiler::optAddAssertion(struct Compiler::AssertionDsc*) 1018306:+2.48%:0.85%:+0.0025%:VisitEHSuccs<0,`AllSuccessorEnumerator::AllSuccessorEnumerator'::`2'::<lambda_1> >891863:+2.70%:0.75%:+0.0022%:public: struct Range __cdecl RangeCheck::ComputeRange(struct BasicBlock*, struct GenTree*, bool) 790041:+4.09%:0.66%:+0.0020%:public: bool __cdecl Compiler::fgMorphBlockStmt(struct BasicBlock*, struct Statement*) 717239:+6.03%:0.60%:+0.0018%:public: struct FlowEdge* __cdecl Compiler::BlockDominancePreds(struct BasicBlock*) 595655:+0.18%:0.50%:+0.0015%: memset 573612:+2.48%:0.48%:+0.0014%:public: void __cdecl Compiler::fgValueNumberPhiDef(struct GenTreeLclVar*, struct BasicBlock*, bool) 551275:+31.48%:0.46%:+0.0014%:public: struct EHblkDsc* __cdecl Compiler::ehGetBlockExnFlowDsc(struct BasicBlock*) 383941:+0.44%:0.32%:+0.0009%:public: void __cdecl Compiler::compInit(classArenaAllocator*, struct CORINFO_METHOD_STRUCT_*, classICorJitInfo*, struct CORINFO_METHOD_INFO*, struct InlineInfo*) 374654:+2.29%:0.31%:+0.0009%:public: struct Scev* __cdecl ScalarEvolutionContext::Simplify(struct Scev*, struct SimplificationAssumptions const &) 367471:+0.29%:0.31%:+0.0009%:private: void __cdecl SsaBuilder::BlockRenameVariables(struct BasicBlock*) 312764:+2.04%:0.26%:+0.0008%:public: bool __cdecl JitHashTable<struct GenTree *, struct JitPtrKeyFuncs<struct GenTree>, struct Scev*, classCompAllocator, classJitHashTableBehavior>::Set(struct GenTree*, struct Scev*, enumJitHashTable<struct GenTree *, struct JitPtrKeyFuncs<struct GenTree>, struct Scev*, classCompAllocator, classJitHashTableBehavior>::SetKind) 304582:+0.07%:0.26%:+0.0008%:public: struct GenTree* __cdecl Compiler::optAssertionProp_LclVar(unsigned __int64 *const &, struct GenTreeLclVarCommon*, struct Statement*) 297432:+4.41%:0.25%:+0.0007%:public: enumRelopEvaluationResult __cdecl ScalarEvolutionContext::EvaluateRelop(unsigned int) 281935:+0.55%:0.24%:+0.0007%:private: void __cdecl JitHashTable<struct CORINFO_FIELD_STRUCT_*, struct JitPtrKeyFuncs<struct CORINFO_FIELD_STRUCT_>, classFieldSeq, classCompAllocator, classJitHashTableBehavior>::Grow(void) 254204:+2.33%:0.21%:+0.0006%:public: static void __cdecl BitSetOps<unsigned __int64 *, 1, struct BitVecTraits *, struct BitVecTraits>::ClearD(struct BitVecTraits*, unsigned __int64 *&) 221830:+1.38%:0.19%:+0.0005%:private: void __cdecl SsaDefArray<class LclSsaVarDsc>::GrowArray(classCompAllocator) 220372:+0.14%:0.18%:+0.0005%:public: enumPhaseStatus __cdecl Compiler::optAssertionPropMain(void) 197369:+3.85%:0.17%:+0.0005%:public: bool __cdecl RangeCheck::ComputeDoesOverflow(struct BasicBlock*, struct GenTree*, struct Range const &) 194313:+0.26%:0.16%:+0.0005%:public: void __cdecl Compiler::optImpliedAssertions(unsigned short, unsigned __int64 *&) 186496:+2.80%:0.16%:+0.0005%:public: struct Range __cdecl RangeCheck::GetRange(struct BasicBlock*, struct GenTree*, bool) 178680:+0.22%:0.15%:+0.0004%:private: void __cdecl SsaBuilder::ComputeIteratedDominanceFrontier(struct BasicBlock*, classJitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>, classCompAllocator, classJitHashTableBehavior> const *, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>*) 175067:+0.24%:0.15%:+0.0004%:public: void __cdecl DataFlow::ForwardAnalysis<class AssertionPropFlowCallback>(classAssertionPropFlowCallback&) 174825:+0.88%:0.15%:+0.0004%:public: void __cdecl ValueNumStore::GetConstantBoundInfo(unsigned int, struct ValueNumStore::ConstantBoundInfo*) 172806:+0.31%:0.14%:+0.0004%:protected: void __cdecl Compiler::compCompile(void **, unsigned int *, classJitFlags*) 172736:+2.73%:0.14%:+0.0004%:public: void __cdecl RangeCheck::MergeEdgeAssertions(struct GenTreeLclVarCommon*, unsigned __int64 *const &, struct Range*) 164132:+3.16%:0.14%:+0.0004%:public: static struct Range __cdecl RangeOps::Merge(struct Range const &, struct Range const &, bool) 162139:+3.63%:0.14%:+0.0004%:public: struct Range __cdecl RangeCheck::ComputeRangeForBinOp(struct BasicBlock*, struct GenTreeOp*, bool) 155473:+0.24%:0.13%:+0.0004%:public: void __cdecl Compiler::optImpliedByTypeOfAssertions(unsigned __int64 *&) 150968:+0.33%:0.13%:+0.0004%:BasicBlock::VisitAllSuccs<`Compiler::optReachable'::`12'::<lambda_1>>147163:+0.33%:0.12%:+0.0004%:public: unsigned short __cdecl Compiler::optGlobalAssertionIsEqualOrNotEqualZero(unsigned __int64 *const &, struct GenTree*) 146382:+0.76%:0.12%:+0.0004%:public: enumPhaseStatus __cdecl Compiler::optVNBasedDeadStoreRemoval(void) 139613:+0.29%:0.12%:+0.0003%: `Compiler::optReachable'::`12'::<lambda_1>::operator() 138149:+0.06%:0.12%:+0.0003%:public: unsigned int __cdecl ValueNumStore::VNForFunc(enumvar_types, enumVNFunc, unsigned int, unsigned int) 136733:+0.65%:0.11%:+0.0003%: `LoopLocalOccurrences::GetOrCreateMap'::`2'::<lambda_1>::operator() 130909:+0.04%:0.11%:+0.0003%:public: unsigned int * __cdecl JitHashTable<struct ValueNumStore::VNDefFuncApp<2>, struct ValueNumStore::VNDefFuncAppKeyFuncs<2>, unsigned int, classCompAllocator, classJitHashTableBehavior>::LookupPointerOrAdd(struct ValueNumStore::VNDefFuncApp<2>, unsigned int) 124058:+0.16%:0.10%:+0.0003%:public: static void __cdecl BitSetOps<unsigned __int64 *, 1, struct BitVecTraits *, struct BitVecTraits>::IntersectionD(struct BitVecTraits*, unsigned __int64 *&, unsigned __int64 *) -374413:-3.47%:0.31%:-0.0009%:public: enumPhaseStatus __cdecl Compiler::fgSsaBuild(void) -7939897:-26.44%:6.65%:-0.0196%:private: void __cdecl SsaBuilder::AddPhiArg(struct BasicBlock*, struct Statement*, struct GenTreePhi*, unsigned int, unsigned int, struct BasicBlock*) -9406160:-14.99%:7.88%:-0.0232%:private: void __cdecl SsaBuilder::InsertPhiFunctions(void) 
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 23, 2024

Closed

@dotnet-policy-servicedotnet-policy-serviceBot
removed this from the 10.0.0 milestone
Sep 22, 2024
@jakobbotschjakobbotsch
reopened this
Oct 7, 2024
@jakobbotschjakobbotsch
mentioned this pull request
Oct 31, 2024

Closed

@jakobbotsch
jakobbotsch
commented
Oct 31, 2024

Copy link


Copy Markdown

MemberAuthor
This seems to also break strength reduction sometimes, need to investigate that.
image
@jakobbotschjakobbotsch
mentioned this pull request
Nov 4, 2024
 Merged

@jakobbotschjakobbotsch
mentioned this pull request
Nov 6, 2024
 Closed

@jakobbotsch
jakobbotsch
commented
Nov 6, 2024

Copy link


Copy Markdown

MemberAuthor
This seems to also break strength reduction sometimes, need to investigate that.
The problem here was the fact that I added the lvTracked check in optLocalHasNonLoopUses. I did that separately in #109505, to take those regressions separately (this was a correctness fix). To resolve those regressions I have then added the necessary liveness computation to this PR: that is, when inserting a local into SSA we now also compute the set of blocks in which that local is live-in, which is then stored in a side table. The cost of that computation does not look to be too bad -- overall TP cost of SSA insertion + liveness computation is around 0.2% for win-x64, with some collections being outliers in both directions. I think that cost is acceptable given that this gets us a fair amount of new strength reductions.
Still have some other regressions to investigate.
@jakobbotsch
jakobbotsch
commented
Nov 6, 2024

Copy link


Copy Markdown

MemberAuthor
cc @dotnet/jit-contrib PTAL @AndyAyersMS -- this now computes liveness for SSA-inserted locals, see my comment above. Can you review those new parts?
Diffs.
I went through a bunch of ASP.NET regressions and they fell into two categories:
  • Some regressions are because of new eliminated comparisons in assertion prop, which can lead to diferent register allocation due to fewer uses of those locals, and this can end up being a regression overall
  • Other regressions happen when IVs created by IV opts do not get a register allocated (nothing new here)
Sometimes strength reduction is also a size regression while being a perfscore regression, but there is nothing new there.
Some stats for number of strength reductions in win-x64 collections with this PR:

aspnet: 1710 -> 1819 (+6.4%)

benchmarks.run_pgo: 2177 -> 2252 (+3.4%)

libraries_tests.run: 10483 -> 11115 (+6.0%) with 74 new misses

realworld: 471 -> 523 (+11%)
@jakobbotsch
jakobbotsch
marked this pull request as ready for review
November 6, 2024 16:32
@jakobbotsch
jakobbotsch
commented
Nov 6, 2024

Copy link


Copy Markdown

MemberAuthor
This seems to be more expensive for arm64, likely because there are more CSEs that are now having their liveness computed.

One thing is that we only very rarely need that liveness, for IV opts only, so if we could only compute it at that point it would be better. But it would require us to keep the list of reaching defs/uses up to date between CSE and IV opts, while currently we only need to keep the liveness information up to date. Probably not impossible, but seems impractical given that assertion prop runs there. An alternative would be to find some way to predict whether we'll need the liveness, but I also didn't see a simple way of doing that from CSE.
AndyAyersMS
AndyAyersMS
approved these changes
Nov 6, 2024
View reviewed changes
Comment threadsrc/coreclr/jit/ssabuilder.cpp
 m_queue.Reset();
 m_queue.Push(use.Block);

 while (!m_queue.Empty())
@AndyAyersMSAndyAyersMSNov 6, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a check that you actually do find the def block, and/or don't try to search past the first block? (that is, def dominates use)
@jakobbotschjakobbotschNov 7, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we necessarily find the reaching def (we might hit a block marked live-in already due to another use), but makes sense to add an assert that we don't get to the first block. I added that assert into AddInsertedSsaLiveIn since it never makes sense to mark these added locals as live into the first BB.
@build-analysisbuild-analysisBot
mentioned this pull request
Nov 7, 2024

Open

3 tasks
@jakobbotsch
jakobbotsch
merged commit 9c1f53e
into
dotnet:mainNov 7, 2024
@jakobbotsch
jakobbotsch
deleted the

ssa-updating

branch
November 7, 2024 10:18

This was referenced Nov 8, 2024

Closed


Closed

@DrewScogginsDrewScoggins
mentioned this pull request
Nov 14, 2024

Closed

@AndyAyersMSAndyAyersMS
mentioned this pull request
Nov 21, 2024

Closed

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Dec 8, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@AndyAyersMSAndyAyersMSAndyAyersMS approved these changes

Assignees

@jakobbotschjakobbotsch

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

JIT: Cloned loop not fully strength reduced on arm64


2 participants

@jakobbotsch@AndyAyersMS

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
JIT: Put all CSEs into SSA - #106637
Merged
jakobbotsch merged 29 commits into
dotnet:mainfrom 
jakobbotsch:ssa-updating
Nov 7, 2024
Merged
JIT: Put all CSEs into SSA#106637
jakobbotsch merged 29 commits into
dotnet:mainfrom 
jakobbotsch:ssa-updating
Conversation
@jakobbotsch
@jakobbotschjakobbotsch
commented
Aug 19, 2024•

edited

Loading

Copy link


Copy Markdown

Member
This adds an SSA updater that can incrementally put locals that were added to the IR into SSA form and starts using this infrastructure from CSE. Previously only single-def CSEs were put into SSA, which can cause e.g. IV opts to miss out on them.
The SSA updater requires all uses and definitions to be supplied. Based on the definitions it is then possible to compute the candidate blocks for phi nodes in the usual way as the iterated dominance frontier. Since we do not have liveness for the local we do not insert the phi definitions eagerly; instead, we recursively compute the reaching def for each use by walking its dominators.

Walking the dominators we either expect to find a real definition, or to hit a block that is part of the iterated dominance frontier of the definitions, in which case we know we have a live phi definition that we can insert.
Inserting phi definitions needs to recursively do the same thing for the phi arguments. To make this faster we could memoize the reaching SSA numbers for each block, though this is not currently done.
This also requires computing the DFS tree, dominators, and dominance frontiers after RBO (which invalidates those). Since the DFS tree and dominator tree is usually computed anyway by IV opts, this is not that costly.

The cost is further reduced by avoiding any of these computations in the single-def case (which CSE already handled). This special case is also moved into the SSA updater.
Finally, since IV opts need liveness to work, the SSA inserter also computes liveness for the inserted locals. This is done by marking all paths from each use back to its reaching definition as having that local live-in.
Fix#109412
@jakobbotsch
JIT: Put all CSEs into SSA
ddf2b02
This adds an SSA updater that can incrementally put locals that were
added to the IR into SSA form and starts using this infrastructure from
CSE. It updates CSE to use this SSA updater to put all CSEs into SSA.
Previously only single-def CSEs were put into SSA, which can cause e.g.
IV opts to miss out on them.
The SSA updater requires all uses and definitions to be supplied. Based
on the definitions it is then possible to compute the candidate blocks
for phi nodes in the usual way as the iterated dominance frontier. Since
we do not have liveness for the local we do not insert the phi
definitions eagerly; instead, we recursively compute the reaching def
for each use by walking its dominators.
Walking the dominators we either expect to find a real definition, or to
hit a block that is part of the iterated dominance frontier of the
definitions, in which case we know we have a live phi definition that we
can insert.
Inserting phi definitions needs to recursively do the same thing for the
phi arguments. To make this faster we could memoize the reaching SSA
numbers for each block, though this is not currently done.
This also requires computing the DFS tree, dominators, and dominance
frontiers after RBO (which invalidates those). The DFS tree and
dominators should be reusable by IV opts in most cases, though currently
we invalidate them after CSE because assertion prop sometimes will
change the flow graph and does not know how to invalidate the DFS tree.
This can also be fixed to improve throughput.
We currently do not get much use out of this because IV opts also need
liveness for the IVs. More specifically it needs to know whether the IVs
are live out of the loop; it should be possible to keep some breadcrumbs
around for inserted locals to cheaply know this, I think.
@ghostghost
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
Aug 19, 2024
@jakobbotsch

This comment was marked as outdated.

Sign in to view

This was referenced Aug 19, 2024

Closed


Closed

@jakobbotsch
jakobbotsch
commented
Aug 20, 2024

Copy link


Copy Markdown

MemberAuthor
TP diffs for benchmarks.run_pgo now look like:
Base:120833007777, Diff:120935018667, +0.0844%317810259:NA:25.72%:+0.2630%:public: void __cdecl FlowGraphDominanceFrontiers::ComputeIteratedDominanceFrontier(struct BasicBlock*, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>*) 162099515:NA:13.12%:+0.1342%:public: static classFlowGraphDominanceFrontiers* __cdecl FlowGraphDominanceFrontiers::Build(classFlowGraphDominatorTree*) 39047720:NA:3.16%:+0.0323%:private: static void __cdecl SsaBuilder::AddNewPhiArg(classCompiler*, struct BasicBlock*, struct Statement*, struct GenTreePhi*, unsigned int, unsigned int, struct BasicBlock*) 38346141:NA:3.10%:+0.0317%:public: bool __cdecl Compiler::fgMorphBlockStmt(struct BasicBlock*, struct Statement*, bool) 18905583:+52.64%:1.53%:+0.0156%:public: virtual void __cdecl CSE_HeuristicCommon::PerformCSE(classCSE_Candidate*) 13835238:NA:1.12%:+0.0114%:private: static struct Statement* __cdecl SsaBuilder::InsertPhi(classCompiler*, struct BasicBlock*, unsigned int) 8753827:+0.85%:0.71%:+0.0072%:public: __cdecl `public: unsigned int __cdecl Compiler::fgRunDfs<class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_1>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_2>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_3>, 0>(class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_1>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_2>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_3>)'::`2'::<lambda_1>::operator()(struct BasicBlock*) const
7221209:+0.28%:0.58%:+0.0060%:public: void * __cdecl ArenaAllocator::allocateMemory(unsigned __int64) 7119522:+2.54%:0.58%:+0.0059%:public: static classFlowGraphDominatorTree* __cdecl FlowGraphDominatorTree::Build(classFlowGraphDfsTree const *) 6499831:+7.25%:0.53%:+0.0054%:private: bool __cdecl jitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>::ensure_capacity(unsigned __int64) 4424130:+12.56%:0.36%:+0.0037%:private: void __cdecl JitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, struct FlowEdge*, classCompAllocator, classJitHashTableBehavior>::Grow(void) 3705888:NA:0.30%:+0.0031%:public: static void __cdecl SsaBuilder::InsertInSsa(classCompiler*, unsigned int, classArrayStack<struct UseDefLocation> &, classArrayStack<struct UseDefLocation> &) 3361499:NA:0.27%:+0.0028%:private: struct UseDefLocation __cdecl IncrementalSsaBuilder::FindOrCreateReachingDef(struct UseDefLocation const &) 2892505:+0.42%:0.23%:+0.0024%:public: unsigned short __cdecl Compiler::optAddAssertion(struct Compiler::AssertionDsc*) 2651134:+0.69%:0.21%:+0.0022%:BasicBlock::VisitAllSuccs<`AllSuccessorEnumerator::AllSuccessorEnumerator'::`2'::<lambda_1>>2535856:+4.24%:0.21%:+0.0021%:public: struct FlowEdge* __cdecl Compiler::BlockPredsWithEH(struct BasicBlock*) 1743270:NA:0.14%:+0.0014%:public: void __cdecl IncrementalSsaBuilder::Insert(void) 1693606:+0.58%:0.14%:+0.0014%:public: void __cdecl RangeCheck::MergeEdgeAssertions(unsigned int, unsigned __int64 *const &, struct Range*) 1583199:NA:0.13%:+0.0013%:private: bool __cdecl IncrementalSsaBuilder::FindReachingDefInBlock(struct UseDefLocation const &, struct BasicBlock*, struct UseDefLocation*) 1557652:+1.89%:0.13%:+0.0013%:public: void __cdecl Compiler::fgValueNumberPhiDef(struct GenTreeLclVar*, struct BasicBlock*, bool) 1349037:+0.75%:0.11%:+0.0011%:VisitEHSuccs<0,`AllSuccessorEnumerator::AllSuccessorEnumerator'::`2'::<lambda_1> >1261895:+0.82%:0.10%:+0.0010%: `Compiler::optReachable'::`12'::<lambda_1>::operator() -5032126:-0.49%:0.41%:-0.0042%: memset -26356661:-50.03%:2.13%:-0.0218%:private: void __cdecl SsaBuilder::AddPhiArg(struct BasicBlock*, struct Statement*, struct GenTreePhi*, unsigned int, unsigned int, struct BasicBlock*) -30940686:-15.02%:2.50%:-0.0256%:private: void __cdecl SsaBuilder::InsertPhiFunctions(void) -36618036:-100.00%:2.96%:-0.0303%:public: bool __cdecl Compiler::fgMorphBlockStmt(struct BasicBlock*, struct Statement*) -139925973:-100.00%:11.32%:-0.1158%:private: void __cdecl SsaBuilder::ComputeDominanceFrontiers(struct BasicBlock**, int, classJitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>, classCompAllocator, classJitHashTableBehavior>*) -322436319:-100.00%:26.09%:-0.2668%:private: void __cdecl SsaBuilder::ComputeIteratedDominanceFrontier(struct BasicBlock*, classJitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>, classCompAllocator, classJitHashTableBehavior> const *, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>*) 
So looks small enough that we can take this change without much more consideration.
One thing is whether we need to memoize reaching defs in some way to avoid quadratic (or even potentially exponential) behavior. Need to think about that a bit.
@jakobbotschjakobbotsch
added this to the 10.0.0 milestone
Aug 20, 2024
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 20, 2024

Closed

@jakobbotsch
jakobbotsch
commented
Aug 21, 2024

Copy link


Copy Markdown

MemberAuthor
benchmarks.run_pgo seems to be one of the least affected collections. The most affected one is benchmarks.run. With some manual reconciliation of renamed functions in the diff, the detailed TP diff for it looks like:
Base:40469192366, Diff:40551805661, +0.2041%16221652:+33.98%:13.58%:+0.0401%:private: void __cdecl SsaBuilder::ComputeDominanceFrontiers(struct BasicBlock**, int, classJitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>, classCompAllocator, classJitHashTableBehavior>*) 14025772:+48.54%:11.74%:+0.0347%:public: virtual void __cdecl CSE_HeuristicCommon::PerformCSE(classCSE_Candidate*) 11802368:NA:9.88%:+0.0292%:private: static void __cdecl SsaBuilder::AddNewPhiArg(classCompiler*, struct BasicBlock*, struct Statement*, struct GenTreePhi*, unsigned int, unsigned int, struct BasicBlock*) 6227765:+2.88%:5.21%:+0.0154%:public: __cdecl `public: unsigned int __cdecl Compiler::fgRunDfs<class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_1>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_2>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_3>, 0>(class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_1>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_2>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_3>)'::`2'::<lambda_1>::operator()(struct BasicBlock*) const
4914583:+0.60%:4.11%:+0.0121%:public: void * __cdecl ArenaAllocator::allocateMemory(unsigned __int64) 4651869:+6.52%:3.89%:+0.0115%:public: static classFlowGraphDominatorTree* __cdecl FlowGraphDominatorTree::Build(classFlowGraphDfsTree const *) 4369172:+17.68%:3.66%:+0.0108%:private: bool __cdecl jitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>::ensure_capacity(unsigned __int64) 3469086:NA:2.90%:+0.0086%:private: static struct Statement* __cdecl SsaBuilder::InsertPhi(classCompiler*, struct BasicBlock*, unsigned int) 2793726:NA:2.34%:+0.0069%:public: static void __cdecl SsaBuilder::InsertInSsa(classCompiler*, unsigned int, classArrayStack<struct UseDefLocation> &, classArrayStack<struct UseDefLocation> &) 2747882:NA:2.30%:+0.0068%:private: struct UseDefLocation __cdecl IncrementalSsaBuilder::FindOrCreateReachingDef(struct UseDefLocation const &) 2745600:+25.93%:2.30%:+0.0068%:private: void __cdecl JitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, struct FlowEdge*, classCompAllocator, classJitHashTableBehavior>::Grow(void) 2538778:+2.19%:2.13%:+0.0063%:public: void __cdecl RangeCheck::MergeEdgeAssertions(unsigned int, unsigned __int64 *const &, struct Range*) 2129497:+12.45%:1.78%:+0.0053%:public: struct FlowEdge* __cdecl Compiler::BlockPredsWithEH(struct BasicBlock*) 1869746:+2.27%:1.57%:+0.0046%:BasicBlock::VisitAllSuccs<`AllSuccessorEnumerator::AllSuccessorEnumerator'::`2'::<lambda_1>>1822996:NA:1.53%:+0.0045%:private: bool __cdecl IncrementalSsaBuilder::FindReachingDefInBlock(struct UseDefLocation const &, struct BasicBlock*, struct UseDefLocation*) 1790224:NA:1.50%:+0.0044%:public: void __cdecl IncrementalSsaBuilder::Insert(void) 1522909:+0.56%:1.28%:+0.0038%:public: unsigned short __cdecl Compiler::optAddAssertion(struct Compiler::AssertionDsc*) 1018306:+2.48%:0.85%:+0.0025%:VisitEHSuccs<0,`AllSuccessorEnumerator::AllSuccessorEnumerator'::`2'::<lambda_1> >891863:+2.70%:0.75%:+0.0022%:public: struct Range __cdecl RangeCheck::ComputeRange(struct BasicBlock*, struct GenTree*, bool) 790041:+4.09%:0.66%:+0.0020%:public: bool __cdecl Compiler::fgMorphBlockStmt(struct BasicBlock*, struct Statement*) 717239:+6.03%:0.60%:+0.0018%:public: struct FlowEdge* __cdecl Compiler::BlockDominancePreds(struct BasicBlock*) 595655:+0.18%:0.50%:+0.0015%: memset 573612:+2.48%:0.48%:+0.0014%:public: void __cdecl Compiler::fgValueNumberPhiDef(struct GenTreeLclVar*, struct BasicBlock*, bool) 551275:+31.48%:0.46%:+0.0014%:public: struct EHblkDsc* __cdecl Compiler::ehGetBlockExnFlowDsc(struct BasicBlock*) 383941:+0.44%:0.32%:+0.0009%:public: void __cdecl Compiler::compInit(classArenaAllocator*, struct CORINFO_METHOD_STRUCT_*, classICorJitInfo*, struct CORINFO_METHOD_INFO*, struct InlineInfo*) 374654:+2.29%:0.31%:+0.0009%:public: struct Scev* __cdecl ScalarEvolutionContext::Simplify(struct Scev*, struct SimplificationAssumptions const &) 367471:+0.29%:0.31%:+0.0009%:private: void __cdecl SsaBuilder::BlockRenameVariables(struct BasicBlock*) 312764:+2.04%:0.26%:+0.0008%:public: bool __cdecl JitHashTable<struct GenTree *, struct JitPtrKeyFuncs<struct GenTree>, struct Scev*, classCompAllocator, classJitHashTableBehavior>::Set(struct GenTree*, struct Scev*, enumJitHashTable<struct GenTree *, struct JitPtrKeyFuncs<struct GenTree>, struct Scev*, classCompAllocator, classJitHashTableBehavior>::SetKind) 304582:+0.07%:0.26%:+0.0008%:public: struct GenTree* __cdecl Compiler::optAssertionProp_LclVar(unsigned __int64 *const &, struct GenTreeLclVarCommon*, struct Statement*) 297432:+4.41%:0.25%:+0.0007%:public: enumRelopEvaluationResult __cdecl ScalarEvolutionContext::EvaluateRelop(unsigned int) 281935:+0.55%:0.24%:+0.0007%:private: void __cdecl JitHashTable<struct CORINFO_FIELD_STRUCT_*, struct JitPtrKeyFuncs<struct CORINFO_FIELD_STRUCT_>, classFieldSeq, classCompAllocator, classJitHashTableBehavior>::Grow(void) 254204:+2.33%:0.21%:+0.0006%:public: static void __cdecl BitSetOps<unsigned __int64 *, 1, struct BitVecTraits *, struct BitVecTraits>::ClearD(struct BitVecTraits*, unsigned __int64 *&) 221830:+1.38%:0.19%:+0.0005%:private: void __cdecl SsaDefArray<class LclSsaVarDsc>::GrowArray(classCompAllocator) 220372:+0.14%:0.18%:+0.0005%:public: enumPhaseStatus __cdecl Compiler::optAssertionPropMain(void) 197369:+3.85%:0.17%:+0.0005%:public: bool __cdecl RangeCheck::ComputeDoesOverflow(struct BasicBlock*, struct GenTree*, struct Range const &) 194313:+0.26%:0.16%:+0.0005%:public: void __cdecl Compiler::optImpliedAssertions(unsigned short, unsigned __int64 *&) 186496:+2.80%:0.16%:+0.0005%:public: struct Range __cdecl RangeCheck::GetRange(struct BasicBlock*, struct GenTree*, bool) 178680:+0.22%:0.15%:+0.0004%:private: void __cdecl SsaBuilder::ComputeIteratedDominanceFrontier(struct BasicBlock*, classJitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>, classCompAllocator, classJitHashTableBehavior> const *, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>*) 175067:+0.24%:0.15%:+0.0004%:public: void __cdecl DataFlow::ForwardAnalysis<class AssertionPropFlowCallback>(classAssertionPropFlowCallback&) 174825:+0.88%:0.15%:+0.0004%:public: void __cdecl ValueNumStore::GetConstantBoundInfo(unsigned int, struct ValueNumStore::ConstantBoundInfo*) 172806:+0.31%:0.14%:+0.0004%:protected: void __cdecl Compiler::compCompile(void **, unsigned int *, classJitFlags*) 172736:+2.73%:0.14%:+0.0004%:public: void __cdecl RangeCheck::MergeEdgeAssertions(struct GenTreeLclVarCommon*, unsigned __int64 *const &, struct Range*) 164132:+3.16%:0.14%:+0.0004%:public: static struct Range __cdecl RangeOps::Merge(struct Range const &, struct Range const &, bool) 162139:+3.63%:0.14%:+0.0004%:public: struct Range __cdecl RangeCheck::ComputeRangeForBinOp(struct BasicBlock*, struct GenTreeOp*, bool) 155473:+0.24%:0.13%:+0.0004%:public: void __cdecl Compiler::optImpliedByTypeOfAssertions(unsigned __int64 *&) 150968:+0.33%:0.13%:+0.0004%:BasicBlock::VisitAllSuccs<`Compiler::optReachable'::`12'::<lambda_1>>147163:+0.33%:0.12%:+0.0004%:public: unsigned short __cdecl Compiler::optGlobalAssertionIsEqualOrNotEqualZero(unsigned __int64 *const &, struct GenTree*) 146382:+0.76%:0.12%:+0.0004%:public: enumPhaseStatus __cdecl Compiler::optVNBasedDeadStoreRemoval(void) 139613:+0.29%:0.12%:+0.0003%: `Compiler::optReachable'::`12'::<lambda_1>::operator() 138149:+0.06%:0.12%:+0.0003%:public: unsigned int __cdecl ValueNumStore::VNForFunc(enumvar_types, enumVNFunc, unsigned int, unsigned int) 136733:+0.65%:0.11%:+0.0003%: `LoopLocalOccurrences::GetOrCreateMap'::`2'::<lambda_1>::operator() 130909:+0.04%:0.11%:+0.0003%:public: unsigned int * __cdecl JitHashTable<struct ValueNumStore::VNDefFuncApp<2>, struct ValueNumStore::VNDefFuncAppKeyFuncs<2>, unsigned int, classCompAllocator, classJitHashTableBehavior>::LookupPointerOrAdd(struct ValueNumStore::VNDefFuncApp<2>, unsigned int) 124058:+0.16%:0.10%:+0.0003%:public: static void __cdecl BitSetOps<unsigned __int64 *, 1, struct BitVecTraits *, struct BitVecTraits>::IntersectionD(struct BitVecTraits*, unsigned __int64 *&, unsigned __int64 *) -374413:-3.47%:0.31%:-0.0009%:public: enumPhaseStatus __cdecl Compiler::fgSsaBuild(void) -7939897:-26.44%:6.65%:-0.0196%:private: void __cdecl SsaBuilder::AddPhiArg(struct BasicBlock*, struct Statement*, struct GenTreePhi*, unsigned int, unsigned int, struct BasicBlock*) -9406160:-14.99%:7.88%:-0.0232%:private: void __cdecl SsaBuilder::InsertPhiFunctions(void) 
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 23, 2024

Closed

@dotnet-policy-servicedotnet-policy-serviceBot
removed this from the 10.0.0 milestone
Sep 22, 2024
@jakobbotschjakobbotsch
reopened this
Oct 7, 2024
@jakobbotschjakobbotsch
mentioned this pull request
Oct 31, 2024

Closed

@jakobbotsch
jakobbotsch
commented
Oct 31, 2024

Copy link


Copy Markdown

MemberAuthor
This seems to also break strength reduction sometimes, need to investigate that.
image
@jakobbotschjakobbotsch
mentioned this pull request
Nov 4, 2024
 Merged

@jakobbotschjakobbotsch
mentioned this pull request
Nov 6, 2024
 Closed

@jakobbotsch
jakobbotsch
commented
Nov 6, 2024

Copy link


Copy Markdown

MemberAuthor
This seems to also break strength reduction sometimes, need to investigate that.
The problem here was the fact that I added the lvTracked check in optLocalHasNonLoopUses. I did that separately in #109505, to take those regressions separately (this was a correctness fix). To resolve those regressions I have then added the necessary liveness computation to this PR: that is, when inserting a local into SSA we now also compute the set of blocks in which that local is live-in, which is then stored in a side table. The cost of that computation does not look to be too bad -- overall TP cost of SSA insertion + liveness computation is around 0.2% for win-x64, with some collections being outliers in both directions. I think that cost is acceptable given that this gets us a fair amount of new strength reductions.
Still have some other regressions to investigate.
@jakobbotsch
jakobbotsch
commented
Nov 6, 2024

Copy link


Copy Markdown

MemberAuthor
cc @dotnet/jit-contrib PTAL @AndyAyersMS -- this now computes liveness for SSA-inserted locals, see my comment above. Can you review those new parts?
Diffs.
I went through a bunch of ASP.NET regressions and they fell into two categories:
  • Some regressions are because of new eliminated comparisons in assertion prop, which can lead to diferent register allocation due to fewer uses of those locals, and this can end up being a regression overall
  • Other regressions happen when IVs created by IV opts do not get a register allocated (nothing new here)
Sometimes strength reduction is also a size regression while being a perfscore regression, but there is nothing new there.
Some stats for number of strength reductions in win-x64 collections with this PR:

aspnet: 1710 -> 1819 (+6.4%)

benchmarks.run_pgo: 2177 -> 2252 (+3.4%)

libraries_tests.run: 10483 -> 11115 (+6.0%) with 74 new misses

realworld: 471 -> 523 (+11%)
@jakobbotsch
jakobbotsch
marked this pull request as ready for review
November 6, 2024 16:32
@jakobbotsch
jakobbotsch
commented
Nov 6, 2024

Copy link


Copy Markdown

MemberAuthor
This seems to be more expensive for arm64, likely because there are more CSEs that are now having their liveness computed.

One thing is that we only very rarely need that liveness, for IV opts only, so if we could only compute it at that point it would be better. But it would require us to keep the list of reaching defs/uses up to date between CSE and IV opts, while currently we only need to keep the liveness information up to date. Probably not impossible, but seems impractical given that assertion prop runs there. An alternative would be to find some way to predict whether we'll need the liveness, but I also didn't see a simple way of doing that from CSE.
AndyAyersMS
AndyAyersMS
approved these changes
Nov 6, 2024
View reviewed changes
Comment threadsrc/coreclr/jit/ssabuilder.cpp
 m_queue.Reset();
 m_queue.Push(use.Block);

 while (!m_queue.Empty())
@AndyAyersMSAndyAyersMSNov 6, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a check that you actually do find the def block, and/or don't try to search past the first block? (that is, def dominates use)
@jakobbotschjakobbotschNov 7, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we necessarily find the reaching def (we might hit a block marked live-in already due to another use), but makes sense to add an assert that we don't get to the first block. I added that assert into AddInsertedSsaLiveIn since it never makes sense to mark these added locals as live into the first BB.
@build-analysisbuild-analysisBot
mentioned this pull request
Nov 7, 2024

Open

3 tasks
@jakobbotsch
jakobbotsch
merged commit 9c1f53e
into
dotnet:mainNov 7, 2024
@jakobbotsch
jakobbotsch
deleted the

ssa-updating

branch
November 7, 2024 10:18

This was referenced Nov 8, 2024

Closed


Closed

@DrewScogginsDrewScoggins
mentioned this pull request
Nov 14, 2024

Closed

@AndyAyersMSAndyAyersMS
mentioned this pull request
Nov 21, 2024

Closed

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Dec 8, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@AndyAyersMSAndyAyersMSAndyAyersMS approved these changes

Assignees

@jakobbotschjakobbotsch

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

JIT: Cloned loop not fully strength reduced on arm64


2 participants

@jakobbotsch@AndyAyersMS

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
JIT: Put all CSEs into SSA - #106637
Merged
jakobbotsch merged 29 commits into
dotnet:mainfrom 
jakobbotsch:ssa-updating
Nov 7, 2024
Merged
JIT: Put all CSEs into SSA#106637
jakobbotsch merged 29 commits into
dotnet:mainfrom 
jakobbotsch:ssa-updating
Conversation
@jakobbotsch
@jakobbotschjakobbotsch
commented
Aug 19, 2024•

edited

Loading

Copy link


Copy Markdown

Member
This adds an SSA updater that can incrementally put locals that were added to the IR into SSA form and starts using this infrastructure from CSE. Previously only single-def CSEs were put into SSA, which can cause e.g. IV opts to miss out on them.
The SSA updater requires all uses and definitions to be supplied. Based on the definitions it is then possible to compute the candidate blocks for phi nodes in the usual way as the iterated dominance frontier. Since we do not have liveness for the local we do not insert the phi definitions eagerly; instead, we recursively compute the reaching def for each use by walking its dominators.

Walking the dominators we either expect to find a real definition, or to hit a block that is part of the iterated dominance frontier of the definitions, in which case we know we have a live phi definition that we can insert.
Inserting phi definitions needs to recursively do the same thing for the phi arguments. To make this faster we could memoize the reaching SSA numbers for each block, though this is not currently done.
This also requires computing the DFS tree, dominators, and dominance frontiers after RBO (which invalidates those). Since the DFS tree and dominator tree is usually computed anyway by IV opts, this is not that costly.

The cost is further reduced by avoiding any of these computations in the single-def case (which CSE already handled). This special case is also moved into the SSA updater.
Finally, since IV opts need liveness to work, the SSA inserter also computes liveness for the inserted locals. This is done by marking all paths from each use back to its reaching definition as having that local live-in.
Fix#109412
@jakobbotsch
JIT: Put all CSEs into SSA
ddf2b02
This adds an SSA updater that can incrementally put locals that were
added to the IR into SSA form and starts using this infrastructure from
CSE. It updates CSE to use this SSA updater to put all CSEs into SSA.
Previously only single-def CSEs were put into SSA, which can cause e.g.
IV opts to miss out on them.
The SSA updater requires all uses and definitions to be supplied. Based
on the definitions it is then possible to compute the candidate blocks
for phi nodes in the usual way as the iterated dominance frontier. Since
we do not have liveness for the local we do not insert the phi
definitions eagerly; instead, we recursively compute the reaching def
for each use by walking its dominators.
Walking the dominators we either expect to find a real definition, or to
hit a block that is part of the iterated dominance frontier of the
definitions, in which case we know we have a live phi definition that we
can insert.
Inserting phi definitions needs to recursively do the same thing for the
phi arguments. To make this faster we could memoize the reaching SSA
numbers for each block, though this is not currently done.
This also requires computing the DFS tree, dominators, and dominance
frontiers after RBO (which invalidates those). The DFS tree and
dominators should be reusable by IV opts in most cases, though currently
we invalidate them after CSE because assertion prop sometimes will
change the flow graph and does not know how to invalidate the DFS tree.
This can also be fixed to improve throughput.
We currently do not get much use out of this because IV opts also need
liveness for the IVs. More specifically it needs to know whether the IVs
are live out of the loop; it should be possible to keep some breadcrumbs
around for inserted locals to cheaply know this, I think.
@ghostghost
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
Aug 19, 2024
@jakobbotsch

This comment was marked as outdated.

Sign in to view

This was referenced Aug 19, 2024

Closed


Closed

@jakobbotsch
jakobbotsch
commented
Aug 20, 2024

Copy link


Copy Markdown

MemberAuthor
TP diffs for benchmarks.run_pgo now look like:
Base:120833007777, Diff:120935018667, +0.0844%317810259:NA:25.72%:+0.2630%:public: void __cdecl FlowGraphDominanceFrontiers::ComputeIteratedDominanceFrontier(struct BasicBlock*, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>*) 162099515:NA:13.12%:+0.1342%:public: static classFlowGraphDominanceFrontiers* __cdecl FlowGraphDominanceFrontiers::Build(classFlowGraphDominatorTree*) 39047720:NA:3.16%:+0.0323%:private: static void __cdecl SsaBuilder::AddNewPhiArg(classCompiler*, struct BasicBlock*, struct Statement*, struct GenTreePhi*, unsigned int, unsigned int, struct BasicBlock*) 38346141:NA:3.10%:+0.0317%:public: bool __cdecl Compiler::fgMorphBlockStmt(struct BasicBlock*, struct Statement*, bool) 18905583:+52.64%:1.53%:+0.0156%:public: virtual void __cdecl CSE_HeuristicCommon::PerformCSE(classCSE_Candidate*) 13835238:NA:1.12%:+0.0114%:private: static struct Statement* __cdecl SsaBuilder::InsertPhi(classCompiler*, struct BasicBlock*, unsigned int) 8753827:+0.85%:0.71%:+0.0072%:public: __cdecl `public: unsigned int __cdecl Compiler::fgRunDfs<class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_1>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_2>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_3>, 0>(class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_1>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_2>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_3>)'::`2'::<lambda_1>::operator()(struct BasicBlock*) const
7221209:+0.28%:0.58%:+0.0060%:public: void * __cdecl ArenaAllocator::allocateMemory(unsigned __int64) 7119522:+2.54%:0.58%:+0.0059%:public: static classFlowGraphDominatorTree* __cdecl FlowGraphDominatorTree::Build(classFlowGraphDfsTree const *) 6499831:+7.25%:0.53%:+0.0054%:private: bool __cdecl jitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>::ensure_capacity(unsigned __int64) 4424130:+12.56%:0.36%:+0.0037%:private: void __cdecl JitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, struct FlowEdge*, classCompAllocator, classJitHashTableBehavior>::Grow(void) 3705888:NA:0.30%:+0.0031%:public: static void __cdecl SsaBuilder::InsertInSsa(classCompiler*, unsigned int, classArrayStack<struct UseDefLocation> &, classArrayStack<struct UseDefLocation> &) 3361499:NA:0.27%:+0.0028%:private: struct UseDefLocation __cdecl IncrementalSsaBuilder::FindOrCreateReachingDef(struct UseDefLocation const &) 2892505:+0.42%:0.23%:+0.0024%:public: unsigned short __cdecl Compiler::optAddAssertion(struct Compiler::AssertionDsc*) 2651134:+0.69%:0.21%:+0.0022%:BasicBlock::VisitAllSuccs<`AllSuccessorEnumerator::AllSuccessorEnumerator'::`2'::<lambda_1>>2535856:+4.24%:0.21%:+0.0021%:public: struct FlowEdge* __cdecl Compiler::BlockPredsWithEH(struct BasicBlock*) 1743270:NA:0.14%:+0.0014%:public: void __cdecl IncrementalSsaBuilder::Insert(void) 1693606:+0.58%:0.14%:+0.0014%:public: void __cdecl RangeCheck::MergeEdgeAssertions(unsigned int, unsigned __int64 *const &, struct Range*) 1583199:NA:0.13%:+0.0013%:private: bool __cdecl IncrementalSsaBuilder::FindReachingDefInBlock(struct UseDefLocation const &, struct BasicBlock*, struct UseDefLocation*) 1557652:+1.89%:0.13%:+0.0013%:public: void __cdecl Compiler::fgValueNumberPhiDef(struct GenTreeLclVar*, struct BasicBlock*, bool) 1349037:+0.75%:0.11%:+0.0011%:VisitEHSuccs<0,`AllSuccessorEnumerator::AllSuccessorEnumerator'::`2'::<lambda_1> >1261895:+0.82%:0.10%:+0.0010%: `Compiler::optReachable'::`12'::<lambda_1>::operator() -5032126:-0.49%:0.41%:-0.0042%: memset -26356661:-50.03%:2.13%:-0.0218%:private: void __cdecl SsaBuilder::AddPhiArg(struct BasicBlock*, struct Statement*, struct GenTreePhi*, unsigned int, unsigned int, struct BasicBlock*) -30940686:-15.02%:2.50%:-0.0256%:private: void __cdecl SsaBuilder::InsertPhiFunctions(void) -36618036:-100.00%:2.96%:-0.0303%:public: bool __cdecl Compiler::fgMorphBlockStmt(struct BasicBlock*, struct Statement*) -139925973:-100.00%:11.32%:-0.1158%:private: void __cdecl SsaBuilder::ComputeDominanceFrontiers(struct BasicBlock**, int, classJitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>, classCompAllocator, classJitHashTableBehavior>*) -322436319:-100.00%:26.09%:-0.2668%:private: void __cdecl SsaBuilder::ComputeIteratedDominanceFrontier(struct BasicBlock*, classJitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>, classCompAllocator, classJitHashTableBehavior> const *, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>*) 
So looks small enough that we can take this change without much more consideration.
One thing is whether we need to memoize reaching defs in some way to avoid quadratic (or even potentially exponential) behavior. Need to think about that a bit.
@jakobbotschjakobbotsch
added this to the 10.0.0 milestone
Aug 20, 2024
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 20, 2024

Closed

@jakobbotsch
jakobbotsch
commented
Aug 21, 2024

Copy link


Copy Markdown

MemberAuthor
benchmarks.run_pgo seems to be one of the least affected collections. The most affected one is benchmarks.run. With some manual reconciliation of renamed functions in the diff, the detailed TP diff for it looks like:
Base:40469192366, Diff:40551805661, +0.2041%16221652:+33.98%:13.58%:+0.0401%:private: void __cdecl SsaBuilder::ComputeDominanceFrontiers(struct BasicBlock**, int, classJitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>, classCompAllocator, classJitHashTableBehavior>*) 14025772:+48.54%:11.74%:+0.0347%:public: virtual void __cdecl CSE_HeuristicCommon::PerformCSE(classCSE_Candidate*) 11802368:NA:9.88%:+0.0292%:private: static void __cdecl SsaBuilder::AddNewPhiArg(classCompiler*, struct BasicBlock*, struct Statement*, struct GenTreePhi*, unsigned int, unsigned int, struct BasicBlock*) 6227765:+2.88%:5.21%:+0.0154%:public: __cdecl `public: unsigned int __cdecl Compiler::fgRunDfs<class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_1>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_2>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_3>, 0>(class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_1>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_2>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_3>)'::`2'::<lambda_1>::operator()(struct BasicBlock*) const
4914583:+0.60%:4.11%:+0.0121%:public: void * __cdecl ArenaAllocator::allocateMemory(unsigned __int64) 4651869:+6.52%:3.89%:+0.0115%:public: static classFlowGraphDominatorTree* __cdecl FlowGraphDominatorTree::Build(classFlowGraphDfsTree const *) 4369172:+17.68%:3.66%:+0.0108%:private: bool __cdecl jitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>::ensure_capacity(unsigned __int64) 3469086:NA:2.90%:+0.0086%:private: static struct Statement* __cdecl SsaBuilder::InsertPhi(classCompiler*, struct BasicBlock*, unsigned int) 2793726:NA:2.34%:+0.0069%:public: static void __cdecl SsaBuilder::InsertInSsa(classCompiler*, unsigned int, classArrayStack<struct UseDefLocation> &, classArrayStack<struct UseDefLocation> &) 2747882:NA:2.30%:+0.0068%:private: struct UseDefLocation __cdecl IncrementalSsaBuilder::FindOrCreateReachingDef(struct UseDefLocation const &) 2745600:+25.93%:2.30%:+0.0068%:private: void __cdecl JitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, struct FlowEdge*, classCompAllocator, classJitHashTableBehavior>::Grow(void) 2538778:+2.19%:2.13%:+0.0063%:public: void __cdecl RangeCheck::MergeEdgeAssertions(unsigned int, unsigned __int64 *const &, struct Range*) 2129497:+12.45%:1.78%:+0.0053%:public: struct FlowEdge* __cdecl Compiler::BlockPredsWithEH(struct BasicBlock*) 1869746:+2.27%:1.57%:+0.0046%:BasicBlock::VisitAllSuccs<`AllSuccessorEnumerator::AllSuccessorEnumerator'::`2'::<lambda_1>>1822996:NA:1.53%:+0.0045%:private: bool __cdecl IncrementalSsaBuilder::FindReachingDefInBlock(struct UseDefLocation const &, struct BasicBlock*, struct UseDefLocation*) 1790224:NA:1.50%:+0.0044%:public: void __cdecl IncrementalSsaBuilder::Insert(void) 1522909:+0.56%:1.28%:+0.0038%:public: unsigned short __cdecl Compiler::optAddAssertion(struct Compiler::AssertionDsc*) 1018306:+2.48%:0.85%:+0.0025%:VisitEHSuccs<0,`AllSuccessorEnumerator::AllSuccessorEnumerator'::`2'::<lambda_1> >891863:+2.70%:0.75%:+0.0022%:public: struct Range __cdecl RangeCheck::ComputeRange(struct BasicBlock*, struct GenTree*, bool) 790041:+4.09%:0.66%:+0.0020%:public: bool __cdecl Compiler::fgMorphBlockStmt(struct BasicBlock*, struct Statement*) 717239:+6.03%:0.60%:+0.0018%:public: struct FlowEdge* __cdecl Compiler::BlockDominancePreds(struct BasicBlock*) 595655:+0.18%:0.50%:+0.0015%: memset 573612:+2.48%:0.48%:+0.0014%:public: void __cdecl Compiler::fgValueNumberPhiDef(struct GenTreeLclVar*, struct BasicBlock*, bool) 551275:+31.48%:0.46%:+0.0014%:public: struct EHblkDsc* __cdecl Compiler::ehGetBlockExnFlowDsc(struct BasicBlock*) 383941:+0.44%:0.32%:+0.0009%:public: void __cdecl Compiler::compInit(classArenaAllocator*, struct CORINFO_METHOD_STRUCT_*, classICorJitInfo*, struct CORINFO_METHOD_INFO*, struct InlineInfo*) 374654:+2.29%:0.31%:+0.0009%:public: struct Scev* __cdecl ScalarEvolutionContext::Simplify(struct Scev*, struct SimplificationAssumptions const &) 367471:+0.29%:0.31%:+0.0009%:private: void __cdecl SsaBuilder::BlockRenameVariables(struct BasicBlock*) 312764:+2.04%:0.26%:+0.0008%:public: bool __cdecl JitHashTable<struct GenTree *, struct JitPtrKeyFuncs<struct GenTree>, struct Scev*, classCompAllocator, classJitHashTableBehavior>::Set(struct GenTree*, struct Scev*, enumJitHashTable<struct GenTree *, struct JitPtrKeyFuncs<struct GenTree>, struct Scev*, classCompAllocator, classJitHashTableBehavior>::SetKind) 304582:+0.07%:0.26%:+0.0008%:public: struct GenTree* __cdecl Compiler::optAssertionProp_LclVar(unsigned __int64 *const &, struct GenTreeLclVarCommon*, struct Statement*) 297432:+4.41%:0.25%:+0.0007%:public: enumRelopEvaluationResult __cdecl ScalarEvolutionContext::EvaluateRelop(unsigned int) 281935:+0.55%:0.24%:+0.0007%:private: void __cdecl JitHashTable<struct CORINFO_FIELD_STRUCT_*, struct JitPtrKeyFuncs<struct CORINFO_FIELD_STRUCT_>, classFieldSeq, classCompAllocator, classJitHashTableBehavior>::Grow(void) 254204:+2.33%:0.21%:+0.0006%:public: static void __cdecl BitSetOps<unsigned __int64 *, 1, struct BitVecTraits *, struct BitVecTraits>::ClearD(struct BitVecTraits*, unsigned __int64 *&) 221830:+1.38%:0.19%:+0.0005%:private: void __cdecl SsaDefArray<class LclSsaVarDsc>::GrowArray(classCompAllocator) 220372:+0.14%:0.18%:+0.0005%:public: enumPhaseStatus __cdecl Compiler::optAssertionPropMain(void) 197369:+3.85%:0.17%:+0.0005%:public: bool __cdecl RangeCheck::ComputeDoesOverflow(struct BasicBlock*, struct GenTree*, struct Range const &) 194313:+0.26%:0.16%:+0.0005%:public: void __cdecl Compiler::optImpliedAssertions(unsigned short, unsigned __int64 *&) 186496:+2.80%:0.16%:+0.0005%:public: struct Range __cdecl RangeCheck::GetRange(struct BasicBlock*, struct GenTree*, bool) 178680:+0.22%:0.15%:+0.0004%:private: void __cdecl SsaBuilder::ComputeIteratedDominanceFrontier(struct BasicBlock*, classJitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>, classCompAllocator, classJitHashTableBehavior> const *, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>*) 175067:+0.24%:0.15%:+0.0004%:public: void __cdecl DataFlow::ForwardAnalysis<class AssertionPropFlowCallback>(classAssertionPropFlowCallback&) 174825:+0.88%:0.15%:+0.0004%:public: void __cdecl ValueNumStore::GetConstantBoundInfo(unsigned int, struct ValueNumStore::ConstantBoundInfo*) 172806:+0.31%:0.14%:+0.0004%:protected: void __cdecl Compiler::compCompile(void **, unsigned int *, classJitFlags*) 172736:+2.73%:0.14%:+0.0004%:public: void __cdecl RangeCheck::MergeEdgeAssertions(struct GenTreeLclVarCommon*, unsigned __int64 *const &, struct Range*) 164132:+3.16%:0.14%:+0.0004%:public: static struct Range __cdecl RangeOps::Merge(struct Range const &, struct Range const &, bool) 162139:+3.63%:0.14%:+0.0004%:public: struct Range __cdecl RangeCheck::ComputeRangeForBinOp(struct BasicBlock*, struct GenTreeOp*, bool) 155473:+0.24%:0.13%:+0.0004%:public: void __cdecl Compiler::optImpliedByTypeOfAssertions(unsigned __int64 *&) 150968:+0.33%:0.13%:+0.0004%:BasicBlock::VisitAllSuccs<`Compiler::optReachable'::`12'::<lambda_1>>147163:+0.33%:0.12%:+0.0004%:public: unsigned short __cdecl Compiler::optGlobalAssertionIsEqualOrNotEqualZero(unsigned __int64 *const &, struct GenTree*) 146382:+0.76%:0.12%:+0.0004%:public: enumPhaseStatus __cdecl Compiler::optVNBasedDeadStoreRemoval(void) 139613:+0.29%:0.12%:+0.0003%: `Compiler::optReachable'::`12'::<lambda_1>::operator() 138149:+0.06%:0.12%:+0.0003%:public: unsigned int __cdecl ValueNumStore::VNForFunc(enumvar_types, enumVNFunc, unsigned int, unsigned int) 136733:+0.65%:0.11%:+0.0003%: `LoopLocalOccurrences::GetOrCreateMap'::`2'::<lambda_1>::operator() 130909:+0.04%:0.11%:+0.0003%:public: unsigned int * __cdecl JitHashTable<struct ValueNumStore::VNDefFuncApp<2>, struct ValueNumStore::VNDefFuncAppKeyFuncs<2>, unsigned int, classCompAllocator, classJitHashTableBehavior>::LookupPointerOrAdd(struct ValueNumStore::VNDefFuncApp<2>, unsigned int) 124058:+0.16%:0.10%:+0.0003%:public: static void __cdecl BitSetOps<unsigned __int64 *, 1, struct BitVecTraits *, struct BitVecTraits>::IntersectionD(struct BitVecTraits*, unsigned __int64 *&, unsigned __int64 *) -374413:-3.47%:0.31%:-0.0009%:public: enumPhaseStatus __cdecl Compiler::fgSsaBuild(void) -7939897:-26.44%:6.65%:-0.0196%:private: void __cdecl SsaBuilder::AddPhiArg(struct BasicBlock*, struct Statement*, struct GenTreePhi*, unsigned int, unsigned int, struct BasicBlock*) -9406160:-14.99%:7.88%:-0.0232%:private: void __cdecl SsaBuilder::InsertPhiFunctions(void) 
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 23, 2024

Closed

@dotnet-policy-servicedotnet-policy-serviceBot
removed this from the 10.0.0 milestone
Sep 22, 2024
@jakobbotschjakobbotsch
reopened this
Oct 7, 2024
@jakobbotschjakobbotsch
mentioned this pull request
Oct 31, 2024

Closed

@jakobbotsch
jakobbotsch
commented
Oct 31, 2024

Copy link


Copy Markdown

MemberAuthor
This seems to also break strength reduction sometimes, need to investigate that.
image
@jakobbotschjakobbotsch
mentioned this pull request
Nov 4, 2024
 Merged

@jakobbotschjakobbotsch
mentioned this pull request
Nov 6, 2024
 Closed

@jakobbotsch
jakobbotsch
commented
Nov 6, 2024

Copy link


Copy Markdown

MemberAuthor
This seems to also break strength reduction sometimes, need to investigate that.
The problem here was the fact that I added the lvTracked check in optLocalHasNonLoopUses. I did that separately in #109505, to take those regressions separately (this was a correctness fix). To resolve those regressions I have then added the necessary liveness computation to this PR: that is, when inserting a local into SSA we now also compute the set of blocks in which that local is live-in, which is then stored in a side table. The cost of that computation does not look to be too bad -- overall TP cost of SSA insertion + liveness computation is around 0.2% for win-x64, with some collections being outliers in both directions. I think that cost is acceptable given that this gets us a fair amount of new strength reductions.
Still have some other regressions to investigate.
@jakobbotsch
jakobbotsch
commented
Nov 6, 2024

Copy link


Copy Markdown

MemberAuthor
cc @dotnet/jit-contrib PTAL @AndyAyersMS -- this now computes liveness for SSA-inserted locals, see my comment above. Can you review those new parts?
Diffs.
I went through a bunch of ASP.NET regressions and they fell into two categories:
  • Some regressions are because of new eliminated comparisons in assertion prop, which can lead to diferent register allocation due to fewer uses of those locals, and this can end up being a regression overall
  • Other regressions happen when IVs created by IV opts do not get a register allocated (nothing new here)
Sometimes strength reduction is also a size regression while being a perfscore regression, but there is nothing new there.
Some stats for number of strength reductions in win-x64 collections with this PR:

aspnet: 1710 -> 1819 (+6.4%)

benchmarks.run_pgo: 2177 -> 2252 (+3.4%)

libraries_tests.run: 10483 -> 11115 (+6.0%) with 74 new misses

realworld: 471 -> 523 (+11%)
@jakobbotsch
jakobbotsch
marked this pull request as ready for review
November 6, 2024 16:32
@jakobbotsch
jakobbotsch
commented
Nov 6, 2024

Copy link


Copy Markdown

MemberAuthor
This seems to be more expensive for arm64, likely because there are more CSEs that are now having their liveness computed.

One thing is that we only very rarely need that liveness, for IV opts only, so if we could only compute it at that point it would be better. But it would require us to keep the list of reaching defs/uses up to date between CSE and IV opts, while currently we only need to keep the liveness information up to date. Probably not impossible, but seems impractical given that assertion prop runs there. An alternative would be to find some way to predict whether we'll need the liveness, but I also didn't see a simple way of doing that from CSE.
AndyAyersMS
AndyAyersMS
approved these changes
Nov 6, 2024
View reviewed changes
Comment threadsrc/coreclr/jit/ssabuilder.cpp
 m_queue.Reset();
 m_queue.Push(use.Block);

 while (!m_queue.Empty())
@AndyAyersMSAndyAyersMSNov 6, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a check that you actually do find the def block, and/or don't try to search past the first block? (that is, def dominates use)
@jakobbotschjakobbotschNov 7, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we necessarily find the reaching def (we might hit a block marked live-in already due to another use), but makes sense to add an assert that we don't get to the first block. I added that assert into AddInsertedSsaLiveIn since it never makes sense to mark these added locals as live into the first BB.
@build-analysisbuild-analysisBot
mentioned this pull request
Nov 7, 2024

Open

3 tasks
@jakobbotsch
jakobbotsch
merged commit 9c1f53e
into
dotnet:mainNov 7, 2024
@jakobbotsch
jakobbotsch
deleted the

ssa-updating

branch
November 7, 2024 10:18

This was referenced Nov 8, 2024

Closed


Closed

@DrewScogginsDrewScoggins
mentioned this pull request
Nov 14, 2024

Closed

@AndyAyersMSAndyAyersMS
mentioned this pull request
Nov 21, 2024

Closed

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Dec 8, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@AndyAyersMSAndyAyersMSAndyAyersMS approved these changes

Assignees

@jakobbotschjakobbotsch

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

JIT: Cloned loop not fully strength reduced on arm64


2 participants

@jakobbotsch@AndyAyersMS

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
JIT: Put all CSEs into SSA - #106637
Merged
jakobbotsch merged 29 commits into
dotnet:mainfrom 
jakobbotsch:ssa-updating
Nov 7, 2024
Merged
JIT: Put all CSEs into SSA#106637
jakobbotsch merged 29 commits into
dotnet:mainfrom 
jakobbotsch:ssa-updating
Conversation
@jakobbotsch
@jakobbotschjakobbotsch
commented
Aug 19, 2024•

edited

Loading

Copy link


Copy Markdown

Member
This adds an SSA updater that can incrementally put locals that were added to the IR into SSA form and starts using this infrastructure from CSE. Previously only single-def CSEs were put into SSA, which can cause e.g. IV opts to miss out on them.
The SSA updater requires all uses and definitions to be supplied. Based on the definitions it is then possible to compute the candidate blocks for phi nodes in the usual way as the iterated dominance frontier. Since we do not have liveness for the local we do not insert the phi definitions eagerly; instead, we recursively compute the reaching def for each use by walking its dominators.

Walking the dominators we either expect to find a real definition, or to hit a block that is part of the iterated dominance frontier of the definitions, in which case we know we have a live phi definition that we can insert.
Inserting phi definitions needs to recursively do the same thing for the phi arguments. To make this faster we could memoize the reaching SSA numbers for each block, though this is not currently done.
This also requires computing the DFS tree, dominators, and dominance frontiers after RBO (which invalidates those). Since the DFS tree and dominator tree is usually computed anyway by IV opts, this is not that costly.

The cost is further reduced by avoiding any of these computations in the single-def case (which CSE already handled). This special case is also moved into the SSA updater.
Finally, since IV opts need liveness to work, the SSA inserter also computes liveness for the inserted locals. This is done by marking all paths from each use back to its reaching definition as having that local live-in.
Fix#109412
@jakobbotsch
JIT: Put all CSEs into SSA
ddf2b02
This adds an SSA updater that can incrementally put locals that were
added to the IR into SSA form and starts using this infrastructure from
CSE. It updates CSE to use this SSA updater to put all CSEs into SSA.
Previously only single-def CSEs were put into SSA, which can cause e.g.
IV opts to miss out on them.
The SSA updater requires all uses and definitions to be supplied. Based
on the definitions it is then possible to compute the candidate blocks
for phi nodes in the usual way as the iterated dominance frontier. Since
we do not have liveness for the local we do not insert the phi
definitions eagerly; instead, we recursively compute the reaching def
for each use by walking its dominators.
Walking the dominators we either expect to find a real definition, or to
hit a block that is part of the iterated dominance frontier of the
definitions, in which case we know we have a live phi definition that we
can insert.
Inserting phi definitions needs to recursively do the same thing for the
phi arguments. To make this faster we could memoize the reaching SSA
numbers for each block, though this is not currently done.
This also requires computing the DFS tree, dominators, and dominance
frontiers after RBO (which invalidates those). The DFS tree and
dominators should be reusable by IV opts in most cases, though currently
we invalidate them after CSE because assertion prop sometimes will
change the flow graph and does not know how to invalidate the DFS tree.
This can also be fixed to improve throughput.
We currently do not get much use out of this because IV opts also need
liveness for the IVs. More specifically it needs to know whether the IVs
are live out of the loop; it should be possible to keep some breadcrumbs
around for inserted locals to cheaply know this, I think.
@ghostghost
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
Aug 19, 2024
@jakobbotsch

This comment was marked as outdated.

Sign in to view

This was referenced Aug 19, 2024

Closed


Closed

@jakobbotsch
jakobbotsch
commented
Aug 20, 2024

Copy link


Copy Markdown

MemberAuthor
TP diffs for benchmarks.run_pgo now look like:
Base:120833007777, Diff:120935018667, +0.0844%317810259:NA:25.72%:+0.2630%:public: void __cdecl FlowGraphDominanceFrontiers::ComputeIteratedDominanceFrontier(struct BasicBlock*, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>*) 162099515:NA:13.12%:+0.1342%:public: static classFlowGraphDominanceFrontiers* __cdecl FlowGraphDominanceFrontiers::Build(classFlowGraphDominatorTree*) 39047720:NA:3.16%:+0.0323%:private: static void __cdecl SsaBuilder::AddNewPhiArg(classCompiler*, struct BasicBlock*, struct Statement*, struct GenTreePhi*, unsigned int, unsigned int, struct BasicBlock*) 38346141:NA:3.10%:+0.0317%:public: bool __cdecl Compiler::fgMorphBlockStmt(struct BasicBlock*, struct Statement*, bool) 18905583:+52.64%:1.53%:+0.0156%:public: virtual void __cdecl CSE_HeuristicCommon::PerformCSE(classCSE_Candidate*) 13835238:NA:1.12%:+0.0114%:private: static struct Statement* __cdecl SsaBuilder::InsertPhi(classCompiler*, struct BasicBlock*, unsigned int) 8753827:+0.85%:0.71%:+0.0072%:public: __cdecl `public: unsigned int __cdecl Compiler::fgRunDfs<class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_1>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_2>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_3>, 0>(class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_1>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_2>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_3>)'::`2'::<lambda_1>::operator()(struct BasicBlock*) const
7221209:+0.28%:0.58%:+0.0060%:public: void * __cdecl ArenaAllocator::allocateMemory(unsigned __int64) 7119522:+2.54%:0.58%:+0.0059%:public: static classFlowGraphDominatorTree* __cdecl FlowGraphDominatorTree::Build(classFlowGraphDfsTree const *) 6499831:+7.25%:0.53%:+0.0054%:private: bool __cdecl jitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>::ensure_capacity(unsigned __int64) 4424130:+12.56%:0.36%:+0.0037%:private: void __cdecl JitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, struct FlowEdge*, classCompAllocator, classJitHashTableBehavior>::Grow(void) 3705888:NA:0.30%:+0.0031%:public: static void __cdecl SsaBuilder::InsertInSsa(classCompiler*, unsigned int, classArrayStack<struct UseDefLocation> &, classArrayStack<struct UseDefLocation> &) 3361499:NA:0.27%:+0.0028%:private: struct UseDefLocation __cdecl IncrementalSsaBuilder::FindOrCreateReachingDef(struct UseDefLocation const &) 2892505:+0.42%:0.23%:+0.0024%:public: unsigned short __cdecl Compiler::optAddAssertion(struct Compiler::AssertionDsc*) 2651134:+0.69%:0.21%:+0.0022%:BasicBlock::VisitAllSuccs<`AllSuccessorEnumerator::AllSuccessorEnumerator'::`2'::<lambda_1>>2535856:+4.24%:0.21%:+0.0021%:public: struct FlowEdge* __cdecl Compiler::BlockPredsWithEH(struct BasicBlock*) 1743270:NA:0.14%:+0.0014%:public: void __cdecl IncrementalSsaBuilder::Insert(void) 1693606:+0.58%:0.14%:+0.0014%:public: void __cdecl RangeCheck::MergeEdgeAssertions(unsigned int, unsigned __int64 *const &, struct Range*) 1583199:NA:0.13%:+0.0013%:private: bool __cdecl IncrementalSsaBuilder::FindReachingDefInBlock(struct UseDefLocation const &, struct BasicBlock*, struct UseDefLocation*) 1557652:+1.89%:0.13%:+0.0013%:public: void __cdecl Compiler::fgValueNumberPhiDef(struct GenTreeLclVar*, struct BasicBlock*, bool) 1349037:+0.75%:0.11%:+0.0011%:VisitEHSuccs<0,`AllSuccessorEnumerator::AllSuccessorEnumerator'::`2'::<lambda_1> >1261895:+0.82%:0.10%:+0.0010%: `Compiler::optReachable'::`12'::<lambda_1>::operator() -5032126:-0.49%:0.41%:-0.0042%: memset -26356661:-50.03%:2.13%:-0.0218%:private: void __cdecl SsaBuilder::AddPhiArg(struct BasicBlock*, struct Statement*, struct GenTreePhi*, unsigned int, unsigned int, struct BasicBlock*) -30940686:-15.02%:2.50%:-0.0256%:private: void __cdecl SsaBuilder::InsertPhiFunctions(void) -36618036:-100.00%:2.96%:-0.0303%:public: bool __cdecl Compiler::fgMorphBlockStmt(struct BasicBlock*, struct Statement*) -139925973:-100.00%:11.32%:-0.1158%:private: void __cdecl SsaBuilder::ComputeDominanceFrontiers(struct BasicBlock**, int, classJitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>, classCompAllocator, classJitHashTableBehavior>*) -322436319:-100.00%:26.09%:-0.2668%:private: void __cdecl SsaBuilder::ComputeIteratedDominanceFrontier(struct BasicBlock*, classJitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>, classCompAllocator, classJitHashTableBehavior> const *, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>*) 
So looks small enough that we can take this change without much more consideration.
One thing is whether we need to memoize reaching defs in some way to avoid quadratic (or even potentially exponential) behavior. Need to think about that a bit.
@jakobbotschjakobbotsch
added this to the 10.0.0 milestone
Aug 20, 2024
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 20, 2024

Closed

@jakobbotsch
jakobbotsch
commented
Aug 21, 2024

Copy link


Copy Markdown

MemberAuthor
benchmarks.run_pgo seems to be one of the least affected collections. The most affected one is benchmarks.run. With some manual reconciliation of renamed functions in the diff, the detailed TP diff for it looks like:
Base:40469192366, Diff:40551805661, +0.2041%16221652:+33.98%:13.58%:+0.0401%:private: void __cdecl SsaBuilder::ComputeDominanceFrontiers(struct BasicBlock**, int, classJitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>, classCompAllocator, classJitHashTableBehavior>*) 14025772:+48.54%:11.74%:+0.0347%:public: virtual void __cdecl CSE_HeuristicCommon::PerformCSE(classCSE_Candidate*) 11802368:NA:9.88%:+0.0292%:private: static void __cdecl SsaBuilder::AddNewPhiArg(classCompiler*, struct BasicBlock*, struct Statement*, struct GenTreePhi*, unsigned int, unsigned int, struct BasicBlock*) 6227765:+2.88%:5.21%:+0.0154%:public: __cdecl `public: unsigned int __cdecl Compiler::fgRunDfs<class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_1>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_2>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_3>, 0>(class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_1>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_2>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_3>)'::`2'::<lambda_1>::operator()(struct BasicBlock*) const
4914583:+0.60%:4.11%:+0.0121%:public: void * __cdecl ArenaAllocator::allocateMemory(unsigned __int64) 4651869:+6.52%:3.89%:+0.0115%:public: static classFlowGraphDominatorTree* __cdecl FlowGraphDominatorTree::Build(classFlowGraphDfsTree const *) 4369172:+17.68%:3.66%:+0.0108%:private: bool __cdecl jitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>::ensure_capacity(unsigned __int64) 3469086:NA:2.90%:+0.0086%:private: static struct Statement* __cdecl SsaBuilder::InsertPhi(classCompiler*, struct BasicBlock*, unsigned int) 2793726:NA:2.34%:+0.0069%:public: static void __cdecl SsaBuilder::InsertInSsa(classCompiler*, unsigned int, classArrayStack<struct UseDefLocation> &, classArrayStack<struct UseDefLocation> &) 2747882:NA:2.30%:+0.0068%:private: struct UseDefLocation __cdecl IncrementalSsaBuilder::FindOrCreateReachingDef(struct UseDefLocation const &) 2745600:+25.93%:2.30%:+0.0068%:private: void __cdecl JitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, struct FlowEdge*, classCompAllocator, classJitHashTableBehavior>::Grow(void) 2538778:+2.19%:2.13%:+0.0063%:public: void __cdecl RangeCheck::MergeEdgeAssertions(unsigned int, unsigned __int64 *const &, struct Range*) 2129497:+12.45%:1.78%:+0.0053%:public: struct FlowEdge* __cdecl Compiler::BlockPredsWithEH(struct BasicBlock*) 1869746:+2.27%:1.57%:+0.0046%:BasicBlock::VisitAllSuccs<`AllSuccessorEnumerator::AllSuccessorEnumerator'::`2'::<lambda_1>>1822996:NA:1.53%:+0.0045%:private: bool __cdecl IncrementalSsaBuilder::FindReachingDefInBlock(struct UseDefLocation const &, struct BasicBlock*, struct UseDefLocation*) 1790224:NA:1.50%:+0.0044%:public: void __cdecl IncrementalSsaBuilder::Insert(void) 1522909:+0.56%:1.28%:+0.0038%:public: unsigned short __cdecl Compiler::optAddAssertion(struct Compiler::AssertionDsc*) 1018306:+2.48%:0.85%:+0.0025%:VisitEHSuccs<0,`AllSuccessorEnumerator::AllSuccessorEnumerator'::`2'::<lambda_1> >891863:+2.70%:0.75%:+0.0022%:public: struct Range __cdecl RangeCheck::ComputeRange(struct BasicBlock*, struct GenTree*, bool) 790041:+4.09%:0.66%:+0.0020%:public: bool __cdecl Compiler::fgMorphBlockStmt(struct BasicBlock*, struct Statement*) 717239:+6.03%:0.60%:+0.0018%:public: struct FlowEdge* __cdecl Compiler::BlockDominancePreds(struct BasicBlock*) 595655:+0.18%:0.50%:+0.0015%: memset 573612:+2.48%:0.48%:+0.0014%:public: void __cdecl Compiler::fgValueNumberPhiDef(struct GenTreeLclVar*, struct BasicBlock*, bool) 551275:+31.48%:0.46%:+0.0014%:public: struct EHblkDsc* __cdecl Compiler::ehGetBlockExnFlowDsc(struct BasicBlock*) 383941:+0.44%:0.32%:+0.0009%:public: void __cdecl Compiler::compInit(classArenaAllocator*, struct CORINFO_METHOD_STRUCT_*, classICorJitInfo*, struct CORINFO_METHOD_INFO*, struct InlineInfo*) 374654:+2.29%:0.31%:+0.0009%:public: struct Scev* __cdecl ScalarEvolutionContext::Simplify(struct Scev*, struct SimplificationAssumptions const &) 367471:+0.29%:0.31%:+0.0009%:private: void __cdecl SsaBuilder::BlockRenameVariables(struct BasicBlock*) 312764:+2.04%:0.26%:+0.0008%:public: bool __cdecl JitHashTable<struct GenTree *, struct JitPtrKeyFuncs<struct GenTree>, struct Scev*, classCompAllocator, classJitHashTableBehavior>::Set(struct GenTree*, struct Scev*, enumJitHashTable<struct GenTree *, struct JitPtrKeyFuncs<struct GenTree>, struct Scev*, classCompAllocator, classJitHashTableBehavior>::SetKind) 304582:+0.07%:0.26%:+0.0008%:public: struct GenTree* __cdecl Compiler::optAssertionProp_LclVar(unsigned __int64 *const &, struct GenTreeLclVarCommon*, struct Statement*) 297432:+4.41%:0.25%:+0.0007%:public: enumRelopEvaluationResult __cdecl ScalarEvolutionContext::EvaluateRelop(unsigned int) 281935:+0.55%:0.24%:+0.0007%:private: void __cdecl JitHashTable<struct CORINFO_FIELD_STRUCT_*, struct JitPtrKeyFuncs<struct CORINFO_FIELD_STRUCT_>, classFieldSeq, classCompAllocator, classJitHashTableBehavior>::Grow(void) 254204:+2.33%:0.21%:+0.0006%:public: static void __cdecl BitSetOps<unsigned __int64 *, 1, struct BitVecTraits *, struct BitVecTraits>::ClearD(struct BitVecTraits*, unsigned __int64 *&) 221830:+1.38%:0.19%:+0.0005%:private: void __cdecl SsaDefArray<class LclSsaVarDsc>::GrowArray(classCompAllocator) 220372:+0.14%:0.18%:+0.0005%:public: enumPhaseStatus __cdecl Compiler::optAssertionPropMain(void) 197369:+3.85%:0.17%:+0.0005%:public: bool __cdecl RangeCheck::ComputeDoesOverflow(struct BasicBlock*, struct GenTree*, struct Range const &) 194313:+0.26%:0.16%:+0.0005%:public: void __cdecl Compiler::optImpliedAssertions(unsigned short, unsigned __int64 *&) 186496:+2.80%:0.16%:+0.0005%:public: struct Range __cdecl RangeCheck::GetRange(struct BasicBlock*, struct GenTree*, bool) 178680:+0.22%:0.15%:+0.0004%:private: void __cdecl SsaBuilder::ComputeIteratedDominanceFrontier(struct BasicBlock*, classJitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>, classCompAllocator, classJitHashTableBehavior> const *, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>*) 175067:+0.24%:0.15%:+0.0004%:public: void __cdecl DataFlow::ForwardAnalysis<class AssertionPropFlowCallback>(classAssertionPropFlowCallback&) 174825:+0.88%:0.15%:+0.0004%:public: void __cdecl ValueNumStore::GetConstantBoundInfo(unsigned int, struct ValueNumStore::ConstantBoundInfo*) 172806:+0.31%:0.14%:+0.0004%:protected: void __cdecl Compiler::compCompile(void **, unsigned int *, classJitFlags*) 172736:+2.73%:0.14%:+0.0004%:public: void __cdecl RangeCheck::MergeEdgeAssertions(struct GenTreeLclVarCommon*, unsigned __int64 *const &, struct Range*) 164132:+3.16%:0.14%:+0.0004%:public: static struct Range __cdecl RangeOps::Merge(struct Range const &, struct Range const &, bool) 162139:+3.63%:0.14%:+0.0004%:public: struct Range __cdecl RangeCheck::ComputeRangeForBinOp(struct BasicBlock*, struct GenTreeOp*, bool) 155473:+0.24%:0.13%:+0.0004%:public: void __cdecl Compiler::optImpliedByTypeOfAssertions(unsigned __int64 *&) 150968:+0.33%:0.13%:+0.0004%:BasicBlock::VisitAllSuccs<`Compiler::optReachable'::`12'::<lambda_1>>147163:+0.33%:0.12%:+0.0004%:public: unsigned short __cdecl Compiler::optGlobalAssertionIsEqualOrNotEqualZero(unsigned __int64 *const &, struct GenTree*) 146382:+0.76%:0.12%:+0.0004%:public: enumPhaseStatus __cdecl Compiler::optVNBasedDeadStoreRemoval(void) 139613:+0.29%:0.12%:+0.0003%: `Compiler::optReachable'::`12'::<lambda_1>::operator() 138149:+0.06%:0.12%:+0.0003%:public: unsigned int __cdecl ValueNumStore::VNForFunc(enumvar_types, enumVNFunc, unsigned int, unsigned int) 136733:+0.65%:0.11%:+0.0003%: `LoopLocalOccurrences::GetOrCreateMap'::`2'::<lambda_1>::operator() 130909:+0.04%:0.11%:+0.0003%:public: unsigned int * __cdecl JitHashTable<struct ValueNumStore::VNDefFuncApp<2>, struct ValueNumStore::VNDefFuncAppKeyFuncs<2>, unsigned int, classCompAllocator, classJitHashTableBehavior>::LookupPointerOrAdd(struct ValueNumStore::VNDefFuncApp<2>, unsigned int) 124058:+0.16%:0.10%:+0.0003%:public: static void __cdecl BitSetOps<unsigned __int64 *, 1, struct BitVecTraits *, struct BitVecTraits>::IntersectionD(struct BitVecTraits*, unsigned __int64 *&, unsigned __int64 *) -374413:-3.47%:0.31%:-0.0009%:public: enumPhaseStatus __cdecl Compiler::fgSsaBuild(void) -7939897:-26.44%:6.65%:-0.0196%:private: void __cdecl SsaBuilder::AddPhiArg(struct BasicBlock*, struct Statement*, struct GenTreePhi*, unsigned int, unsigned int, struct BasicBlock*) -9406160:-14.99%:7.88%:-0.0232%:private: void __cdecl SsaBuilder::InsertPhiFunctions(void) 
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 23, 2024

Closed

@dotnet-policy-servicedotnet-policy-serviceBot
removed this from the 10.0.0 milestone
Sep 22, 2024
@jakobbotschjakobbotsch
reopened this
Oct 7, 2024
@jakobbotschjakobbotsch
mentioned this pull request
Oct 31, 2024

Closed

@jakobbotsch
jakobbotsch
commented
Oct 31, 2024

Copy link


Copy Markdown

MemberAuthor
This seems to also break strength reduction sometimes, need to investigate that.
image
@jakobbotschjakobbotsch
mentioned this pull request
Nov 4, 2024
 Merged

@jakobbotschjakobbotsch
mentioned this pull request
Nov 6, 2024
 Closed

@jakobbotsch
jakobbotsch
commented
Nov 6, 2024

Copy link


Copy Markdown

MemberAuthor
This seems to also break strength reduction sometimes, need to investigate that.
The problem here was the fact that I added the lvTracked check in optLocalHasNonLoopUses. I did that separately in #109505, to take those regressions separately (this was a correctness fix). To resolve those regressions I have then added the necessary liveness computation to this PR: that is, when inserting a local into SSA we now also compute the set of blocks in which that local is live-in, which is then stored in a side table. The cost of that computation does not look to be too bad -- overall TP cost of SSA insertion + liveness computation is around 0.2% for win-x64, with some collections being outliers in both directions. I think that cost is acceptable given that this gets us a fair amount of new strength reductions.
Still have some other regressions to investigate.
@jakobbotsch
jakobbotsch
commented
Nov 6, 2024

Copy link


Copy Markdown

MemberAuthor
cc @dotnet/jit-contrib PTAL @AndyAyersMS -- this now computes liveness for SSA-inserted locals, see my comment above. Can you review those new parts?
Diffs.
I went through a bunch of ASP.NET regressions and they fell into two categories:
  • Some regressions are because of new eliminated comparisons in assertion prop, which can lead to diferent register allocation due to fewer uses of those locals, and this can end up being a regression overall
  • Other regressions happen when IVs created by IV opts do not get a register allocated (nothing new here)
Sometimes strength reduction is also a size regression while being a perfscore regression, but there is nothing new there.
Some stats for number of strength reductions in win-x64 collections with this PR:

aspnet: 1710 -> 1819 (+6.4%)

benchmarks.run_pgo: 2177 -> 2252 (+3.4%)

libraries_tests.run: 10483 -> 11115 (+6.0%) with 74 new misses

realworld: 471 -> 523 (+11%)
@jakobbotsch
jakobbotsch
marked this pull request as ready for review
November 6, 2024 16:32
@jakobbotsch
jakobbotsch
commented
Nov 6, 2024

Copy link


Copy Markdown

MemberAuthor
This seems to be more expensive for arm64, likely because there are more CSEs that are now having their liveness computed.

One thing is that we only very rarely need that liveness, for IV opts only, so if we could only compute it at that point it would be better. But it would require us to keep the list of reaching defs/uses up to date between CSE and IV opts, while currently we only need to keep the liveness information up to date. Probably not impossible, but seems impractical given that assertion prop runs there. An alternative would be to find some way to predict whether we'll need the liveness, but I also didn't see a simple way of doing that from CSE.
AndyAyersMS
AndyAyersMS
approved these changes
Nov 6, 2024
View reviewed changes
Comment threadsrc/coreclr/jit/ssabuilder.cpp
 m_queue.Reset();
 m_queue.Push(use.Block);

 while (!m_queue.Empty())
@AndyAyersMSAndyAyersMSNov 6, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a check that you actually do find the def block, and/or don't try to search past the first block? (that is, def dominates use)
@jakobbotschjakobbotschNov 7, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we necessarily find the reaching def (we might hit a block marked live-in already due to another use), but makes sense to add an assert that we don't get to the first block. I added that assert into AddInsertedSsaLiveIn since it never makes sense to mark these added locals as live into the first BB.
@build-analysisbuild-analysisBot
mentioned this pull request
Nov 7, 2024

Open

3 tasks
@jakobbotsch
jakobbotsch
merged commit 9c1f53e
into
dotnet:mainNov 7, 2024
@jakobbotsch
jakobbotsch
deleted the

ssa-updating

branch
November 7, 2024 10:18

This was referenced Nov 8, 2024

Closed


Closed

@DrewScogginsDrewScoggins
mentioned this pull request
Nov 14, 2024

Closed

@AndyAyersMSAndyAyersMS
mentioned this pull request
Nov 21, 2024

Closed

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Dec 8, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@AndyAyersMSAndyAyersMSAndyAyersMS approved these changes

Assignees

@jakobbotschjakobbotsch

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

JIT: Cloned loop not fully strength reduced on arm64


2 participants

@jakobbotsch@AndyAyersMS

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
JIT: Put all CSEs into SSA - #106637
Merged
jakobbotsch merged 29 commits into
dotnet:mainfrom 
jakobbotsch:ssa-updating
Nov 7, 2024
Merged
JIT: Put all CSEs into SSA#106637
jakobbotsch merged 29 commits into
dotnet:mainfrom 
jakobbotsch:ssa-updating
Conversation
@jakobbotsch
@jakobbotschjakobbotsch
commented
Aug 19, 2024•

edited

Loading

Copy link


Copy Markdown

Member
This adds an SSA updater that can incrementally put locals that were added to the IR into SSA form and starts using this infrastructure from CSE. Previously only single-def CSEs were put into SSA, which can cause e.g. IV opts to miss out on them.
The SSA updater requires all uses and definitions to be supplied. Based on the definitions it is then possible to compute the candidate blocks for phi nodes in the usual way as the iterated dominance frontier. Since we do not have liveness for the local we do not insert the phi definitions eagerly; instead, we recursively compute the reaching def for each use by walking its dominators.

Walking the dominators we either expect to find a real definition, or to hit a block that is part of the iterated dominance frontier of the definitions, in which case we know we have a live phi definition that we can insert.
Inserting phi definitions needs to recursively do the same thing for the phi arguments. To make this faster we could memoize the reaching SSA numbers for each block, though this is not currently done.
This also requires computing the DFS tree, dominators, and dominance frontiers after RBO (which invalidates those). Since the DFS tree and dominator tree is usually computed anyway by IV opts, this is not that costly.

The cost is further reduced by avoiding any of these computations in the single-def case (which CSE already handled). This special case is also moved into the SSA updater.
Finally, since IV opts need liveness to work, the SSA inserter also computes liveness for the inserted locals. This is done by marking all paths from each use back to its reaching definition as having that local live-in.
Fix#109412
@jakobbotsch
JIT: Put all CSEs into SSA
ddf2b02
This adds an SSA updater that can incrementally put locals that were
added to the IR into SSA form and starts using this infrastructure from
CSE. It updates CSE to use this SSA updater to put all CSEs into SSA.
Previously only single-def CSEs were put into SSA, which can cause e.g.
IV opts to miss out on them.
The SSA updater requires all uses and definitions to be supplied. Based
on the definitions it is then possible to compute the candidate blocks
for phi nodes in the usual way as the iterated dominance frontier. Since
we do not have liveness for the local we do not insert the phi
definitions eagerly; instead, we recursively compute the reaching def
for each use by walking its dominators.
Walking the dominators we either expect to find a real definition, or to
hit a block that is part of the iterated dominance frontier of the
definitions, in which case we know we have a live phi definition that we
can insert.
Inserting phi definitions needs to recursively do the same thing for the
phi arguments. To make this faster we could memoize the reaching SSA
numbers for each block, though this is not currently done.
This also requires computing the DFS tree, dominators, and dominance
frontiers after RBO (which invalidates those). The DFS tree and
dominators should be reusable by IV opts in most cases, though currently
we invalidate them after CSE because assertion prop sometimes will
change the flow graph and does not know how to invalidate the DFS tree.
This can also be fixed to improve throughput.
We currently do not get much use out of this because IV opts also need
liveness for the IVs. More specifically it needs to know whether the IVs
are live out of the loop; it should be possible to keep some breadcrumbs
around for inserted locals to cheaply know this, I think.
@ghostghost
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
Aug 19, 2024
@jakobbotsch

This comment was marked as outdated.

Sign in to view

This was referenced Aug 19, 2024

Closed


Closed

@jakobbotsch
jakobbotsch
commented
Aug 20, 2024

Copy link


Copy Markdown

MemberAuthor
TP diffs for benchmarks.run_pgo now look like:
Base:120833007777, Diff:120935018667, +0.0844%317810259:NA:25.72%:+0.2630%:public: void __cdecl FlowGraphDominanceFrontiers::ComputeIteratedDominanceFrontier(struct BasicBlock*, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>*) 162099515:NA:13.12%:+0.1342%:public: static classFlowGraphDominanceFrontiers* __cdecl FlowGraphDominanceFrontiers::Build(classFlowGraphDominatorTree*) 39047720:NA:3.16%:+0.0323%:private: static void __cdecl SsaBuilder::AddNewPhiArg(classCompiler*, struct BasicBlock*, struct Statement*, struct GenTreePhi*, unsigned int, unsigned int, struct BasicBlock*) 38346141:NA:3.10%:+0.0317%:public: bool __cdecl Compiler::fgMorphBlockStmt(struct BasicBlock*, struct Statement*, bool) 18905583:+52.64%:1.53%:+0.0156%:public: virtual void __cdecl CSE_HeuristicCommon::PerformCSE(classCSE_Candidate*) 13835238:NA:1.12%:+0.0114%:private: static struct Statement* __cdecl SsaBuilder::InsertPhi(classCompiler*, struct BasicBlock*, unsigned int) 8753827:+0.85%:0.71%:+0.0072%:public: __cdecl `public: unsigned int __cdecl Compiler::fgRunDfs<class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_1>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_2>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_3>, 0>(class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_1>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_2>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_3>)'::`2'::<lambda_1>::operator()(struct BasicBlock*) const
7221209:+0.28%:0.58%:+0.0060%:public: void * __cdecl ArenaAllocator::allocateMemory(unsigned __int64) 7119522:+2.54%:0.58%:+0.0059%:public: static classFlowGraphDominatorTree* __cdecl FlowGraphDominatorTree::Build(classFlowGraphDfsTree const *) 6499831:+7.25%:0.53%:+0.0054%:private: bool __cdecl jitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>::ensure_capacity(unsigned __int64) 4424130:+12.56%:0.36%:+0.0037%:private: void __cdecl JitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, struct FlowEdge*, classCompAllocator, classJitHashTableBehavior>::Grow(void) 3705888:NA:0.30%:+0.0031%:public: static void __cdecl SsaBuilder::InsertInSsa(classCompiler*, unsigned int, classArrayStack<struct UseDefLocation> &, classArrayStack<struct UseDefLocation> &) 3361499:NA:0.27%:+0.0028%:private: struct UseDefLocation __cdecl IncrementalSsaBuilder::FindOrCreateReachingDef(struct UseDefLocation const &) 2892505:+0.42%:0.23%:+0.0024%:public: unsigned short __cdecl Compiler::optAddAssertion(struct Compiler::AssertionDsc*) 2651134:+0.69%:0.21%:+0.0022%:BasicBlock::VisitAllSuccs<`AllSuccessorEnumerator::AllSuccessorEnumerator'::`2'::<lambda_1>>2535856:+4.24%:0.21%:+0.0021%:public: struct FlowEdge* __cdecl Compiler::BlockPredsWithEH(struct BasicBlock*) 1743270:NA:0.14%:+0.0014%:public: void __cdecl IncrementalSsaBuilder::Insert(void) 1693606:+0.58%:0.14%:+0.0014%:public: void __cdecl RangeCheck::MergeEdgeAssertions(unsigned int, unsigned __int64 *const &, struct Range*) 1583199:NA:0.13%:+0.0013%:private: bool __cdecl IncrementalSsaBuilder::FindReachingDefInBlock(struct UseDefLocation const &, struct BasicBlock*, struct UseDefLocation*) 1557652:+1.89%:0.13%:+0.0013%:public: void __cdecl Compiler::fgValueNumberPhiDef(struct GenTreeLclVar*, struct BasicBlock*, bool) 1349037:+0.75%:0.11%:+0.0011%:VisitEHSuccs<0,`AllSuccessorEnumerator::AllSuccessorEnumerator'::`2'::<lambda_1> >1261895:+0.82%:0.10%:+0.0010%: `Compiler::optReachable'::`12'::<lambda_1>::operator() -5032126:-0.49%:0.41%:-0.0042%: memset -26356661:-50.03%:2.13%:-0.0218%:private: void __cdecl SsaBuilder::AddPhiArg(struct BasicBlock*, struct Statement*, struct GenTreePhi*, unsigned int, unsigned int, struct BasicBlock*) -30940686:-15.02%:2.50%:-0.0256%:private: void __cdecl SsaBuilder::InsertPhiFunctions(void) -36618036:-100.00%:2.96%:-0.0303%:public: bool __cdecl Compiler::fgMorphBlockStmt(struct BasicBlock*, struct Statement*) -139925973:-100.00%:11.32%:-0.1158%:private: void __cdecl SsaBuilder::ComputeDominanceFrontiers(struct BasicBlock**, int, classJitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>, classCompAllocator, classJitHashTableBehavior>*) -322436319:-100.00%:26.09%:-0.2668%:private: void __cdecl SsaBuilder::ComputeIteratedDominanceFrontier(struct BasicBlock*, classJitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>, classCompAllocator, classJitHashTableBehavior> const *, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>*) 
So looks small enough that we can take this change without much more consideration.
One thing is whether we need to memoize reaching defs in some way to avoid quadratic (or even potentially exponential) behavior. Need to think about that a bit.
@jakobbotschjakobbotsch
added this to the 10.0.0 milestone
Aug 20, 2024
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 20, 2024

Closed

@jakobbotsch
jakobbotsch
commented
Aug 21, 2024

Copy link


Copy Markdown

MemberAuthor
benchmarks.run_pgo seems to be one of the least affected collections. The most affected one is benchmarks.run. With some manual reconciliation of renamed functions in the diff, the detailed TP diff for it looks like:
Base:40469192366, Diff:40551805661, +0.2041%16221652:+33.98%:13.58%:+0.0401%:private: void __cdecl SsaBuilder::ComputeDominanceFrontiers(struct BasicBlock**, int, classJitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>, classCompAllocator, classJitHashTableBehavior>*) 14025772:+48.54%:11.74%:+0.0347%:public: virtual void __cdecl CSE_HeuristicCommon::PerformCSE(classCSE_Candidate*) 11802368:NA:9.88%:+0.0292%:private: static void __cdecl SsaBuilder::AddNewPhiArg(classCompiler*, struct BasicBlock*, struct Statement*, struct GenTreePhi*, unsigned int, unsigned int, struct BasicBlock*) 6227765:+2.88%:5.21%:+0.0154%:public: __cdecl `public: unsigned int __cdecl Compiler::fgRunDfs<class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_1>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_2>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_3>, 0>(class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_1>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_2>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_3>)'::`2'::<lambda_1>::operator()(struct BasicBlock*) const
4914583:+0.60%:4.11%:+0.0121%:public: void * __cdecl ArenaAllocator::allocateMemory(unsigned __int64) 4651869:+6.52%:3.89%:+0.0115%:public: static classFlowGraphDominatorTree* __cdecl FlowGraphDominatorTree::Build(classFlowGraphDfsTree const *) 4369172:+17.68%:3.66%:+0.0108%:private: bool __cdecl jitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>::ensure_capacity(unsigned __int64) 3469086:NA:2.90%:+0.0086%:private: static struct Statement* __cdecl SsaBuilder::InsertPhi(classCompiler*, struct BasicBlock*, unsigned int) 2793726:NA:2.34%:+0.0069%:public: static void __cdecl SsaBuilder::InsertInSsa(classCompiler*, unsigned int, classArrayStack<struct UseDefLocation> &, classArrayStack<struct UseDefLocation> &) 2747882:NA:2.30%:+0.0068%:private: struct UseDefLocation __cdecl IncrementalSsaBuilder::FindOrCreateReachingDef(struct UseDefLocation const &) 2745600:+25.93%:2.30%:+0.0068%:private: void __cdecl JitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, struct FlowEdge*, classCompAllocator, classJitHashTableBehavior>::Grow(void) 2538778:+2.19%:2.13%:+0.0063%:public: void __cdecl RangeCheck::MergeEdgeAssertions(unsigned int, unsigned __int64 *const &, struct Range*) 2129497:+12.45%:1.78%:+0.0053%:public: struct FlowEdge* __cdecl Compiler::BlockPredsWithEH(struct BasicBlock*) 1869746:+2.27%:1.57%:+0.0046%:BasicBlock::VisitAllSuccs<`AllSuccessorEnumerator::AllSuccessorEnumerator'::`2'::<lambda_1>>1822996:NA:1.53%:+0.0045%:private: bool __cdecl IncrementalSsaBuilder::FindReachingDefInBlock(struct UseDefLocation const &, struct BasicBlock*, struct UseDefLocation*) 1790224:NA:1.50%:+0.0044%:public: void __cdecl IncrementalSsaBuilder::Insert(void) 1522909:+0.56%:1.28%:+0.0038%:public: unsigned short __cdecl Compiler::optAddAssertion(struct Compiler::AssertionDsc*) 1018306:+2.48%:0.85%:+0.0025%:VisitEHSuccs<0,`AllSuccessorEnumerator::AllSuccessorEnumerator'::`2'::<lambda_1> >891863:+2.70%:0.75%:+0.0022%:public: struct Range __cdecl RangeCheck::ComputeRange(struct BasicBlock*, struct GenTree*, bool) 790041:+4.09%:0.66%:+0.0020%:public: bool __cdecl Compiler::fgMorphBlockStmt(struct BasicBlock*, struct Statement*) 717239:+6.03%:0.60%:+0.0018%:public: struct FlowEdge* __cdecl Compiler::BlockDominancePreds(struct BasicBlock*) 595655:+0.18%:0.50%:+0.0015%: memset 573612:+2.48%:0.48%:+0.0014%:public: void __cdecl Compiler::fgValueNumberPhiDef(struct GenTreeLclVar*, struct BasicBlock*, bool) 551275:+31.48%:0.46%:+0.0014%:public: struct EHblkDsc* __cdecl Compiler::ehGetBlockExnFlowDsc(struct BasicBlock*) 383941:+0.44%:0.32%:+0.0009%:public: void __cdecl Compiler::compInit(classArenaAllocator*, struct CORINFO_METHOD_STRUCT_*, classICorJitInfo*, struct CORINFO_METHOD_INFO*, struct InlineInfo*) 374654:+2.29%:0.31%:+0.0009%:public: struct Scev* __cdecl ScalarEvolutionContext::Simplify(struct Scev*, struct SimplificationAssumptions const &) 367471:+0.29%:0.31%:+0.0009%:private: void __cdecl SsaBuilder::BlockRenameVariables(struct BasicBlock*) 312764:+2.04%:0.26%:+0.0008%:public: bool __cdecl JitHashTable<struct GenTree *, struct JitPtrKeyFuncs<struct GenTree>, struct Scev*, classCompAllocator, classJitHashTableBehavior>::Set(struct GenTree*, struct Scev*, enumJitHashTable<struct GenTree *, struct JitPtrKeyFuncs<struct GenTree>, struct Scev*, classCompAllocator, classJitHashTableBehavior>::SetKind) 304582:+0.07%:0.26%:+0.0008%:public: struct GenTree* __cdecl Compiler::optAssertionProp_LclVar(unsigned __int64 *const &, struct GenTreeLclVarCommon*, struct Statement*) 297432:+4.41%:0.25%:+0.0007%:public: enumRelopEvaluationResult __cdecl ScalarEvolutionContext::EvaluateRelop(unsigned int) 281935:+0.55%:0.24%:+0.0007%:private: void __cdecl JitHashTable<struct CORINFO_FIELD_STRUCT_*, struct JitPtrKeyFuncs<struct CORINFO_FIELD_STRUCT_>, classFieldSeq, classCompAllocator, classJitHashTableBehavior>::Grow(void) 254204:+2.33%:0.21%:+0.0006%:public: static void __cdecl BitSetOps<unsigned __int64 *, 1, struct BitVecTraits *, struct BitVecTraits>::ClearD(struct BitVecTraits*, unsigned __int64 *&) 221830:+1.38%:0.19%:+0.0005%:private: void __cdecl SsaDefArray<class LclSsaVarDsc>::GrowArray(classCompAllocator) 220372:+0.14%:0.18%:+0.0005%:public: enumPhaseStatus __cdecl Compiler::optAssertionPropMain(void) 197369:+3.85%:0.17%:+0.0005%:public: bool __cdecl RangeCheck::ComputeDoesOverflow(struct BasicBlock*, struct GenTree*, struct Range const &) 194313:+0.26%:0.16%:+0.0005%:public: void __cdecl Compiler::optImpliedAssertions(unsigned short, unsigned __int64 *&) 186496:+2.80%:0.16%:+0.0005%:public: struct Range __cdecl RangeCheck::GetRange(struct BasicBlock*, struct GenTree*, bool) 178680:+0.22%:0.15%:+0.0004%:private: void __cdecl SsaBuilder::ComputeIteratedDominanceFrontier(struct BasicBlock*, classJitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>, classCompAllocator, classJitHashTableBehavior> const *, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>*) 175067:+0.24%:0.15%:+0.0004%:public: void __cdecl DataFlow::ForwardAnalysis<class AssertionPropFlowCallback>(classAssertionPropFlowCallback&) 174825:+0.88%:0.15%:+0.0004%:public: void __cdecl ValueNumStore::GetConstantBoundInfo(unsigned int, struct ValueNumStore::ConstantBoundInfo*) 172806:+0.31%:0.14%:+0.0004%:protected: void __cdecl Compiler::compCompile(void **, unsigned int *, classJitFlags*) 172736:+2.73%:0.14%:+0.0004%:public: void __cdecl RangeCheck::MergeEdgeAssertions(struct GenTreeLclVarCommon*, unsigned __int64 *const &, struct Range*) 164132:+3.16%:0.14%:+0.0004%:public: static struct Range __cdecl RangeOps::Merge(struct Range const &, struct Range const &, bool) 162139:+3.63%:0.14%:+0.0004%:public: struct Range __cdecl RangeCheck::ComputeRangeForBinOp(struct BasicBlock*, struct GenTreeOp*, bool) 155473:+0.24%:0.13%:+0.0004%:public: void __cdecl Compiler::optImpliedByTypeOfAssertions(unsigned __int64 *&) 150968:+0.33%:0.13%:+0.0004%:BasicBlock::VisitAllSuccs<`Compiler::optReachable'::`12'::<lambda_1>>147163:+0.33%:0.12%:+0.0004%:public: unsigned short __cdecl Compiler::optGlobalAssertionIsEqualOrNotEqualZero(unsigned __int64 *const &, struct GenTree*) 146382:+0.76%:0.12%:+0.0004%:public: enumPhaseStatus __cdecl Compiler::optVNBasedDeadStoreRemoval(void) 139613:+0.29%:0.12%:+0.0003%: `Compiler::optReachable'::`12'::<lambda_1>::operator() 138149:+0.06%:0.12%:+0.0003%:public: unsigned int __cdecl ValueNumStore::VNForFunc(enumvar_types, enumVNFunc, unsigned int, unsigned int) 136733:+0.65%:0.11%:+0.0003%: `LoopLocalOccurrences::GetOrCreateMap'::`2'::<lambda_1>::operator() 130909:+0.04%:0.11%:+0.0003%:public: unsigned int * __cdecl JitHashTable<struct ValueNumStore::VNDefFuncApp<2>, struct ValueNumStore::VNDefFuncAppKeyFuncs<2>, unsigned int, classCompAllocator, classJitHashTableBehavior>::LookupPointerOrAdd(struct ValueNumStore::VNDefFuncApp<2>, unsigned int) 124058:+0.16%:0.10%:+0.0003%:public: static void __cdecl BitSetOps<unsigned __int64 *, 1, struct BitVecTraits *, struct BitVecTraits>::IntersectionD(struct BitVecTraits*, unsigned __int64 *&, unsigned __int64 *) -374413:-3.47%:0.31%:-0.0009%:public: enumPhaseStatus __cdecl Compiler::fgSsaBuild(void) -7939897:-26.44%:6.65%:-0.0196%:private: void __cdecl SsaBuilder::AddPhiArg(struct BasicBlock*, struct Statement*, struct GenTreePhi*, unsigned int, unsigned int, struct BasicBlock*) -9406160:-14.99%:7.88%:-0.0232%:private: void __cdecl SsaBuilder::InsertPhiFunctions(void) 
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 23, 2024

Closed

@dotnet-policy-servicedotnet-policy-serviceBot
removed this from the 10.0.0 milestone
Sep 22, 2024
@jakobbotschjakobbotsch
reopened this
Oct 7, 2024
@jakobbotschjakobbotsch
mentioned this pull request
Oct 31, 2024

Closed

@jakobbotsch
jakobbotsch
commented
Oct 31, 2024

Copy link


Copy Markdown

MemberAuthor
This seems to also break strength reduction sometimes, need to investigate that.
image
@jakobbotschjakobbotsch
mentioned this pull request
Nov 4, 2024
 Merged

@jakobbotschjakobbotsch
mentioned this pull request
Nov 6, 2024
 Closed

@jakobbotsch
jakobbotsch
commented
Nov 6, 2024

Copy link


Copy Markdown

MemberAuthor
This seems to also break strength reduction sometimes, need to investigate that.
The problem here was the fact that I added the lvTracked check in optLocalHasNonLoopUses. I did that separately in #109505, to take those regressions separately (this was a correctness fix). To resolve those regressions I have then added the necessary liveness computation to this PR: that is, when inserting a local into SSA we now also compute the set of blocks in which that local is live-in, which is then stored in a side table. The cost of that computation does not look to be too bad -- overall TP cost of SSA insertion + liveness computation is around 0.2% for win-x64, with some collections being outliers in both directions. I think that cost is acceptable given that this gets us a fair amount of new strength reductions.
Still have some other regressions to investigate.
@jakobbotsch
jakobbotsch
commented
Nov 6, 2024

Copy link


Copy Markdown

MemberAuthor
cc @dotnet/jit-contrib PTAL @AndyAyersMS -- this now computes liveness for SSA-inserted locals, see my comment above. Can you review those new parts?
Diffs.
I went through a bunch of ASP.NET regressions and they fell into two categories:
  • Some regressions are because of new eliminated comparisons in assertion prop, which can lead to diferent register allocation due to fewer uses of those locals, and this can end up being a regression overall
  • Other regressions happen when IVs created by IV opts do not get a register allocated (nothing new here)
Sometimes strength reduction is also a size regression while being a perfscore regression, but there is nothing new there.
Some stats for number of strength reductions in win-x64 collections with this PR:

aspnet: 1710 -> 1819 (+6.4%)

benchmarks.run_pgo: 2177 -> 2252 (+3.4%)

libraries_tests.run: 10483 -> 11115 (+6.0%) with 74 new misses

realworld: 471 -> 523 (+11%)
@jakobbotsch
jakobbotsch
marked this pull request as ready for review
November 6, 2024 16:32
@jakobbotsch
jakobbotsch
commented
Nov 6, 2024

Copy link


Copy Markdown

MemberAuthor
This seems to be more expensive for arm64, likely because there are more CSEs that are now having their liveness computed.

One thing is that we only very rarely need that liveness, for IV opts only, so if we could only compute it at that point it would be better. But it would require us to keep the list of reaching defs/uses up to date between CSE and IV opts, while currently we only need to keep the liveness information up to date. Probably not impossible, but seems impractical given that assertion prop runs there. An alternative would be to find some way to predict whether we'll need the liveness, but I also didn't see a simple way of doing that from CSE.
AndyAyersMS
AndyAyersMS
approved these changes
Nov 6, 2024
View reviewed changes
Comment threadsrc/coreclr/jit/ssabuilder.cpp
 m_queue.Reset();
 m_queue.Push(use.Block);

 while (!m_queue.Empty())
@AndyAyersMSAndyAyersMSNov 6, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a check that you actually do find the def block, and/or don't try to search past the first block? (that is, def dominates use)
@jakobbotschjakobbotschNov 7, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we necessarily find the reaching def (we might hit a block marked live-in already due to another use), but makes sense to add an assert that we don't get to the first block. I added that assert into AddInsertedSsaLiveIn since it never makes sense to mark these added locals as live into the first BB.
@build-analysisbuild-analysisBot
mentioned this pull request
Nov 7, 2024

Open

3 tasks
@jakobbotsch
jakobbotsch
merged commit 9c1f53e
into
dotnet:mainNov 7, 2024
@jakobbotsch
jakobbotsch
deleted the

ssa-updating

branch
November 7, 2024 10:18

This was referenced Nov 8, 2024

Closed


Closed

@DrewScogginsDrewScoggins
mentioned this pull request
Nov 14, 2024

Closed

@AndyAyersMSAndyAyersMS
mentioned this pull request
Nov 21, 2024

Closed

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Dec 8, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@AndyAyersMSAndyAyersMSAndyAyersMS approved these changes

Assignees

@jakobbotschjakobbotsch

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

JIT: Cloned loop not fully strength reduced on arm64


2 participants

@jakobbotsch@AndyAyersMS

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
JIT: Put all CSEs into SSA - #106637
Merged
jakobbotsch merged 29 commits into
dotnet:mainfrom 
jakobbotsch:ssa-updating
Nov 7, 2024
Merged
JIT: Put all CSEs into SSA#106637
jakobbotsch merged 29 commits into
dotnet:mainfrom 
jakobbotsch:ssa-updating
Conversation
@jakobbotsch
@jakobbotschjakobbotsch
commented
Aug 19, 2024•

edited

Loading

Copy link


Copy Markdown

Member
This adds an SSA updater that can incrementally put locals that were added to the IR into SSA form and starts using this infrastructure from CSE. Previously only single-def CSEs were put into SSA, which can cause e.g. IV opts to miss out on them.
The SSA updater requires all uses and definitions to be supplied. Based on the definitions it is then possible to compute the candidate blocks for phi nodes in the usual way as the iterated dominance frontier. Since we do not have liveness for the local we do not insert the phi definitions eagerly; instead, we recursively compute the reaching def for each use by walking its dominators.

Walking the dominators we either expect to find a real definition, or to hit a block that is part of the iterated dominance frontier of the definitions, in which case we know we have a live phi definition that we can insert.
Inserting phi definitions needs to recursively do the same thing for the phi arguments. To make this faster we could memoize the reaching SSA numbers for each block, though this is not currently done.
This also requires computing the DFS tree, dominators, and dominance frontiers after RBO (which invalidates those). Since the DFS tree and dominator tree is usually computed anyway by IV opts, this is not that costly.

The cost is further reduced by avoiding any of these computations in the single-def case (which CSE already handled). This special case is also moved into the SSA updater.
Finally, since IV opts need liveness to work, the SSA inserter also computes liveness for the inserted locals. This is done by marking all paths from each use back to its reaching definition as having that local live-in.
Fix#109412
@jakobbotsch
JIT: Put all CSEs into SSA
ddf2b02
This adds an SSA updater that can incrementally put locals that were
added to the IR into SSA form and starts using this infrastructure from
CSE. It updates CSE to use this SSA updater to put all CSEs into SSA.
Previously only single-def CSEs were put into SSA, which can cause e.g.
IV opts to miss out on them.
The SSA updater requires all uses and definitions to be supplied. Based
on the definitions it is then possible to compute the candidate blocks
for phi nodes in the usual way as the iterated dominance frontier. Since
we do not have liveness for the local we do not insert the phi
definitions eagerly; instead, we recursively compute the reaching def
for each use by walking its dominators.
Walking the dominators we either expect to find a real definition, or to
hit a block that is part of the iterated dominance frontier of the
definitions, in which case we know we have a live phi definition that we
can insert.
Inserting phi definitions needs to recursively do the same thing for the
phi arguments. To make this faster we could memoize the reaching SSA
numbers for each block, though this is not currently done.
This also requires computing the DFS tree, dominators, and dominance
frontiers after RBO (which invalidates those). The DFS tree and
dominators should be reusable by IV opts in most cases, though currently
we invalidate them after CSE because assertion prop sometimes will
change the flow graph and does not know how to invalidate the DFS tree.
This can also be fixed to improve throughput.
We currently do not get much use out of this because IV opts also need
liveness for the IVs. More specifically it needs to know whether the IVs
are live out of the loop; it should be possible to keep some breadcrumbs
around for inserted locals to cheaply know this, I think.
@ghostghost
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
Aug 19, 2024
@jakobbotsch

This comment was marked as outdated.

Sign in to view

This was referenced Aug 19, 2024

Closed


Closed

@jakobbotsch
jakobbotsch
commented
Aug 20, 2024

Copy link


Copy Markdown

MemberAuthor
TP diffs for benchmarks.run_pgo now look like:
Base:120833007777, Diff:120935018667, +0.0844%317810259:NA:25.72%:+0.2630%:public: void __cdecl FlowGraphDominanceFrontiers::ComputeIteratedDominanceFrontier(struct BasicBlock*, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>*) 162099515:NA:13.12%:+0.1342%:public: static classFlowGraphDominanceFrontiers* __cdecl FlowGraphDominanceFrontiers::Build(classFlowGraphDominatorTree*) 39047720:NA:3.16%:+0.0323%:private: static void __cdecl SsaBuilder::AddNewPhiArg(classCompiler*, struct BasicBlock*, struct Statement*, struct GenTreePhi*, unsigned int, unsigned int, struct BasicBlock*) 38346141:NA:3.10%:+0.0317%:public: bool __cdecl Compiler::fgMorphBlockStmt(struct BasicBlock*, struct Statement*, bool) 18905583:+52.64%:1.53%:+0.0156%:public: virtual void __cdecl CSE_HeuristicCommon::PerformCSE(classCSE_Candidate*) 13835238:NA:1.12%:+0.0114%:private: static struct Statement* __cdecl SsaBuilder::InsertPhi(classCompiler*, struct BasicBlock*, unsigned int) 8753827:+0.85%:0.71%:+0.0072%:public: __cdecl `public: unsigned int __cdecl Compiler::fgRunDfs<class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_1>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_2>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_3>, 0>(class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_1>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_2>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_3>)'::`2'::<lambda_1>::operator()(struct BasicBlock*) const
7221209:+0.28%:0.58%:+0.0060%:public: void * __cdecl ArenaAllocator::allocateMemory(unsigned __int64) 7119522:+2.54%:0.58%:+0.0059%:public: static classFlowGraphDominatorTree* __cdecl FlowGraphDominatorTree::Build(classFlowGraphDfsTree const *) 6499831:+7.25%:0.53%:+0.0054%:private: bool __cdecl jitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>::ensure_capacity(unsigned __int64) 4424130:+12.56%:0.36%:+0.0037%:private: void __cdecl JitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, struct FlowEdge*, classCompAllocator, classJitHashTableBehavior>::Grow(void) 3705888:NA:0.30%:+0.0031%:public: static void __cdecl SsaBuilder::InsertInSsa(classCompiler*, unsigned int, classArrayStack<struct UseDefLocation> &, classArrayStack<struct UseDefLocation> &) 3361499:NA:0.27%:+0.0028%:private: struct UseDefLocation __cdecl IncrementalSsaBuilder::FindOrCreateReachingDef(struct UseDefLocation const &) 2892505:+0.42%:0.23%:+0.0024%:public: unsigned short __cdecl Compiler::optAddAssertion(struct Compiler::AssertionDsc*) 2651134:+0.69%:0.21%:+0.0022%:BasicBlock::VisitAllSuccs<`AllSuccessorEnumerator::AllSuccessorEnumerator'::`2'::<lambda_1>>2535856:+4.24%:0.21%:+0.0021%:public: struct FlowEdge* __cdecl Compiler::BlockPredsWithEH(struct BasicBlock*) 1743270:NA:0.14%:+0.0014%:public: void __cdecl IncrementalSsaBuilder::Insert(void) 1693606:+0.58%:0.14%:+0.0014%:public: void __cdecl RangeCheck::MergeEdgeAssertions(unsigned int, unsigned __int64 *const &, struct Range*) 1583199:NA:0.13%:+0.0013%:private: bool __cdecl IncrementalSsaBuilder::FindReachingDefInBlock(struct UseDefLocation const &, struct BasicBlock*, struct UseDefLocation*) 1557652:+1.89%:0.13%:+0.0013%:public: void __cdecl Compiler::fgValueNumberPhiDef(struct GenTreeLclVar*, struct BasicBlock*, bool) 1349037:+0.75%:0.11%:+0.0011%:VisitEHSuccs<0,`AllSuccessorEnumerator::AllSuccessorEnumerator'::`2'::<lambda_1> >1261895:+0.82%:0.10%:+0.0010%: `Compiler::optReachable'::`12'::<lambda_1>::operator() -5032126:-0.49%:0.41%:-0.0042%: memset -26356661:-50.03%:2.13%:-0.0218%:private: void __cdecl SsaBuilder::AddPhiArg(struct BasicBlock*, struct Statement*, struct GenTreePhi*, unsigned int, unsigned int, struct BasicBlock*) -30940686:-15.02%:2.50%:-0.0256%:private: void __cdecl SsaBuilder::InsertPhiFunctions(void) -36618036:-100.00%:2.96%:-0.0303%:public: bool __cdecl Compiler::fgMorphBlockStmt(struct BasicBlock*, struct Statement*) -139925973:-100.00%:11.32%:-0.1158%:private: void __cdecl SsaBuilder::ComputeDominanceFrontiers(struct BasicBlock**, int, classJitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>, classCompAllocator, classJitHashTableBehavior>*) -322436319:-100.00%:26.09%:-0.2668%:private: void __cdecl SsaBuilder::ComputeIteratedDominanceFrontier(struct BasicBlock*, classJitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>, classCompAllocator, classJitHashTableBehavior> const *, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>*) 
So looks small enough that we can take this change without much more consideration.
One thing is whether we need to memoize reaching defs in some way to avoid quadratic (or even potentially exponential) behavior. Need to think about that a bit.
@jakobbotschjakobbotsch
added this to the 10.0.0 milestone
Aug 20, 2024
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 20, 2024

Closed

@jakobbotsch
jakobbotsch
commented
Aug 21, 2024

Copy link


Copy Markdown

MemberAuthor
benchmarks.run_pgo seems to be one of the least affected collections. The most affected one is benchmarks.run. With some manual reconciliation of renamed functions in the diff, the detailed TP diff for it looks like:
Base:40469192366, Diff:40551805661, +0.2041%16221652:+33.98%:13.58%:+0.0401%:private: void __cdecl SsaBuilder::ComputeDominanceFrontiers(struct BasicBlock**, int, classJitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>, classCompAllocator, classJitHashTableBehavior>*) 14025772:+48.54%:11.74%:+0.0347%:public: virtual void __cdecl CSE_HeuristicCommon::PerformCSE(classCSE_Candidate*) 11802368:NA:9.88%:+0.0292%:private: static void __cdecl SsaBuilder::AddNewPhiArg(classCompiler*, struct BasicBlock*, struct Statement*, struct GenTreePhi*, unsigned int, unsigned int, struct BasicBlock*) 6227765:+2.88%:5.21%:+0.0154%:public: __cdecl `public: unsigned int __cdecl Compiler::fgRunDfs<class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_1>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_2>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_3>, 0>(class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_1>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_2>, class `public: classFlowGraphDfsTree* __cdecl Compiler::fgComputeDfs<0>(void)'::`2'::<lambda_3>)'::`2'::<lambda_1>::operator()(struct BasicBlock*) const
4914583:+0.60%:4.11%:+0.0121%:public: void * __cdecl ArenaAllocator::allocateMemory(unsigned __int64) 4651869:+6.52%:3.89%:+0.0115%:public: static classFlowGraphDominatorTree* __cdecl FlowGraphDominatorTree::Build(classFlowGraphDfsTree const *) 4369172:+17.68%:3.66%:+0.0108%:private: bool __cdecl jitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>::ensure_capacity(unsigned __int64) 3469086:NA:2.90%:+0.0086%:private: static struct Statement* __cdecl SsaBuilder::InsertPhi(classCompiler*, struct BasicBlock*, unsigned int) 2793726:NA:2.34%:+0.0069%:public: static void __cdecl SsaBuilder::InsertInSsa(classCompiler*, unsigned int, classArrayStack<struct UseDefLocation> &, classArrayStack<struct UseDefLocation> &) 2747882:NA:2.30%:+0.0068%:private: struct UseDefLocation __cdecl IncrementalSsaBuilder::FindOrCreateReachingDef(struct UseDefLocation const &) 2745600:+25.93%:2.30%:+0.0068%:private: void __cdecl JitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, struct FlowEdge*, classCompAllocator, classJitHashTableBehavior>::Grow(void) 2538778:+2.19%:2.13%:+0.0063%:public: void __cdecl RangeCheck::MergeEdgeAssertions(unsigned int, unsigned __int64 *const &, struct Range*) 2129497:+12.45%:1.78%:+0.0053%:public: struct FlowEdge* __cdecl Compiler::BlockPredsWithEH(struct BasicBlock*) 1869746:+2.27%:1.57%:+0.0046%:BasicBlock::VisitAllSuccs<`AllSuccessorEnumerator::AllSuccessorEnumerator'::`2'::<lambda_1>>1822996:NA:1.53%:+0.0045%:private: bool __cdecl IncrementalSsaBuilder::FindReachingDefInBlock(struct UseDefLocation const &, struct BasicBlock*, struct UseDefLocation*) 1790224:NA:1.50%:+0.0044%:public: void __cdecl IncrementalSsaBuilder::Insert(void) 1522909:+0.56%:1.28%:+0.0038%:public: unsigned short __cdecl Compiler::optAddAssertion(struct Compiler::AssertionDsc*) 1018306:+2.48%:0.85%:+0.0025%:VisitEHSuccs<0,`AllSuccessorEnumerator::AllSuccessorEnumerator'::`2'::<lambda_1> >891863:+2.70%:0.75%:+0.0022%:public: struct Range __cdecl RangeCheck::ComputeRange(struct BasicBlock*, struct GenTree*, bool) 790041:+4.09%:0.66%:+0.0020%:public: bool __cdecl Compiler::fgMorphBlockStmt(struct BasicBlock*, struct Statement*) 717239:+6.03%:0.60%:+0.0018%:public: struct FlowEdge* __cdecl Compiler::BlockDominancePreds(struct BasicBlock*) 595655:+0.18%:0.50%:+0.0015%: memset 573612:+2.48%:0.48%:+0.0014%:public: void __cdecl Compiler::fgValueNumberPhiDef(struct GenTreeLclVar*, struct BasicBlock*, bool) 551275:+31.48%:0.46%:+0.0014%:public: struct EHblkDsc* __cdecl Compiler::ehGetBlockExnFlowDsc(struct BasicBlock*) 383941:+0.44%:0.32%:+0.0009%:public: void __cdecl Compiler::compInit(classArenaAllocator*, struct CORINFO_METHOD_STRUCT_*, classICorJitInfo*, struct CORINFO_METHOD_INFO*, struct InlineInfo*) 374654:+2.29%:0.31%:+0.0009%:public: struct Scev* __cdecl ScalarEvolutionContext::Simplify(struct Scev*, struct SimplificationAssumptions const &) 367471:+0.29%:0.31%:+0.0009%:private: void __cdecl SsaBuilder::BlockRenameVariables(struct BasicBlock*) 312764:+2.04%:0.26%:+0.0008%:public: bool __cdecl JitHashTable<struct GenTree *, struct JitPtrKeyFuncs<struct GenTree>, struct Scev*, classCompAllocator, classJitHashTableBehavior>::Set(struct GenTree*, struct Scev*, enumJitHashTable<struct GenTree *, struct JitPtrKeyFuncs<struct GenTree>, struct Scev*, classCompAllocator, classJitHashTableBehavior>::SetKind) 304582:+0.07%:0.26%:+0.0008%:public: struct GenTree* __cdecl Compiler::optAssertionProp_LclVar(unsigned __int64 *const &, struct GenTreeLclVarCommon*, struct Statement*) 297432:+4.41%:0.25%:+0.0007%:public: enumRelopEvaluationResult __cdecl ScalarEvolutionContext::EvaluateRelop(unsigned int) 281935:+0.55%:0.24%:+0.0007%:private: void __cdecl JitHashTable<struct CORINFO_FIELD_STRUCT_*, struct JitPtrKeyFuncs<struct CORINFO_FIELD_STRUCT_>, classFieldSeq, classCompAllocator, classJitHashTableBehavior>::Grow(void) 254204:+2.33%:0.21%:+0.0006%:public: static void __cdecl BitSetOps<unsigned __int64 *, 1, struct BitVecTraits *, struct BitVecTraits>::ClearD(struct BitVecTraits*, unsigned __int64 *&) 221830:+1.38%:0.19%:+0.0005%:private: void __cdecl SsaDefArray<class LclSsaVarDsc>::GrowArray(classCompAllocator) 220372:+0.14%:0.18%:+0.0005%:public: enumPhaseStatus __cdecl Compiler::optAssertionPropMain(void) 197369:+3.85%:0.17%:+0.0005%:public: bool __cdecl RangeCheck::ComputeDoesOverflow(struct BasicBlock*, struct GenTree*, struct Range const &) 194313:+0.26%:0.16%:+0.0005%:public: void __cdecl Compiler::optImpliedAssertions(unsigned short, unsigned __int64 *&) 186496:+2.80%:0.16%:+0.0005%:public: struct Range __cdecl RangeCheck::GetRange(struct BasicBlock*, struct GenTree*, bool) 178680:+0.22%:0.15%:+0.0004%:private: void __cdecl SsaBuilder::ComputeIteratedDominanceFrontier(struct BasicBlock*, classJitHashTable<struct BasicBlock *, struct JitPtrKeyFuncs<struct BasicBlock>, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>, classCompAllocator, classJitHashTableBehavior> const *, classjitstd::vector<struct BasicBlock *, class jitstd::allocator<struct BasicBlock *>>*) 175067:+0.24%:0.15%:+0.0004%:public: void __cdecl DataFlow::ForwardAnalysis<class AssertionPropFlowCallback>(classAssertionPropFlowCallback&) 174825:+0.88%:0.15%:+0.0004%:public: void __cdecl ValueNumStore::GetConstantBoundInfo(unsigned int, struct ValueNumStore::ConstantBoundInfo*) 172806:+0.31%:0.14%:+0.0004%:protected: void __cdecl Compiler::compCompile(void **, unsigned int *, classJitFlags*) 172736:+2.73%:0.14%:+0.0004%:public: void __cdecl RangeCheck::MergeEdgeAssertions(struct GenTreeLclVarCommon*, unsigned __int64 *const &, struct Range*) 164132:+3.16%:0.14%:+0.0004%:public: static struct Range __cdecl RangeOps::Merge(struct Range const &, struct Range const &, bool) 162139:+3.63%:0.14%:+0.0004%:public: struct Range __cdecl RangeCheck::ComputeRangeForBinOp(struct BasicBlock*, struct GenTreeOp*, bool) 155473:+0.24%:0.13%:+0.0004%:public: void __cdecl Compiler::optImpliedByTypeOfAssertions(unsigned __int64 *&) 150968:+0.33%:0.13%:+0.0004%:BasicBlock::VisitAllSuccs<`Compiler::optReachable'::`12'::<lambda_1>>147163:+0.33%:0.12%:+0.0004%:public: unsigned short __cdecl Compiler::optGlobalAssertionIsEqualOrNotEqualZero(unsigned __int64 *const &, struct GenTree*) 146382:+0.76%:0.12%:+0.0004%:public: enumPhaseStatus __cdecl Compiler::optVNBasedDeadStoreRemoval(void) 139613:+0.29%:0.12%:+0.0003%: `Compiler::optReachable'::`12'::<lambda_1>::operator() 138149:+0.06%:0.12%:+0.0003%:public: unsigned int __cdecl ValueNumStore::VNForFunc(enumvar_types, enumVNFunc, unsigned int, unsigned int) 136733:+0.65%:0.11%:+0.0003%: `LoopLocalOccurrences::GetOrCreateMap'::`2'::<lambda_1>::operator() 130909:+0.04%:0.11%:+0.0003%:public: unsigned int * __cdecl JitHashTable<struct ValueNumStore::VNDefFuncApp<2>, struct ValueNumStore::VNDefFuncAppKeyFuncs<2>, unsigned int, classCompAllocator, classJitHashTableBehavior>::LookupPointerOrAdd(struct ValueNumStore::VNDefFuncApp<2>, unsigned int) 124058:+0.16%:0.10%:+0.0003%:public: static void __cdecl BitSetOps<unsigned __int64 *, 1, struct BitVecTraits *, struct BitVecTraits>::IntersectionD(struct BitVecTraits*, unsigned __int64 *&, unsigned __int64 *) -374413:-3.47%:0.31%:-0.0009%:public: enumPhaseStatus __cdecl Compiler::fgSsaBuild(void) -7939897:-26.44%:6.65%:-0.0196%:private: void __cdecl SsaBuilder::AddPhiArg(struct BasicBlock*, struct Statement*, struct GenTreePhi*, unsigned int, unsigned int, struct BasicBlock*) -9406160:-14.99%:7.88%:-0.0232%:private: void __cdecl SsaBuilder::InsertPhiFunctions(void) 
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 23, 2024

Closed

@dotnet-policy-servicedotnet-policy-serviceBot
removed this from the 10.0.0 milestone
Sep 22, 2024
@jakobbotschjakobbotsch
reopened this
Oct 7, 2024
@jakobbotschjakobbotsch
mentioned this pull request
Oct 31, 2024

Closed

@jakobbotsch
jakobbotsch
commented
Oct 31, 2024

Copy link


Copy Markdown

MemberAuthor
This seems to also break strength reduction sometimes, need to investigate that.
image
@jakobbotschjakobbotsch
mentioned this pull request
Nov 4, 2024
 Merged

@jakobbotschjakobbotsch
mentioned this pull request
Nov 6, 2024
 Closed

@jakobbotsch
jakobbotsch
commented
Nov 6, 2024

Copy link


Copy Markdown

MemberAuthor
This seems to also break strength reduction sometimes, need to investigate that.
The problem here was the fact that I added the lvTracked check in optLocalHasNonLoopUses. I did that separately in #109505, to take those regressions separately (this was a correctness fix). To resolve those regressions I have then added the necessary liveness computation to this PR: that is, when inserting a local into SSA we now also compute the set of blocks in which that local is live-in, which is then stored in a side table. The cost of that computation does not look to be too bad -- overall TP cost of SSA insertion + liveness computation is around 0.2% for win-x64, with some collections being outliers in both directions. I think that cost is acceptable given that this gets us a fair amount of new strength reductions.
Still have some other regressions to investigate.
@jakobbotsch
jakobbotsch
commented
Nov 6, 2024

Copy link


Copy Markdown

MemberAuthor
cc @dotnet/jit-contrib PTAL @AndyAyersMS -- this now computes liveness for SSA-inserted locals, see my comment above. Can you review those new parts?
Diffs.
I went through a bunch of ASP.NET regressions and they fell into two categories:
  • Some regressions are because of new eliminated comparisons in assertion prop, which can lead to diferent register allocation due to fewer uses of those locals, and this can end up being a regression overall
  • Other regressions happen when IVs created by IV opts do not get a register allocated (nothing new here)
Sometimes strength reduction is also a size regression while being a perfscore regression, but there is nothing new there.
Some stats for number of strength reductions in win-x64 collections with this PR:

aspnet: 1710 -> 1819 (+6.4%)

benchmarks.run_pgo: 2177 -> 2252 (+3.4%)

libraries_tests.run: 10483 -> 11115 (+6.0%) with 74 new misses

realworld: 471 -> 523 (+11%)
@jakobbotsch
jakobbotsch
marked this pull request as ready for review
November 6, 2024 16:32
@jakobbotsch
jakobbotsch
commented
Nov 6, 2024

Copy link


Copy Markdown

MemberAuthor
This seems to be more expensive for arm64, likely because there are more CSEs that are now having their liveness computed.

One thing is that we only very rarely need that liveness, for IV opts only, so if we could only compute it at that point it would be better. But it would require us to keep the list of reaching defs/uses up to date between CSE and IV opts, while currently we only need to keep the liveness information up to date. Probably not impossible, but seems impractical given that assertion prop runs there. An alternative would be to find some way to predict whether we'll need the liveness, but I also didn't see a simple way of doing that from CSE.
AndyAyersMS
AndyAyersMS
approved these changes
Nov 6, 2024
View reviewed changes
Comment threadsrc/coreclr/jit/ssabuilder.cpp
 m_queue.Reset();
 m_queue.Push(use.Block);

 while (!m_queue.Empty())
@AndyAyersMSAndyAyersMSNov 6, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a check that you actually do find the def block, and/or don't try to search past the first block? (that is, def dominates use)
@jakobbotschjakobbotschNov 7, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we necessarily find the reaching def (we might hit a block marked live-in already due to another use), but makes sense to add an assert that we don't get to the first block. I added that assert into AddInsertedSsaLiveIn since it never makes sense to mark these added locals as live into the first BB.
@build-analysisbuild-analysisBot
mentioned this pull request
Nov 7, 2024

Open

3 tasks
@jakobbotsch
jakobbotsch
merged commit 9c1f53e
into
dotnet:mainNov 7, 2024
@jakobbotsch
jakobbotsch
deleted the

ssa-updating

branch
November 7, 2024 10:18

This was referenced Nov 8, 2024

Closed


Closed

@DrewScogginsDrewScoggins
mentioned this pull request
Nov 14, 2024

Closed

@AndyAyersMSAndyAyersMS
mentioned this pull request
Nov 21, 2024

Closed

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Dec 8, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@AndyAyersMSAndyAyersMSAndyAyersMS approved these changes

Assignees

@jakobbotschjakobbotsch

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

JIT: Cloned loop not fully strength reduced on arm64


2 participants

@jakobbotsch@AndyAyersMS