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

Local linux builds should not enable PGO by default #85785

Closed
sbomer opened this issue May 4, 2023 · 4 comments · Fixed by #84676
Closed

Local linux builds should not enable PGO by default #85785

sbomer opened this issue May 4, 2023 · 4 comments · Fixed by #84676

Comments

@sbomer
Copy link
Member

sbomer commented May 4, 2023

With recent upgrades to compiler versions (and upcoming upgrade to clang 16) in our official builds, our profile data uses an increasingly recent version of the PGO data format, which requires a similarly recent version of the compiler in order to optimize using this data.

Local builds and the runtime-dev-innerloop build should not enable PGO optimization in order to avoid requiring the very latest clang.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label May 4, 2023
@ghost
Copy link

ghost commented May 4, 2023

Tagging subscribers to this area: @hoyosjs
See info in area-owners.md if you want to be subscribed.

Issue Details

With recent upgrades to compiler versions (and upcoming upgrade to clang 16) in our official builds, our profile data uses an increasingly recent version of the PGO data format, which requires a similarly recent version of the compiler in order to optimize using this data.

Local builds and the runtime-dev-innerloop build should not enable PGO optimization in order to avoid requiring the very latest clang.

Author: sbomer
Assignees: -
Labels:

area-Infrastructure-coreclr, untriaged

Milestone: -

@sbomer sbomer self-assigned this May 4, 2023
@BruceForstall
Copy link
Member

This is specifically about non-Windows builds, right? Presumably local Windows Release builds could continue to consume PGO data without issue?

@sbomer sbomer changed the title Local builds should not enable PGO by default Local linux builds should not enable PGO by default May 4, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 6, 2023
sbomer added a commit that referenced this issue May 10, 2023
This pulls in the updates from
dotnet/dotnet-buildtools-prereqs-docker@53fee55
and moves to floating tags for the mariner images, which now
build the product with LLVM 16.

Once this flows to dotnet-optimization, this will require updates
to make the LLVM 16 tools available there. Before the opt data
collected with new LLVM flows into runtime, I plan to fix
#85785 so that local
builds won't by default require clang-16.
@ghost ghost removed in-pr There is an active PR which will close this issue when it is merged untriaged New issue has not been triaged by the area owner labels May 10, 2023
@sbomer sbomer reopened this May 10, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label May 10, 2023
@sbomer
Copy link
Member Author

sbomer commented May 21, 2023

After some more thought, I think we should make this dependent on the compiler version. If your build machine has clang-16 available, there's no reason not to enable PGO. This makes the fix pretty simple, so I pushed it to #86436 to unblock the opt data flow. The fix will result in the following warning if clang-16 isn't available, but won't fail the build:

CMake Warning at pgosupport.cmake:79 (message):
    PGO is not supported; Clang 16 or later is required for profile guided
    optimizations

I initially stared going down the path of changing NoPgoOptimize to default to true for linux builds, and passing NoPgoOptimize=false where required. But I didn't like this for a few reasons:

  • It requires passing more arguments through the already-complex yml.
  • It makes it more difficult to repro ci builds locally (requires an extra argument, in addition to the latest compiler)
  • NoPgoOptimize=false is just confusing (it doesn't mean PGO optimizations are enabled - only that PGO will be enabled in release builds, for supported platforms)

@sbomer
Copy link
Member Author

sbomer commented May 22, 2023

Fixed in #86436.

@sbomer sbomer closed this as completed May 22, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label May 22, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jun 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants