-
Notifications
You must be signed in to change notification settings - Fork 15.4k
[LoopStrengthReduce] Mitigation of issues introduced by compilation time optimization in SolveRecurse. #147588
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
da198d1
c08f54e
bba0339
cabb769
96b948c
86b0bf3
176750d
ff7cfeb
3ddbd3c
976d273
090c61b
f444c02
cddfe58
6f14049
f53d813
aa5429d
145b1d6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2229,6 +2229,7 @@ class LSRInstance { | |||||||||||||||||
| void NarrowSearchSpaceByDeletingCostlyFormulas(); | ||||||||||||||||||
| void NarrowSearchSpaceByPickingWinnerRegs(); | ||||||||||||||||||
| void NarrowSearchSpaceUsingHeuristics(); | ||||||||||||||||||
| bool SortLSRUses(); | ||||||||||||||||||
|
|
||||||||||||||||||
| void SolveRecurse(SmallVectorImpl<const Formula *> &Solution, | ||||||||||||||||||
| Cost &SolutionCost, | ||||||||||||||||||
|
|
@@ -5368,6 +5369,46 @@ void LSRInstance::NarrowSearchSpaceUsingHeuristics() { | |||||||||||||||||
| NarrowSearchSpaceByPickingWinnerRegs(); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| /// Sort LSRUses to address side effects of compile time optimization done in | ||||||||||||||||||
| /// SolveRecurse which filters out formulae not including required registers. | ||||||||||||||||||
| /// Such optimization makes the found best solution sensitive to the order | ||||||||||||||||||
| /// of LSRUses processing, hence it's important to ensure that that order | ||||||||||||||||||
| /// isn't random to avoid fluctuations and sub-optimal results. | ||||||||||||||||||
| /// | ||||||||||||||||||
| /// Also check that all LSRUses have formulae as otherwise the situation is | ||||||||||||||||||
| /// unsolvable. | ||||||||||||||||||
| bool LSRInstance::SortLSRUses() { | ||||||||||||||||||
| SmallVector<LSRUse *, 16> NewOrder; | ||||||||||||||||||
| for (auto &LU : Uses) { | ||||||||||||||||||
|
||||||||||||||||||
| if (!LU.Formulae.size()) { | ||||||||||||||||||
|
||||||||||||||||||
| if (!LU.Formulae.size()) { | |
| if (LU.Formulae.empty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, fixed by second commit
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| std::stable_sort( | |
| NewOrder.begin(), NewOrder.end(), [](const LSRUse *L, const LSRUse *R) { | |
| stable_sort( | |
| NewOrder, NewOrder, [](const LSRUse *L, const LSRUse *R) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, fixed by second commit - but one NewOrder in your suggestion is redundant
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| assert(LU.Formulae.size() && | |
| assert(!LU.Formulae.empty() && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, fixed by second commit
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| LLVM_DEBUG(dbgs() << "New best at "; NewCost.print(dbgs()); | |
| dbgs() << ".\nRegs:\n"; | |
| for (const SCEV *S : NewRegs) dbgs() << "- " << *S << "\n"; | |
| dbgs() << '\n'); | |
| LLVM_DEBUG({dbgs() << "New best at "; NewCost.print(dbgs()); | |
| dbgs() << ".\nRegs:\n"; | |
| for (const SCEV *S : NewRegs) dbgs() << "- " << *S << '\n'; | |
| dbgs() << '\n'}); |
Extra {} will clang-format this better (though I suppose this is just getting re-indented here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need this temporary vector? Can't you sort Uses in place?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LSRUse is quite heavy (sizeof(LSRUse)=2184) while intermediate movement of objects can be done during sorting. Then it's faster to sort the array of pointers and establish the new order in the original array afterwards