Skip to content
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

[Driver][Frontend] Introduce load-pass-plugin option #68985

Merged

Conversation

antoniofrighetto
Copy link
Contributor

Allow dynamic loading of LLVM passes via load-pass-plugin option passed to the Swift compiler driver, similarly to current Apple Clang -fpass-plugin option.

Previous discussion: https://forums.swift.org/t/load-external-llvm-pass-plugins-via-swift-frontend/67596

@antoniofrighetto
Copy link
Contributor Author

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.

@antoniofrighetto
Copy link
Contributor Author

(cc/ @DougGregor)

@xedin xedin removed their request for review October 5, 2023 23:05
@antoniofrighetto
Copy link
Contributor Author

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).

@jslegendre
Copy link
Contributor

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

@artemcm
Copy link
Contributor

artemcm commented Oct 19, 2023

There was some discussion on this previously in:
#38017

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.

@antoniofrighetto
Copy link
Contributor Author

Thanks a lot to both for the helpful insights. Support in the new swift-driver should be addressed by swiftlang/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
Copy link
Contributor Author

Fixed conflicts by rebasing to main, and changed test library type from MODULE to SHARED, as CMake seems to be defaulting to generate the plugin with .so extension on macOS too (rather than .dylib, this was previously leading the unittest to fail).

Copy link
Contributor

@weliveindetail weliveindetail left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@antoniofrighetto
Copy link
Contributor Author

Gentle ping (cc/ @lhames).

@wsmoses
Copy link

wsmoses commented Nov 22, 2023

Just wanted to express support for this here as it also helps with adding plugins for AD

@jslegendre
Copy link
Contributor

Also expressing support for this!

@antoniofrighetto
Copy link
Contributor Author

Rebased to main, dropped unittest in Driver as part of recent legacy driver deprecation, added test coverage exercizing performLLVMOptimization. New driver support ready at swiftlang/swift-driver#1472. Kind ping @lhames, @aschwaighofer, @artemcm, @benlangmuir, @weliveindetail.

@antoniofrighetto
Copy link
Contributor Author

@lhames, @airspeedswift Gentle ping. Any chance of this getting reviewed?

@antoniofrighetto
Copy link
Contributor Author

Kind ping (maybe cc/ @adrian-prantl).

lib/IRGen/IRGen.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@adrian-prantl adrian-prantl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a couple of minor comments inside.

@antoniofrighetto
Copy link
Contributor Author

@adrian-prantl Addressed comments and rebased to main, thank you for reviewing this!

@adrian-prantl
Copy link
Contributor

@swift-ci test

@antoniofrighetto
Copy link
Contributor Author

Rebased to main, opened sister PR @ swiftlang/llvm-project#8678, reflecting changes (not sure if we should have targeted a different branch instead).

@antoniofrighetto
Copy link
Contributor Author

@adrian-prantl Kind ping on this, CI was previously failing.

@antoniofrighetto
Copy link
Contributor Author

antoniofrighetto commented Jul 31, 2024

Rebased to main (fixed conflicts), swiftlang/llvm-project#8678 PR rebased to stable/20230725, which should be the expected branch. Very gentle remainder. I do not have commit access, could anyone kindly merge this?

@antoniofrighetto
Copy link
Contributor Author

antoniofrighetto commented Jul 31, 2024

I assume this should be tested with:

Please test with following PR:
https://github.com/swiftlang/llvm-project/pull/8678
https://github.com/swiftlang/swift-driver/pull/1472

@swift-ci Please test

@JDevlieghere
Copy link
Contributor

@JDevlieghere
Copy link
Contributor

JDevlieghere commented Sep 10, 2024

The macOS failure is:

/Users/ec2-user/jenkins/workspace/swift-PR-macos/branch-main/swift/stdlib/private/SwiftPrivate/IO.swift:21:8: error: failed to build module 'Darwin'; this SDK is not supported by the compiler (the SDK is built with 'Apple Swift version 5.9.2 (swiftlang-5.9.2.2.11 clang-1500.1.0.2.2)', while this compiler is 'Apple Swift version 6.1-dev effective-5.10 (LLVM 97c65ef47934098, Swift c06508954544f05)'). Please select a toolchain which matches the SDK.
 19 | #else
 20 | #if canImport(Darwin)
 21 | import Darwin
    |        `- error: failed to build module 'Darwin'; this SDK is not supported by the compiler (the SDK is built with 'Apple Swift version 5.9.2 (swiftlang-5.9.2.2.11 clang-1500.1.0.2.2)', while this compiler is 'Apple Swift version 6.1-dev effective-5.10 (LLVM 97c65ef47934098, Swift c06508954544f05)'). Please select a toolchain which matches the SDK.

edit: should have been fixed by #76344

@JDevlieghere
Copy link
Contributor

swiftlang/llvm-project#8678
swiftlang/swift-driver#1472

@swift-ci test macos

@weliveindetail
Copy link
Contributor

@JDevlieghere This will be affected by the rebranch isn't it? Any chance it can land before?

@artemcm
Copy link
Contributor

artemcm commented Oct 23, 2024

@adrian-prantl @JDevlieghere is this good to merge?

@adrian-prantl
Copy link
Contributor

I don't have any further concerns with this patch.

Allow dynamic loading of LLVM passes via `load-pass-plugin`
option passed to the Swift compiler driver.
@antoniofrighetto
Copy link
Contributor Author

Rebased to main, fixed conflicts (to be tested again as above otherwise CI fails), kind ping for merging this.

@aschwaighofer
Copy link
Contributor

@swift-ci test

@antoniofrighetto
Copy link
Contributor Author

@swift-ci test

Thanks, the other two PRs (swiftlang/llvm-project#8678, swiftlang/swift-driver#1472) are also required for the CI to pass.

@aschwaighofer
Copy link
Contributor

@aschwaighofer
Copy link
Contributor

@antoniofrighetto Can you put up a PR against stable/20240723. Swift has rebased against that newer llvm-project branch.

@antoniofrighetto
Copy link
Contributor Author

Done at swiftlang/llvm-project#9586, thanks.

@aschwaighofer
Copy link
Contributor

@antoniofrighetto
Copy link
Contributor Author

Kind ping (before other rebases conflicts pop up :P)

@al45tair
Copy link
Contributor

@antoniofrighetto @aschwaighofer
The test for this is failing under ASAN because of an ODR rule violation (there are two vtables for llvm::circular_raw_ostream). See #77771.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants