Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Disable LTCG for brotli and zlibng. #111805

Merged
merged 4 commits into from
Jan 31, 2025

Conversation

jkoritzinsky
Copy link
Member

@jkoritzinsky jkoritzinsky commented Jan 24, 2025

Exclusively use CMake's INTERPROCEDURAL_OPTIMIZATION settings to handle turning on/off LTCG on Windows.

By consistently using these settings, we can be sure that we're not building anything for NativeAOT with /LTCG or /GL.

Fixes #111566

Exclusively use CMake's INTERPROCEDURAL_OPTIMIZATION settings to handle turning on/off LTCG on Windows.

By consistently using these settings, we can be sure that we're not building anything for NativeAOT with /LTCG or /GL.
@@ -30,3 +30,4 @@ target_compile_definitions(brotlicommon PRIVATE BROTLI_SHARED_COMPILATION BROTLI
# Don't build the brotli command line tool unless explicitly requested
# (which we never do)
set_target_properties(brotli PROPERTIES EXCLUDE_FROM_ALL ON)
set_property(TARGET brotlienc brotlidec brotlicommon PROPERTY INTERPROCEDURAL_OPTIMIZATION OFF)
Copy link
Member

Choose a reason for hiding this comment

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

Is this going to disable LTCG for regular Brotli build that ships in System.Compression.Native as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've adjusted this to only apply in the coreclr reference (so singlefilehost and nativeaot).

Copy link
Member

Choose a reason for hiding this comment

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

Is it too hard to keep LTCG for singlefilehost?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, our current build setup isn't conducive to building two flavors of our vendored libraries (we would need to be able to define separate target names and include the subdirectories twice).

There are alternative ways we could include the vendored libs, but that would require significant changes to how we build. I think that's something we can consider in .NET 10 but in a later PR (especially if we want this in P1).

Copy link
Member

Choose a reason for hiding this comment

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

Could you please open an issue on this?

Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

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

I'd be fine even if this disables LTCG on all flavors of the library. This would be a stopgap for Preview 1. We'd not resolve the bug and can address the rest after preview 1 snap.

@ericstj ericstj requested a review from carlossanlop January 27, 2025 16:46
@MichalStrehovsky MichalStrehovsky dismissed their stale review January 28, 2025 12:47

Dismissing since we missed the Preview 1 cutoff so there's no point in a half-fix now.

@jkoritzinsky jkoritzinsky linked an issue Jan 28, 2025 that may be closed by this pull request
@jkoritzinsky
Copy link
Member Author

@MichalStrehovsky we can try to backport this fix to the P1 branch as it is a blocker for NAOT once we get this in.

Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

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

I'm not exactly fluent in CMake and this is some advanced CMake, but LGTM to the extent I can read this.

@jkoritzinsky
Copy link
Member Author

/ba-g unrelated failures

@jkoritzinsky jkoritzinsky merged commit 8edaf74 into dotnet:main Jan 31, 2025
155 of 162 checks passed
@jkoritzinsky jkoritzinsky deleted the external-no-ipo branch January 31, 2025 21:50
grendello added a commit to grendello/runtime that referenced this pull request Feb 3, 2025
* main:
  System.Net.Http.WinHttpHandler.StartRequestAsync assertion failed (dotnet#109799)
  Keep test PDB in helix payload for native AOT (dotnet#111949)
  Build the RID-specific System.IO.Ports packages in the VMR (dotnet#112054)
  Always inline number conversions (dotnet#112061)
  Use Contains{Any} in Regex source generator (dotnet#112065)
  Update dependencies from https://github.com/dotnet/arcade build 20250130.5 (dotnet#112013)
  JIT: Transform single-reg args to FIELD_LIST in physical promotion (dotnet#111590)
  Ensure that math calls into the CRT are tracked as needing vzeroupper (dotnet#112011)
  Use double.ConvertToIntegerNative where safe to do in System.Random (dotnet#112046)
  JIT: Compute `fgCalledCount` after synthesis (dotnet#112041)
  Simplify boolean logic in `TimeZoneInfo` (dotnet#112062)
  JIT: Update type when return temp is freshly created (dotnet#111948)
  Remove unused build controls and simplify DotNetBuild.props (dotnet#111986)
  Fix case-insensitive JSON deserialization of enum member names (dotnet#112028)
  WasmAppBuilder: Remove double computation of a value (dotnet#112047)
  Disable LTCG for brotli and zlibng. (dotnet#111805)
  JIT: Improve x86 unsigned to floating cast codegen (dotnet#111595)
  simplify x86 special intrinsic imports (dotnet#111836)
  JIT: Try to retain entry weight during profile synthesis (dotnet#111971)
  Fix explicit offset of ByRefLike fields. (dotnet#111584)
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

libnethost is built containing objects with /LTCG enabled Brotli native code is compiled with LTCG
3 participants