[Driver][Frontend] Introduce load-pass-plugin option
#68985
Conversation
antoniofrighetto
requested review from
artemcm,
tshortli,
hborla,
slavapestov and
xedin
as code owners
|
Please, let me know if anything needs to be improved as far as style conventions et alia is concerned, I'd be happy to do so. |
|
(cc/ @DougGregor) |
antoniofrighetto
force-pushed
the
feature/load-pass-plugin
branch
from
09d9fde
to
2293235
Compare
|
Gentle ping. Any feedback or suggestions would be duly appreciated. Alongside this, I believe the option should be added as part of Options.swift as well. May that be the case? (cc/ @DougGregor, @rintaro). |
|
Hey, this may not be getting much attention because the internal driver (this one) is considered legacy. The default driver as of Xcode 13 swift-driver. You might have better luck over there. Note: I'm not affiliated with the Swift project in any way. Just a guy who'd love to see this merged |
|
There was some discussion on this previously in: The driver and frontend pieces are separate and yes, proper driver support would mean adding the logic in https://github.com/apple/swift-driver. Legacy (C++-based) driver support being optional/discouraged. @lhames @aschwaighofer can comment better on the architecture of loading such passes in IRGen. |
|
Thanks a lot to both for the helpful insights. Support in the new swift-driver should be addressed by apple/swift-driver#1472. The implementation should aim to ensure a clear separation between the driver and the frontend. Also, thanks for sharing #38017, was not aware of this. Reviewed the discussion there carefully, it was very informative. |
antoniofrighetto
force-pushed
the
feature/load-pass-plugin
branch
from
2293235
to
177dc53
Compare
|
Fixed conflicts by rebasing to main, and changed test library type from |
There was a problem hiding this comment.
The way this patch proposes to load the plugin pass in IRGen looks good. It is exactly what we do in Clang and Flang, and what the documentation recommends.
| } else { | ||
| diagnoseSync(Diags, DiagMutex, SourceLoc(), | ||
| diag::unable_to_load_pass_plugin, PluginFile, | ||
| toString(PassPlugin.takeError())); |
There was a problem hiding this comment.
I wonder if the diagnostic handling should remain in performLLVM(). Passing Diags and DiagMutex to performLLVMOptimizations() seems a bit unfortunate. We could instead just return the llvm::Error from here and let the callers take care of it. That would change the return type of the function. A lot of code in include/swift is using llvm::Error already, but I am not sure what the conventions are in Swift. @lhames What do you think?
|
Gentle ping (cc/ @lhames). |
|
Just wanted to express support for this here as it also helps with adding plugins for AD |
|
Also expressing support for this! |
antoniofrighetto
force-pushed
the
feature/load-pass-plugin
branch
from
177dc53
to
7055ddf
Compare
|
Rebased to main, dropped unittest in |
antoniofrighetto
force-pushed
the
feature/load-pass-plugin
branch
from
7055ddf
to
564940c
Compare
|
@lhames, @airspeedswift Gentle ping. Any chance of this getting reviewed? |
|
Kind ping (maybe cc/ @adrian-prantl). |
antoniofrighetto
force-pushed
the
feature/load-pass-plugin
branch
from
564940c
to
bfc69de
Compare
|
@adrian-prantl Addressed comments and rebased to main, thank you for reviewing this! |
|
@swift-ci test |
Allow dynamic loading of LLVM passes via `load-pass-plugin` option passed to the Swift compiler driver.
antoniofrighetto
force-pushed
the
feature/load-pass-plugin
branch
from
bfc69de
to
f4d021f
Compare
|
Rebased to main, opened sister PR @ apple/llvm-project#8678, reflecting changes (not sure if we should have targeted a different branch instead). |
|
@adrian-prantl Kind ping on this, CI was previously failing. |
Allow dynamic loading of LLVM passes via
load-pass-pluginoption passed to the Swift compiler driver, similarly to current Apple Clang-fpass-pluginoption.Previous discussion: https://forums.swift.org/t/load-external-llvm-pass-plugins-via-swift-frontend/67596