Remove -enable-ossa-modules for Synchronization and Distributed #77314

meg-gupta · 2024-10-31T00:26:23Z

Synchronization and Distributed are serialized in OSSA, they import Darwin which is non-ossa. With implicit modules builds, this results in a one time recompilation of Darwin with -enable-ossa-modules.
However with Explicit Build Modules, all dependencies are treating equally and -enable-ossa-modules will be passed on only when present in the original compiler invocation.

This PR removes -enable-ossa-modules for Synchronization and Distributed and adds a diagnostic whenever we are trying to import a non-ossa module into an ossa module when Explicit Build Modules are enabled.

As a consequence one of the perf constraint tests in Synchronization started failing due to presence of copies which was previously eliminated due to being in ossa. I added @_transparent to the relevant stdlib function - Atomic.deinit to workaround this since @_transparent functions are always serialized as ossa irrespective of the presence of -enable-ossa-modules.

This is a temporary fix until ossa modules are default.

Fixes rdar://138407068

meg-gupta · 2024-10-31T00:27:37Z

@swift-ci smoke test

meg-gupta · 2024-10-31T12:54:32Z

Please test with following pull request:
swiftlang/llvm-project#9492

@swift-ci Please test

Azoy · 2024-10-31T16:55:39Z

Is this really easier than just adding -enable-ossa-modules to the Darwin module? Do you have an estimated timeline of when -enable-ossa-modules will be turned on by default?

Azoy · 2024-10-31T16:56:10Z

stdlib/public/core/CMakeLists.txt

@@ -353,6 +353,9 @@ set(swiftCore_common_options
                  SWIFT_COMPILE_FLAGS
                    ${swift_stdlib_compile_flags} -Xcc -DswiftCore_EXPORTS ${SWIFT_STANDARD_LIBRARY_SWIFT_FLAGS})

+# Enable OSSA Modules for swift stdlib


I think you can remove this since it comes for free with the changes in stdlib/cmake/modules/AddSwiftStdlib.cmake.

These modules import Darwin which is not in ossa. -enable-ossa-modules results in a one-time recompilation of dependencies when they are not in ossa. This is not compatible with Explicit Build Modules when the original invocation did not have -enable-ossa-modules.

…s for building the interface

meg-gupta · 2024-10-31T18:14:45Z

@Azoy Adding -enable-ossa-modules to Darwin will also need adding it to its dependencies which are multiple in number.

meg-gupta · 2024-10-31T18:18:20Z

Please test with following pull request:
swiftlang/llvm-project#9492

@swift-ci Please test

tshortli · 2024-10-31T18:15:47Z

lib/Serialization/SerializedModuleLoader.cpp

