[BOLT] Extract comparator for sorting functions by index into helper function #116217
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…function This change extracts the comparator for sorting functions by index into a helper function `compareBinaryFunctionByIndex()` Not sure why the comparator used in `BinaryContext::getSortedFunctions()` is not same as the other two places. I think they should use the same comparator, so I also change `BinaryContext::getSortedFunctions()` to use `compareBinaryFunctionByIndex()` for sorting functions.
Enna1
force-pushed
the
users/Enna1/bolt-extract-function-compare-by-index
branch
from
c2c7b25 to
714df37
Compare
Enna1
requested review from
aaupov,
ayermolo,
dcci,
maksfb and
rafaelauler
as code owners
Member
|
@llvm/pr-subscribers-bolt Author: Enna1 (Enna1) ChangesThis change extracts the comparator for sorting functions by index into a helper function Not sure why the comparator used in Full diff: https://github.com/llvm/llvm-project/pull/116217.diff 4 Files Affected:
diff --git a/bolt/include/bolt/Core/BinaryFunction.h b/bolt/include/bolt/Core/BinaryFunction.h
index 0b3682353f736e..4da08c5d662349 100644
--- a/bolt/include/bolt/Core/BinaryFunction.h
+++ b/bolt/include/bolt/Core/BinaryFunction.h
@@ -2366,6 +2366,19 @@ inline raw_ostream &operator<<(raw_ostream &OS,
return OS;
}
+/// Compare function by index if it is valid, fall back to the original address
+/// otherwise.
+inline bool compareBinaryFunctionByIndex(const BinaryFunction *A,
+ const BinaryFunction *B) {
+ if (A->hasValidIndex() && B->hasValidIndex())
+ return A->getIndex() < B->getIndex();
+ if (A->hasValidIndex() && !B->hasValidIndex())
+ return true;
+ if (!A->hasValidIndex() && B->hasValidIndex())
+ return false;
+ return A->getAddress() < B->getAddress();
+}
+
} // namespace bolt
// GraphTraits specializations for function basic block graphs (CFGs)
diff --git a/bolt/lib/Core/BinaryContext.cpp b/bolt/lib/Core/BinaryContext.cpp
index f246750209d6c4..a808ece12da19c 100644
--- a/bolt/lib/Core/BinaryContext.cpp
+++ b/bolt/lib/Core/BinaryContext.cpp
@@ -1607,13 +1607,7 @@ std::vector<BinaryFunction *> BinaryContext::getSortedFunctions() {
SortedFunctions.begin(),
[](BinaryFunction &BF) { return &BF; });
- llvm::stable_sort(SortedFunctions,
- [](const BinaryFunction *A, const BinaryFunction *B) {
- if (A->hasValidIndex() && B->hasValidIndex()) {
- return A->getIndex() < B->getIndex();
- }
- return A->hasValidIndex();
- });
+ llvm::stable_sort(SortedFunctions, compareBinaryFunctionByIndex);
return SortedFunctions;
}
diff --git a/bolt/lib/Core/BinaryFunction.cpp b/bolt/lib/Core/BinaryFunction.cpp
index 5da777411ba7a1..a9ccaea3c43850 100644
--- a/bolt/lib/Core/BinaryFunction.cpp
+++ b/bolt/lib/Core/BinaryFunction.cpp
@@ -385,16 +385,7 @@ bool BinaryFunction::isForwardCall(const MCSymbol *CalleeSymbol) const {
if (CalleeBF) {
if (CalleeBF->isInjected())
return true;
-
- if (hasValidIndex() && CalleeBF->hasValidIndex()) {
- return getIndex() < CalleeBF->getIndex();
- } else if (hasValidIndex() && !CalleeBF->hasValidIndex()) {
- return true;
- } else if (!hasValidIndex() && CalleeBF->hasValidIndex()) {
- return false;
- } else {
- return getAddress() < CalleeBF->getAddress();
- }
+ return compareBinaryFunctionByIndex(this, CalleeBF);
} else {
// Absolute symbol.
ErrorOr<uint64_t> CalleeAddressOrError = BC.getSymbolValue(*CalleeSymbol);
diff --git a/bolt/lib/Passes/ReorderFunctions.cpp b/bolt/lib/Passes/ReorderFunctions.cpp
index c2d540135bdaa1..1256d71342b13b 100644
--- a/bolt/lib/Passes/ReorderFunctions.cpp
+++ b/bolt/lib/Passes/ReorderFunctions.cpp
@@ -473,16 +473,7 @@ Error ReorderFunctions::runOnFunctions(BinaryContext &BC) {
[](BinaryFunction &BF) { return &BF; });
// Sort functions by index.
- llvm::stable_sort(SortedFunctions,
- [](const BinaryFunction *A, const BinaryFunction *B) {
- if (A->hasValidIndex() && B->hasValidIndex())
- return A->getIndex() < B->getIndex();
- if (A->hasValidIndex() && !B->hasValidIndex())
- return true;
- if (!A->hasValidIndex() && B->hasValidIndex())
- return false;
- return A->getAddress() < B->getAddress();
- });
+ llvm::stable_sort(SortedFunctions, compareBinaryFunctionByIndex);
for (const BinaryFunction *Func : SortedFunctions) {
if (!Func->hasValidIndex())
|
Contributor
Author
|
ping:) |
peterwaller-arm
approved these changes
Nov 26, 2024
Contributor
peterwaller-arm
left a comment
peterwaller-arm
left a comment
There was a problem hiding this comment.
Looks like a reasonable change to me.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change extracts the comparator for sorting functions by index into a helper function
compareBinaryFunctionByIndex()Not sure why the comparator used in
BinaryContext::getSortedFunctions()is not same as the other two places. I think they should use the same comparator, so I also changeBinaryContext::getSortedFunctions()to usecompareBinaryFunctionByIndex()for sorting functions.