Name platform-specific module files using a normalized target triple #22842

beccadax · 2019-02-23T01:30:26Z

Swift currently looks up platform-specific .swiftmodule, .swiftdoc, and .swiftinterface files by architecture name:

Foo.swiftmodule/
    x86_64.swiftmodule
    x86_64.swiftdoc
    arm64.swiftmodule
    arm64.swiftdoc

As part of module stability, we'd like to clean this up by using a normalized form of the target triple instead:

Foo.swiftmodule/
    x86_64-apple-ios-simulator.swiftmodule
    x86_64-apple-ios-simulator.swiftdoc
    arm64-apple-ios.swiftmodule
    arm64-apple-ios.swiftdoc

We will still support the architecture-only names for backwards compatibility, but we'll prefer the target triple names.

Normalization covers architectures ("amd64" -> "x86_64"), OSes ("macosx10.11" -> "macos"), and vendors ("unknown" -> "apple"); it also makes implicit "simulator" environments explicit. Apple platforms are the only ones that currently build for multiple architectures in practice, so normalization is only applied on those; other platforms will look for the full triple passed to the compiler (if they ever see a swiftmodule directory, which again, they won't).

This PR is best reviewed commit-by-commit. The first three commits should be NFC; if they aren't, I'd like to know, because I may have broken something subtle. There is a diagnostic I'd like to add too, but I'll do that in a separate PR.

(@aciidb0mb3r, this shouldn't break you, but you may want to be aware of it.)

Resolves rdar://problem/47094708.

Create a helper type to represent the .swiftmodule/.swiftdoc filename pair and use it in SerializedModuleLoaderBase::findModule().

…from 1-2 target-specific names to 0-N target-specific names.

lib/Serialization/SerializedModuleLoader.cpp

beccadax · 2019-02-23T01:33:23Z

@swift-ci please test

beccadax · 2019-02-23T01:36:07Z

lib/Serialization/SerializedModuleLoader.cpp

  if (Ctx.LangOpts.Target.getArch() == llvm::Triple::ArchType::arm)
-    alternateArchName = "arm";
-  ModuleFilenamePair alternateArchFileNames(alternateArchName);
+    targetFileNamePairs.push_back(StringRef("arm"));


The next commit will change StringRef("arm") to ModuleFilenamePair("arm") to improve clarity. (Forgot to amend that in.)

beccadax · 2019-02-23T02:03:25Z

lib/Basic/Platform.cpp

+              .Cases("i386", "i486", "i586", "i686", "i786", "i886", "i986",
+                     "i386")
+              .Cases("unknown", "", "unknown")
+              .Default(tripleArchName);


This catch-all would cover many of the cases above; I mention them separately because I expect that build system engineers and others who don't often work in the Swift compiler may need to read this code, and I want to be extra-clear about what's supported. (A few other parts of this implementation, like the use of arguably unnecessary helper functions with wordy names, are also influenced by that.)

It would be good to add a comment about why you mention them separately so that future maintainers will know why this is the case and how to add support for additional architectures.

Would you want to not have a default, and conditionalize calling this based on the OS instead?

This already is effectively conditionalized on Triple::isOSDarwin(). I just couldn't think of a good behavior if we didn't recognize the architecture (or, in other functions, the OS or environment)—this isn't a great place to diagnose an error, there's no sensible fixed default, and other parts of the compiler will diagnose anything the compiler is truly incapable of handling.

(I suppose one answer is that we ought to centralize our many disparate notions of OS, architecture, and environment into a single source of truth, but that's not something I want to tackle in this PR.)

beccadax · 2019-02-23T02:04:49Z

lib/Basic/Platform.cpp

+
+static StringRef
+getVendorForAppleTargetSpecificModuleTriple(const llvm::Triple &triple) {
+  return "apple";


Although we previously considered not normalizing the vendor, I discovered later that the Swift benchmark suite currently builds with an empty vendor field. I plan to fix that, but it made me rethink that decision.

There should probably be a comment explaining why you're doing this.

aciidgh · 2019-02-23T02:06:26Z

Thanks for notifying. I wonder if SwiftPM should also switch to this structure instead of emitting swiftmodule and swiftdoc files directly in the build dir.

jrose-apple · 2019-02-23T02:14:56Z

I think SwiftPM's okay for now, but if it ever wants to support multi-arch products then it'll become interesting.

beccadax · 2019-02-25T19:26:41Z

@swift-ci please test

swift-ci · 2019-02-25T19:29:00Z

Build failed
Swift Test Linux Platform
Git Sha - 1d360222b4cc9a15083fe997d157c7a0f5c3cdd7

swift-ci · 2019-02-25T20:23:37Z

Build failed
Swift Test OS X Platform
Git Sha - 1d360222b4cc9a15083fe997d157c7a0f5c3cdd7

devincoughlin

LGTM. You should have Jordan take a look as well. I have some minor comments inline.

devincoughlin · 2019-02-25T18:45:24Z

lib/Basic/Platform.cpp

+              .Cases("i386", "i486", "i586", "i686", "i786", "i886", "i986",
+                     "i386")
+              .Cases("unknown", "", "unknown")
+              .Default(tripleArchName);


It would be good to add a comment about why you mention them separately so that future maintainers will know why this is the case and how to add support for additional architectures.

devincoughlin · 2019-02-25T18:52:07Z

lib/Basic/Platform.cpp

+
+static StringRef
+getVendorForAppleTargetSpecificModuleTriple(const llvm::Triple &triple) {
+  return "apple";


There should probably be a comment explaining why you're doing this.

devincoughlin · 2019-02-25T19:00:35Z

lib/Basic/Platform.cpp

+    if (swift::tripleIsAnySimulator(triple))
+      return StringRef("simulator");
+    else
+      return None;


Nit: LLVM style guidelines discourge using 'else' after a return. http://www.llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return

You do that here and elsewhere.

devincoughlin · 2019-02-25T23:37:52Z

test/ParseableInterface/ModuleCache/force-module-loading-mode-archs.swift

@@ -60,12 +60,12 @@
 // RUN: %empty-directory(%t/Lib.swiftmodule)
 // RUN: touch %t/Lib.swiftmodule/garbage.swiftmodule
 // RUN: touch %t/Lib.swiftmodule/garbage.swiftinterface
-// RUN: not env SWIFT_FORCE_MODULE_LOADING=prefer-parseable %target-swift-frontend -typecheck -parse-stdlib -module-cache-path %t/MCP %s -I %t 2>&1 | %FileCheck -check-prefix=WRONG-ARCH -DARCH=%target-cpu %s
-// RUN: not env SWIFT_FORCE_MODULE_LOADING=prefer-serialized %target-swift-frontend -typecheck -parse-stdlib -module-cache-path %t/MCP %s -I %t 2>&1 | %FileCheck -check-prefix=WRONG-ARCH -DARCH=%target-cpu %s
+// RUN: not env SWIFT_FORCE_MODULE_LOADING=prefer-parseable %target-swift-frontend -typecheck -parse-stdlib -module-cache-path %t/MCP %s -I %t 2>&1 | %FileCheck -check-prefix=WRONG-ARCH -DARCH=%module-target-triple %s


It seems like renaming 'ARCH' to something more specific ("MODULE_TARGET"?) would make it more clear that this is not just the architecture.

It would, but there's a lot that ought to be renamed in this file (WRONG-ARCH, the filename) and ParseableInterface is still under active development, so I'm worried about conflicts if I change too much.

devincoughlin · 2019-02-25T23:48:29Z

test/Serialization/load-target-fallback.swift

+// RUN: mkdir %t/TargetLibrary.swiftmodule
+// RUN: %target-swift-frontend -emit-module -o %t/TargetLibrary.swiftmodule/%module-target-triple.swiftmodule %S/Inputs/def_func.swift -module-name TargetLibrary
+// RUN: touch %t/TargetLibrary.swiftmodule/%target-cpu.swiftmodule
+


I feel like a comment describing what this test does would be helpful to future maintainers.

jrose-apple

Looks pretty good! I agree with Devin's comments and add a few of my own, but this pretty much covers everything already.

jrose-apple · 2019-02-26T03:28:28Z

lib/Basic/Platform.cpp

@@ -12,6 +12,8 @@

 #include "swift/Basic/Platform.h"
 #include "llvm/ADT/Triple.h"
+#include "llvm/ADT/StringSwitch.h"
+#include "llvm/ADT/StringExtras.h"


Nitpick: alphabet

jrose-apple · 2019-02-26T03:29:18Z

lib/Basic/Platform.cpp

+              .Cases("i386", "i486", "i586", "i686", "i786", "i886", "i986",
+                     "i386")
+              .Cases("unknown", "", "unknown")
+              .Default(tripleArchName);


Would you want to not have a default, and conditionalize calling this based on the OS instead?

jrose-apple · 2019-02-26T03:32:12Z

lib/Basic/Platform.cpp

+  return llvm::StringSwitch<StringRef>(tripleOSNameNoVersion)
+              .Cases("macos", "macosx", "darwin", "macos")
+              .Case ("ios", "ios")
+              .Case ("tvos", "tvos")


…watchOS?

jrose-apple · 2019-02-26T03:32:54Z

lib/Basic/Platform.cpp

+    else
+      return llvm::Triple(newArch, newVendor, newOS);
+  } else {
+    return triple;


Nitpick: I'd suggest changing this one in particular to an early return.

I would normally agree with you. My concern is that, if written with an if (!triple.isDarwinOS()) early return, the natural way to add a normalization for non-Darwin OSes will be to change it to is (!triple.isDarwinOS() && !triple.isMyOtherOS()) and then modify the Apple code to handle both platforms. I really want new platforms have a separate branch, but you would need to refactor the code into something like this shape to do it. So I've essentially done that refactoring up front to make it easier.

When loading a module supporting multiple targets, the module loader now looks for a file named with a normalized version of the target triple first, and only falls back to the architecture name if the normalized triple is not found.

* Adds documentation comments. * Turns redundant cases into comments. * Removes the else branch in a couple “if (…) return …; else return …;” patterns.

beccadax · 2019-02-27T21:17:01Z

@swift-ci please test

beccadax added 3 commits February 21, 2019 18:26

[NFC] Clean up module name pairs

652cdfc

Create a helper type to represent the .swiftmodule/.swiftdoc filename pair and use it in SerializedModuleLoaderBase::findModule().

[NFC] Extract helper for loading arch-specific modules

70ba0aa

[NFC] Generalize target-specific module loading…

0df6ff4

…from 1-2 target-specific names to 0-N target-specific names.

beccadax commented Feb 23, 2019

View reviewed changes

lib/Serialization/SerializedModuleLoader.cpp Outdated Show resolved Hide resolved

beccadax requested review from jrose-apple and devincoughlin February 23, 2019 01:33

beccadax commented Feb 23, 2019

View reviewed changes

devincoughlin approved these changes Feb 25, 2019

View reviewed changes

jrose-apple reviewed Feb 26, 2019

View reviewed changes

beccadax added 3 commits February 27, 2019 11:43

Use target triple for “universal” modules

c1cc51d

When loading a module supporting multiple targets, the module loader now looks for a file named with a normalized version of the target triple first, and only falls back to the architecture name if the normalized triple is not found.

Import error now mentions the target, not the arch

3f58a79

[NFC] Restyle getTargetSpecificModuleTriple()

6476098

* Adds documentation comments. * Turns redundant cases into comments. * Removes the else branch in a couple “if (…) return …; else return …;” patterns.

beccadax force-pushed the target-practice branch from d39d959 to 6476098 Compare February 27, 2019 19:48

jrose-apple approved these changes Feb 27, 2019

View reviewed changes

beccadax merged commit f1d1df3 into swiftlang:master Mar 1, 2019

beccadax mentioned this pull request Mar 1, 2019

[5.1] Module filename and location changes #23021

Merged

Name platform-specific module files using a normalized target triple #22842

Name platform-specific module files using a normalized target triple #22842

Uh oh!

Conversation

beccadax commented Feb 23, 2019

Uh oh!

Uh oh!

beccadax commented Feb 23, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beccadax Feb 23, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aciidgh commented Feb 23, 2019

Uh oh!

jrose-apple commented Feb 23, 2019

Uh oh!

beccadax commented Feb 25, 2019

Uh oh!

swift-ci commented Feb 25, 2019

Uh oh!

swift-ci commented Feb 25, 2019

Uh oh!

devincoughlin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beccadax commented Feb 27, 2019

Uh oh!

Uh oh!

beccadax Feb 23, 2019 •

edited

Loading