Skip to content

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.

Sign up for GitHub

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

Open
chiragramani wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Adds IR level PGO Instrumentation options #1992

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 84 additions & 13 deletions Sources/SwiftDriver/Driver/Driver.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3334,24 +3334,91 @@ extension Driver {
}
}

static private func validateProfilingGenerateArgs(
_ parsedOptions: inout ParsedOptions,
diagnosticEngine: DiagnosticsEngine
) {
let genFlags: [Option] = [
.profileGenerate,
.irProfileGenerate,
.irProfileGenerateEQ,
.csProfileGenerate,
.csProfileGenerateEQ,
]
func resolveDualFormConflict(_ plain: Option, _ equalsForm: Option) {
if parsedOptions.hasArgument(plain),
parsedOptions.hasArgument(equalsForm)
{
diagnosticEngine.emit(
.error(Error.conflictingOptions(plain, equalsForm)),
location: nil
)
providedGen.removeAll { $0 == equalsForm }
}
}

var providedGen = genFlags.filter { parsedOptions.hasArgument($0) }
resolveDualFormConflict(.irProfileGenerate, .irProfileGenerateEQ)
resolveDualFormConflict(.csProfileGenerate, .csProfileGenerateEQ)

guard providedGen.count >= 2 else { return }
for i in 1..<providedGen.count {
let error = Error.conflictingOptions(providedGen[i - 1], providedGen[i])
diagnosticEngine.emit(.error(error), location: nil)
}
}

static private func validateProfilingUseArgs(
_ parsedOptions: inout ParsedOptions,
diagnosticEngine: DiagnosticsEngine
) {
let conflictingGenFlags: [Option] = [
.profileGenerate,
.irProfileGenerate,
.irProfileGenerateEQ,
]
let useProfArgs: [Option] = [
.profileUse,
.profileSampleUse,
.irProfileUse
]
let providedUse = useProfArgs.filter { parsedOptions.hasArgument($0) }
guard !providedUse.isEmpty else { return }

// At most one *use* option allowed
if providedUse.count > 1 {
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)
}
}
}

// If no generate flags, we're good.
let providedGen = conflictingGenFlags.filter { parsedOptions.hasArgument($0) }
guard !providedGen.isEmpty else { return }

// We already diagnosed if the user passed more than one "use" option
// (e.g. both `-profile-use` and `-profile-sample-use`). To avoid
// spamming diagnostics, we now treat the first provided "use" flag
// as the canonical representative.
let canonicalUse = providedUse[0]

// Generate vs Use are mutually exclusive
for g in providedGen {
diagnosticEngine.emit(.error(Error.conflictingOptions(g, canonicalUse)), location: nil)
}
}