-    // soft reject, silently ignore.
+    // Diagnose only when explicit build modules is enabled
+    if (Ctx.SerializationOpts.ExplicitModuleBuild) {
+      Ctx.Diags.diagnose(diagLoc,


Why is this only diagnosed for explicit module builds?

I have left the behavior as is for implicit module builds. It's useful for testing -enable-ossa-modules on a module level until support for implicit module builds exist.

Would it be possible to instead introduce a flag that allows OSSA to be lenient for testing, while making both explicit and implicit module builds strict in the same way by default?

include/swift/Serialization/SerializationOptions.h

lib/Frontend/ModuleInterfaceLoader.cpp

atrick

Basically looks good to me

atrick · 2024-10-31T19:28:24Z

include/swift/AST/DiagnosticsSema.def

@@ -901,6 +901,9 @@ ERROR(serialization_target_too_new_repl,none,
      "deployment target of %0 %3: %4",
      (StringRef, llvm::VersionTuple, Identifier, llvm::VersionTuple,
       StringRef))
+ERROR(serialization_non_ossa_module_incompatible, Fatal,
+      "cannot import non-ossa module into an ossa-module",


Maybe:
"cannot import non-OSSA module into an OSSA module"

…modules are disabled

…ncies

ktoso

Not entirely sure on the implications for the Distributed module but seems fine to me?

meg-gupta · 2024-11-02T23:49:37Z

@swift-ci test

meg-gupta · 2024-11-03T00:04:28Z

@swift-ci test

meg-gupta · 2024-11-03T02:59:23Z

Please test with following pull request:
swiftlang/llvm-project#9492

@swift-ci Please test

meg-gupta · 2024-11-03T14:07:03Z

Please test with following pull request:
swiftlang/llvm-project#9492

@swift-ci Please test

meg-gupta · 2024-11-04T06:41:17Z

Please test with following pull request:
swiftlang/llvm-project#9492

@swift-ci Please test windows platform

felipepiovezan · 2024-11-04T16:10:59Z

Hi @meg-gupta , this PR broke the LLDB builds: https://ci.swift.org/view/LLDB/job/oss-lldb-incremental-macos-cmake/8289/console

Whenever an API in ASTContext.h changes, it is advisable to build LLDB as well, as it is one client of that API.

adrian-prantl · 2024-11-04T16:34:15Z

@shahmishal Why didn't the pre-merge CI catch this?

shahmishal · 2024-11-04T16:36:59Z

@adrian-prantl the llvm change it was tested with was never merged

adrian-prantl · 2024-11-04T16:41:58Z

@adrian-prantl the llvm change it was tested with was never merged

that's a relief, I was worried that during the rebranch we introduced a bot misconfiguration!

meg-gupta force-pushed the ossaflag branch from 90266da to cbe26ef Compare October 31, 2024 00:27

meg-gupta changed the title ~~Remove -enable-ossa-module for Synchronization and Distributed~~ Remove -enable-ossa-modules for Synchronization and Distributed Oct 31, 2024

Azoy reviewed Oct 31, 2024

View reviewed changes

meg-gupta added 2 commits October 31, 2024 10:55

Pass -enable-ossa-modules to the generic args while inheriting option…

10aebac

…s for building the interface

meg-gupta force-pushed the ossaflag branch from cbe26ef to 457cd5e Compare October 31, 2024 18:12

meg-gupta marked this pull request as ready for review October 31, 2024 18:13

meg-gupta requested review from hborla, xedin, artemcm, tshortli, slavapestov, ktoso, a team, ahoppen, bnbarham, CodaFi, DougGregor, hamishknight, rintaro and xymus as code owners October 31, 2024 18:13

tshortli reviewed Oct 31, 2024

View reviewed changes

meg-gupta requested review from tshortli, nate-chandler and eeckstein October 31, 2024 18:32

meg-gupta requested a review from atrick October 31, 2024 18:33

atrick reviewed Oct 31, 2024

View reviewed changes

meg-gupta added 4 commits October 31, 2024 13:03

Diagnose when trying to import non-ossa module to ossa when implicit …

51090e4

…modules are disabled

[NFC] Add SerializationOptions to ASTContext

1985b6c

[NFC] clang-format SerializationOptions.h

9dae048

Make Atomic.deinit @_transparent

dd037f8

meg-gupta force-pushed the ossaflag branch 2 times, most recently from 6d44548 to 6a718be Compare October 31, 2024 20:50

Add test to check if -enable-ossa-modules is inherited by its depende…

7a81fcc

…ncies

meg-gupta force-pushed the ossaflag branch from 6a718be to 7a81fcc Compare October 31, 2024 20:50

tshortli approved these changes Oct 31, 2024

View reviewed changes

ktoso approved these changes Oct 31, 2024

View reviewed changes

meg-gupta enabled auto-merge November 2, 2024 23:49

Add a new flag -enable-recompilation-to-ossa-module

3c29c5e

meg-gupta force-pushed the ossaflag branch from 0139052 to 3c29c5e Compare November 3, 2024 00:04

[NFC] Update embedded test

d55146c

meg-gupta merged commit c0a55e1 into swiftlang:main Nov 4, 2024
5 checks passed

This was referenced Dec 18, 2024

Remove diagnostic error for importing non-ossa module to ossa module when EBM is not enabled #78274

Merged

[6.1] Remove diagnostic error for importing non ossa module to ossa module when EBM is not enabled #78276

Merged

This was referenced Jan 7, 2025

Fix build option for Synchronization and Distributed #78460

Merged

[6.1] Fix build option for Synchronization and Distributed #78496

Merged

Remove -enable-ossa-modules for Synchronization and Distributed #77314

Remove -enable-ossa-modules for Synchronization and Distributed #77314

Uh oh!

Conversation

meg-gupta commented Oct 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meg-gupta commented Oct 31, 2024

Uh oh!

meg-gupta commented Oct 31, 2024

Uh oh!

Azoy commented Oct 31, 2024

Uh oh!

Azoy Oct 31, 2024

Choose a reason for hiding this comment

Uh oh!

meg-gupta Oct 31, 2024

Choose a reason for hiding this comment

Uh oh!

meg-gupta commented Oct 31, 2024

Uh oh!

meg-gupta commented Oct 31, 2024

Uh oh!

tshortli Oct 31, 2024

Choose a reason for hiding this comment

Uh oh!

meg-gupta Oct 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tshortli Oct 31, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

atrick left a comment

Choose a reason for hiding this comment

Uh oh!

atrick Oct 31, 2024

Choose a reason for hiding this comment

Uh oh!

meg-gupta Oct 31, 2024

Choose a reason for hiding this comment

Uh oh!

ktoso left a comment

Choose a reason for hiding this comment

Uh oh!

meg-gupta commented Nov 2, 2024

Uh oh!

meg-gupta commented Nov 3, 2024

Uh oh!

meg-gupta commented Nov 3, 2024

Uh oh!

meg-gupta commented Nov 3, 2024

Uh oh!

meg-gupta commented Nov 4, 2024

Uh oh!

Uh oh!

felipepiovezan commented Nov 4, 2024

Uh oh!

adrian-prantl commented Nov 4, 2024

Uh oh!

shahmishal commented Nov 4, 2024

Uh oh!

adrian-prantl commented Nov 4, 2024

Uh oh!

Uh oh!

meg-gupta commented Oct 31, 2024 •

edited

Loading

meg-gupta Oct 31, 2024 •

edited

Loading