-
Notifications
You must be signed in to change notification settings - Fork 216
Adds IR level PGO Instrumentation options #1992
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
base: main
Are you sure you want to change the base?
Conversation
ellishg
left a comment
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.
Do you plan to add support for -ir-profile-use and -cs-profile-use?
| if parsedOptions.hasArgument(.profileGenerate) { | ||
| commandLine.appendFlag("-fprofile-generate") | ||
| } |
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.
This feels like a bug to me. As I understand, Swift's -profile-generate flag does front-end instrumentation, and is different from Clang's -fprofile-generate.
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.
this behavior is existing in swift-driver today, and I’m a bit wary of changing it here because there may be tests and downstream flows that rely on it. This PR is primarily adding the new pieces, that said, if you feel it’s worth addressing this legacy behavior in the same PR, please let me know and I can look into it.
|
Just a friendly heads-up that new changes need to be added with this process: |
Yes, I’ve added |
Thank you for the heads-up! I’ll take a look and make sure to follow that process. |
This PR introduces three new instrumentation flags and plumbs them through to IRGen: 1. `-ir-profile-generate` - enable IR-level instrumentation. 2. `-cs-profile-generate` - enable context-sensitive IR-level instrumentation. 3. `-ir-profile-use` - IR-level PGO input profdata file to enable profile-guided optimization (both IRPGO and CSIRPGO) **Context:** https://forums.swift.org/t/ir-level-pgo-instrumentation-in-swift/82123 **Swift-driver PR:** swiftlang/swift-driver#1992 **Tests and validation:** This PR includes ir level verification tests, also checks few edge-cases when `-ir-profile-use` supplied profile is either missing or is an invalid IR profile. However, for argument validation, linking, and generating IR profiles that can later be consumed by -cs-profile-generate, I’ll need corresponding swift-driver changes. Those changes are being tracked in swiftlang/swift-driver#1992
This PR introduces three new instrumentation flags and plumbs them through to IRGen: 1. `-ir-profile-generate` - enable IR-level instrumentation. 2. `-cs-profile-generate` - enable context-sensitive IR-level instrumentation. 3. `-ir-profile-use` - IR-level PGO input profdata file to enable profile-guided optimization (both IRPGO and CSIRPGO) **Context:** https://forums.swift.org/t/ir-level-pgo-instrumentation-in-swift/82123 **Swift-driver PR:** swiftlang/swift-driver#1992 **Tests and validation:** This PR includes ir level verification tests, also checks few edge-cases when `-ir-profile-use` supplied profile is either missing or is an invalid IR profile. However, for argument validation, linking, and generating IR profiles that can later be consumed by -cs-profile-generate, I’ll need corresponding swift-driver changes. Those changes are being tracked in swiftlang/swift-driver#1992
df3add1 to
4edf9dd
Compare
Hey @artemcm , I have updated the PR post following https://github.com/swiftlang/swift-driver?tab=readme-ov-file#rebuilding-optionsswift. Please let me know if you have any other suggestions. |
This PR introduces three new instrumentation flags and plumbs them through to IRGen: 1. `-ir-profile-generate` - enable IR-level instrumentation. 2. `-cs-profile-generate` - enable context-sensitive IR-level instrumentation. 3. `-ir-profile-use` - IR-level PGO input profdata file to enable profile-guided optimization (both IRPGO and CSIRPGO) **Context:** https://forums.swift.org/t/ir-level-pgo-instrumentation-in-swift/82123 **Swift-driver PR:** swiftlang/swift-driver#1992 **Tests and validation:** This PR includes ir level verification tests, also checks few edge-cases when `-ir-profile-use` supplied profile is either missing or is an invalid IR profile. However, for argument validation, linking, and generating IR profiles that can later be consumed by -cs-profile-generate, I’ll need corresponding swift-driver changes. Those changes are being tracked in swiftlang/swift-driver#1992
|
Hey @artemcm / @cachemeifyoucan , when you get a moment, could you take a look at this PR please? Thanks! |
|
Hi @artemcm / @cachemeifyoucan, would you be able to review this or loop in the appropriate POC? Appreciate it! |
| try commandLine.appendLast(.profileGenerate, from: &parsedOptions) | ||
| try commandLine.appendLast(.irProfileGenerate, from: &parsedOptions) | ||
| try commandLine.appendLast(.irProfileGenerateEQ, from: &parsedOptions) | ||
| try commandLine.appendLast(.irProfileUse, from: &parsedOptions) |
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.
This is a flag that points to the file. It needs path remapping if needed. Use addPathOption.
Also means this is not handled correctly in swift-frontend/scanner. See the comment I added to the other review.
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 @cachemeifyoucan for looking into it. Are you suggesting something as follows
if let irProfileUsePath = parsedOptions.getLastArgument(.irProfileUse)?.asMultiple.last {
try addPathOption(option: .irProfileUse,
path: VirtualPath(path: irProfileUsePath),
to: &commandLine,
remap: jobNeedPathRemap)
}
On trying the above, I get a fatalError in CommandLineArguments.appendFlag function
fatalError("Option cannot be appended as a flag: \(option)")
I think the option .irProfileUse is of .commaJoined similar to existing flag: profileUse and would also have the same issue, do you any suggestions on how to go about it?
| for i in 1..<providedGen.count { | ||
| let error = Error.conflictingOptions(providedGen[i - 1], providedGen[i]) | ||
| diagnosticEngine.emit(.error(error), location: nil) | ||
| } |
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.
Minor: any reason for this not being similar to the C++ driver?
for i in 0..<(providedGen.count - 1) {
for j in (i + 1)..<providedGen.count {
let error = Error.conflictingOptions(providedGen[i], providedGen[j])
diagnosticEngine.emit(.error(error), location: nil)
}
}Edit: I did not realize that you were might have been moving around existing code. @kavon might be able to answer this better, but maybe what might have worked for them and only 3 flags might not work as well for 5 flags (and when the flags do not mix gen and use like in their case).
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.
Right, for more options, a minor code change was needed. Please let me know if you have any related suggestions.
| for i in 0..<(providedUse.count - 1) { | ||
| for j in (i + 1)..<providedUse.count { |
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.
Thinking that maybe is easier to understand (and to repeat here and above), if one uses enumerated():
for (i, left) in providedUse.enumerated() {
for (j, right) in providedUse.enumerated() {
guard i < j else { continue }
diagnosticEngine.emit(.error(Error.conflictingOptions(left, right)), location: nil)
}
}
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.
Thank you @drodriguez , I agree - the above certainly looks better. Updated!
4edf9dd to
5f5e3dc
Compare
Update Options.swift Update as per suggestions
5f5e3dc to
5839e6c
Compare
This PR introduces two new instrumentation flags and plumbs them through to IRGen:
-ir-profile-generate- enable IR-level instrumentation.-cs-profile-generate- enable context-sensitive IR-level instrumentation.Context: https://forums.swift.org/t/ir-level-pgo-instrumentation-in-swift/82123
Open Questions (Reviewer Input Requested)
Naming:
Is
-ir-profile-generatea good flag name?-cs-profile-generatepairs nicely with Clang’s-fcs-profile-generate. However,-ir-profile-generatefeels closer to Clang's-fprofile-generatebut-profile-generatealready exists in Swift as a frontend level and not IR level. Should it stay as is, or is there a clearer alternative?PR separation strategy:
I also updated the auto-generated Options.swift file in this PR, I am aware its not supposed to manually updated and I have a Swift-side PR open here: Add IRPGO and CSIRPGO options to Swift swift#84335
.
Should I first land a separate PR in Swift adding only the new options, and then follow up with this PR for the Driver + IRGen functionality? My current assumption is that splitting them would help unblock this PR. Is that correct?