static func validateProfilingArgs(_ parsedOptions: inout ParsedOptions,
fileSystem: FileSystem,
workingDirectory: AbsolutePath?,
diagnosticEngine: DiagnosticsEngine) {
let conflictingProfArgs: [Option] = [.profileGenerate,
.profileUse,
.profileSampleUse]

// Find out which of the mutually exclusive profiling arguments were provided.
let provided = conflictingProfArgs.filter { parsedOptions.hasArgument($0) }

// If there's at least two of them, there's a conflict.
if provided.count >= 2 {
for i in 1..<provided.count {
let error = Error.conflictingOptions(provided[i-1], provided[i])
diagnosticEngine.emit(.error(error), location: nil)
}
}
validateProfilingGenerateArgs(&parsedOptions, diagnosticEngine: diagnosticEngine)
validateProfilingUseArgs(&parsedOptions, diagnosticEngine: diagnosticEngine)

// Ensure files exist for the given paths.
func checkForMissingProfilingData(_ profileDataArgs: [String]) {
Expand All @@ -3376,6 +3443,10 @@ extension Driver {
if let profileSampleUseArg = parsedOptions.getLastArgument(.profileSampleUse)?.asSingle {
checkForMissingProfilingData([profileSampleUseArg])
}

if let irProfileUseArgs = parsedOptions.getLastArgument(.irProfileUse)?.asMultiple {
checkForMissingProfilingData(irProfileUseArgs)
}
}

static func validateParseableOutputArgs(_ parsedOptions: inout ParsedOptions,
Expand Down
4 changes: 1 addition & 3 deletions Sources/SwiftDriver/Jobs/DarwinToolchain+LinkerSupport.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,9 +258,7 @@ extension DarwinToolchain {
fileSystem: fileSystem
)

if parsedOptions.hasArgument(.profileGenerate) {
commandLine.appendFlag("-fprofile-generate")
}
commandLine.appendFlags(mapInstrumentationTypeToClangArgs(from: &parsedOptions))

// These custom arguments should be right before the object file at the
// end.
Expand Down
5 changes: 5 additions & 0 deletions Sources/SwiftDriver/Jobs/FrontendJobHelpers.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,11 @@ extension Driver {
try commandLine.appendLast(.RpassMissedEQ, from: &parsedOptions)
try commandLine.appendLast(.suppressWarnings, from: &parsedOptions)
try commandLine.appendLast(.profileGenerate, from: &parsedOptions)
try commandLine.appendLast(.irProfileGenerate, from: &parsedOptions)
try commandLine.appendLast(.irProfileGenerateEQ, from: &parsedOptions)
try commandLine.appendLast(.irProfileUse, from: &parsedOptions)
Copy link
Contributor

@cachemeifyoucan cachemeifyoucan Nov 10, 2025

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.

Copy link
Author

@chiragramani chiragramani Nov 14, 2025

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?

Copy link
Author

@chiragramani chiragramani Nov 21, 2025

Choose a reason for hiding this comment

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

Hey @cachemeifyoucan / @drodriguez ,
Can you guide me on how to go about addressing this crash please?

Copy link
Contributor

@drodriguez drodriguez Nov 21, 2025

Choose a reason for hiding this comment

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

I don't understand the why of the request from @cachemeifyoucan . I cannot help with that. .profileUse is also a .commaJoined flag, and it is following the same pattern a couple of lines below. Seems that it has not been a problem until now, so not sure why it should be a problem to follow this pattern now. When the addPathOption was introduced a couple of years ago it seemed to be fine to not change .profileUse.

I think addPathOption is probably not directly usable here because if I understand correctly .commaJoined means that you can add several paths into a single argument separated by commas, and addPathOption is not prepared for that (or for adding several paths). It sounds like one has to write special code to do the same thing that's happening in addPathOption when the flags are .commaJoined, which does whatever is needed with VirtualPath for each element of the input argument (not only the .last).

Copy link
Author

@chiragramani chiragramani Dec 2, 2025

Choose a reason for hiding this comment

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

Thanks @drodriguez , I share the same understanding.
@cachemeifyoucan, @artemcm please let me know what you all think and what I can do to unblock this PR?

Copy link
Contributor

@drodriguez drodriguez Dec 15, 2025

Choose a reason for hiding this comment

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

@cachemeifyoucan should it be done as part of this PR or as part of a follow up PR, since .profileUse is also affected, and it is part of the current code without issues? If this is the only feedback left for this PR, it will allow merging this, and handling the specifics of the new functionality in a different PR.

Copy link
Contributor

@cachemeifyoucan cachemeifyoucan Dec 15, 2025

Choose a reason for hiding this comment

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

Please address it here if there is no good reason. You can do it in separate commit, but it is not something you can drop on the ground and let it break for days.

Copy link
Contributor

@drodriguez drodriguez Dec 15, 2025

Choose a reason for hiding this comment

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

@cachemeifyoucan it has been "broken" for a couple of years for .profileUse (still "broken" on main in fact). It will not be left more "broken" than it has already been.

Copy link
Contributor

@cachemeifyoucan cachemeifyoucan Dec 15, 2025

Choose a reason for hiding this comment

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

Sorry, I thought you always meant .irProfileUse and didn't realize you are talking about an old existing feature.

Please file an issue about missing caching support for both profile usage. I just checked that both profile usages don't work with swift caching even from swift-frontend side so there are more works to do in the compiler before swift-driver can make it possible to work.

Copy link
Contributor

@cachemeifyoucan cachemeifyoucan Dec 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

That said. I no longer block this change once we have the GitHub issue to tracking the missing feature.

try commandLine.appendLast(.csProfileGenerate, from: &parsedOptions)
try commandLine.appendLast(.csProfileGenerateEQ, from: &parsedOptions)
try commandLine.appendLast(.profileUse, from: &parsedOptions)
try commandLine.appendLast(.profileCoverageMapping, from: &parsedOptions)
try commandLine.appendLast(.debugInfoForProfiling, from: &parsedOptions)
Expand Down
2 changes: 1 addition & 1 deletion Sources/SwiftDriver/Jobs/GenericUnixToolchain+LinkerSupport.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,7 @@ extension GenericUnixToolchain {
}
}

if parsedOptions.hasArgument(.profileGenerate) {
if needsInstrumentedProfile(from: &parsedOptions) {
let environment = (targetTriple.environment == .android) ? "-android" : ""
let libProfile = VirtualPath.lookup(targetInfo.runtimeResourcePath.path)
.appending(components: "clang", "lib", targetTriple.osNameUnversioned,
Expand Down
2 changes: 1 addition & 1 deletion Sources/SwiftDriver/Jobs/WebAssemblyToolchain+LinkerSupport.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ extension WebAssemblyToolchain {
commandLine.appendFlag("-fsanitize=\(sanitizerNames)")
}

if parsedOptions.hasArgument(.profileGenerate) {
if needsInstrumentedProfile(from: &parsedOptions) {
let libProfile = VirtualPath.lookup(targetInfo.runtimeResourcePath.path)
.appending(components: "clang", "lib", targetTriple.osName,
"libclang_rt.profile-\(targetTriple.archName).a")
Expand Down
7 changes: 4 additions & 3 deletions Sources/SwiftDriver/Jobs/WindowsToolchain+LinkerSupport.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ extension WindowsToolchain {
// for now, which supports the behavior via a flag.
// TODO: Once we've changed coverage to no longer rely on emitting
// duplicate weak symbols (rdar://131295678), we can remove this.
if parsedOptions.hasArgument(.profileGenerate) { return true }
if needsInstrumentedProfile(from: &parsedOptions) { return true }

return false
}()
Expand Down Expand Up @@ -228,11 +228,12 @@ extension WindowsToolchain {
commandLine.appendFlag("-fsanitize=\(sanitize)")
}

if parsedOptions.contains(.profileGenerate) {
if needsInstrumentedProfile(from: &parsedOptions) {
assert(bForceLLD,
"LLD is currently required for profiling (rdar://131295678)")

commandLine.appendFlag("-fprofile-generate")
commandLine.appendFlags(mapInstrumentationTypeToClangArgs(from: &parsedOptions))

// FIXME(rdar://131295678): Currently profiling requires the ability to
// emit duplicate weak symbols. Assume we're using lld and pass
// `-lld-allow-duplicate-weak` to enable this behavior.
Expand Down
37 changes: 37 additions & 0 deletions Sources/SwiftDriver/Toolchains/Toolchain.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,43 @@ extension Toolchain {
}
return clangArg
}

internal func mapInstrumentationTypeToClangArgs(from options: inout ParsedOptions) -> [String] {
var args: [String] = []

if options.contains(.profileGenerate) || options.contains(.irProfileGenerate) {
args.append("-fprofile-generate")
}

if options.contains(.irProfileGenerateEQ),
let path = options.getLastArgument(.irProfileGenerateEQ)?.asSingle {
args.append("-fprofile-generate=\(path)")
}

if options.contains(.csProfileGenerate) {
args.append("-fcs-profile-generate")
}

if options.contains(.csProfileGenerateEQ),
let path = options.getLastArgument(.csProfileGenerateEQ)?.asSingle {
args.append("-fcs-profile-generate=\(path)")
}

if options.contains(.irProfileUse),
let path = options.getLastArgument(.irProfileUse)?.asMultiple.last {
args.append("-fprofile-use=\(path)")
}

return args
}

internal func needsInstrumentedProfile(from parsedOptions: inout ParsedOptions) -> Bool {
parsedOptions.contains(.profileGenerate) ||
parsedOptions.contains(.irProfileGenerate) ||
parsedOptions.contains(.irProfileGenerateEQ) ||
parsedOptions.contains(.csProfileGenerate) ||
parsedOptions.contains(.csProfileGenerateEQ)
}
}

@_spi(Testing) public enum ToolchainError: Swift.Error {
Expand Down
Loading