-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[GlobalMerge][NFC] Skip sorting by profitability when it is not needed #124146
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
Conversation
We were previously sorting by profitability even if we were choosing to merge all globals together, which is not impacted by UsedGlobalSet order. We can also remove iteration of UsedGlobalSets in reverse order in both cases. In the first csae, the order does not matter. In the second case, we just sort by the order we need instead of sorting in the opposite direction and calling reverse. This change should only be an improvement on compile time. I have not measured it, but I think it would never make things worse.
3676606
to
0bbe4ba
Compare
✅ With the latest revision this PR passed the C/C++ code formatter. |
8118157
to
f71bd49
Compare
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.
Make sense, LGTM!
This was not NFC: it causes the comparator not have strict-weak-ordering, because I believe you meant to use |
The assertion failure didn’t show up in pre-commit CI and I haven’t gotten any build bot failures. So thanks for pointing it out. Can you please revert or make the change to > for me as I am currently AFK for the night? Thanks. |
It's still not NFC with |
…ot needed" (#124411) Reverts #124146 -- new comparator is not a strict-weak as required by stable_sort. Co-authored-by: Michael Maitland <[email protected]>
…hen it is not needed" (#124411) Reverts llvm/llvm-project#124146 -- new comparator is not a strict-weak as required by stable_sort. Co-authored-by: Michael Maitland <[email protected]>
…ot needed" Relands #124146 but without changes to the sorting algorithm and the following reverse.
We were previously sorting by profitability even if we were choosing to merge all globals together, which is not impacted by UsedGlobalSet order.
We can also remove iteration of UsedGlobalSets in reverse order in both cases. In the first csae, the order does not matter. In the second case, we just sort by the order we need instead of sorting in the opposite direction and calling reverse.
This change should only be an improvement on compile time. I have not measured it, but I think it would never make things worse.