-
Notifications
You must be signed in to change notification settings - Fork 1.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
[backend] Use one linker to link all external libraries #3501
Conversation
This avoids creating a separate linker each time to link a device library to avoid the overhead of internal tracking.
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 little surprised this is significantly faster, but sgtm!
python/src/llvm.cc
Outdated
llvm::SMDiagnostic err; | ||
std::unique_ptr<llvm::Module> libMod = llvm::parseIRFile(path, err, ctx); | ||
if (!libMod) { | ||
llvm::errs() << "Failed to load " << path; |
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.
You're not changing it in this PR, but I wonder if you want to raise a Python exception in this case and the other error case below?
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.
Yup! I'm a bit scared about exceptions, but hey this is Python land. :D
Yeah I won't expect huge speedup either. I was actually more wanting to see if this addresses some suspected linking issues; turns out not. Either way seems good to have; won't be harmful. |
This avoids creating a separate linker each time to link a device library to avoid the overhead of internal tracking.
This avoids creating a separate linker each time to link a device library to avoid the overhead of internal tracking.