-
Notifications
You must be signed in to change notification settings - Fork 10.7k
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
[MachineOutliner] Efficient Implementation of MachineOutliner::findCandidates() #90260
Conversation
@llvm/pr-subscribers-backend-aarch64 Author: Xuan Zhang (xuanzh-meta) ChangesThis reduce the time complexity of the main loop of For small For one application, this reduces the runtime of the main loop from 120 seconds to 28 seconds. This is the first commit for an enhanced version of machine outliner -- see RFC. Full diff: https://github.com/llvm/llvm-project/pull/90260.diff 2 Files Affected:
|
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.
Overall, it looks good to me, but let others review it further.
Hi @ornata, I want to follow up on the review for this PR. Appreciate it if you could take a look when you get a chance! |
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.
ltgm. however, I'd like to hear other opinions.
@ornata Do you have concerns or comments on this direction?
8522f88
to
632f482
Compare
llvm/lib/CodeGen/MachineOutliner.cpp
Outdated
@@ -593,6 +593,9 @@ void MachineOutliner::findCandidates( | |||
unsigned NumDiscarded = 0; | |||
unsigned NumKept = 0; | |||
#endif | |||
// Sort the start indices so that we can efficiently check if candidates | |||
// overlap with each other in MachineOutliner::findCandidates(). |
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.
I'm a bit confused why mention this function's name here (MachineOutliner::findCandidates()
). From the wording it would appear that it would referring to another function. Perhaps "overlap with each other further down" will be clearer.
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.
Thanks for the comments. Modified!
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.
LGTM
This reduce the time complexity of the main loop of$O(n^2)$ to $O(n \log n)$ .
findCandidates()
method fromFor small$n$ , the modification does not regress the build time, but it helps significantly when $n$ is large.
For one application, this reduces the runtime of the main loop from 120 seconds to 28 seconds.
This is the first commit for an enhanced version of machine outliner -- see RFC.