[runtime] Remove TwoWordPair and use the Swift calling convention instead. #13299

troughton · 2017-12-06T07:09:09Z

Historically, TwoWordPair seems to be a >4 year old hack that worked around the lack of a dedicated calling convention to interoperate with Swift. On some platforms, it packed two word-sized values into a larger integer to ensure it would be passed in registers.

However, this approach doesn't work on platforms such as 64-bit Windows, where a calling convention such as __vectorcall would need to be used instead (which would pollute the codebase quite significantly).

Now that SwiftCC exists (and works on Windows, once apple/swift-clang#142 is merged) we don't need that hack anymore and can instead mark all functions that used TwoWordPair::Return as using the Swift calling convention.

Note that I split up TwoWordPair into different struct types. Using the templated TwoWordPair gives compile-time warnings that the functions have C-linkage but use an incomplete type that's potentially incompatible with C.

This has passed the long test suite locally, but I wouldn't be surprised if there's some subtle breakage somewhere I've missed. It does also require that every platform which previously needed a workaround supports SwiftCC, which I believe to be the case.

This fixes Swift functionality on 64-bit Windows; previously we would crash in an assertion on ManagedBufferPointer when trying to do anything more than a print statement since ClassExtents wasn't correctly being returned from _getSwiftClassInstanceExtents.

troughton · 2017-12-06T07:12:16Z

@jrose-apple I went with this solution rather than what was suggested on swift-dev since the functions were already marked extern "C" and I wasn't sure how to ensure C-style mangling (or no mangling) with a __vectorcall function. This also just seemed cleaner overall - fewer platform-specific workarounds.

troughton · 2017-12-06T11:13:54Z

include/swift/Runtime/Config.h

@@ -131,6 +131,8 @@
 #define SWIFT_CC_DefaultCC_IMPL SWIFT_CC_c
 #define SWIFT_LLVM_CC_DefaultCC llvm::CallingConv::C

+#define SWIFT_CC_SwiftCC SWIFT_CC_swift


The Swift calling convention is called SWIFT_CC_swift directly and SWIFT_LLVM_CC_SwiftCC to refer to the LLVM name. These need to match (either swift or SwiftCC) for the purposes of the macros in RuntimeFunctions.def, so I chose to alias SwiftCC to swift here.

I don't understand why this is necessary. RuntimeFunctions.def already uses SwiftCC for two functions. Why is this only now a problem?

Previously, both SwiftCC functions in RuntimeFunctions.def used the FUNCTION macro. However, swift_allocBox uses FUNCTION_WITH_GLOBAL_SYMBOL_AND_IMPL.

In RuntimeEntrySymbols.cpp, that FUNCTION_WITH_GLOBAL_SYMBOL_AND_IMPL expands into DEFINE_SYMBOL, whereas FUNCTION, as defined in that same file, expands to nothing for any calling convention other than RegisterPreservingCC.

DEFINE_SYMBOL uses the SWIFT_CC macro, which expands to SWIFT_CC_##CC. Since SWIFT_CC_SwiftCC was undefined, I defined it here.

Got it. That's fine.

troughton · 2017-12-06T11:15:20Z

include/swift/Runtime/HeapObject.h

-#endif
+struct BoxPair {
+  HeapObject *first;
+  OpaqueValue *second;


I wasn't sure for the best name for these variables (they're never actually referred to by name), so I kept with the precedent from TwoWordPair. object and value don't seem ideal as they remind me of e.g. a key-value pair.

object and buffer would be appropriate.

jrose-apple · 2017-12-06T17:28:34Z

lib/IRGen/IRGenModule.cpp

@@ -610,6 +610,10 @@ llvm::Constant *swift::getWrapperFn(llvm::Module &Module,
                                                  RETURNS, ARGS, ATTRS)        \
  FUNCTION_IMPL(ID, NAME, CC, QUOTE(RETURNS), QUOTE(ARGS), QUOTE(ATTRS))

+#define FUNCTION_WITH_GLOBAL_SYMBOL_FOR_CONV_SwiftCC(ID, NAME, SYMBOL, CC,        \
+                                                  RETURNS, ARGS, ATTRS)        \
+  FUNCTION_IMPL(ID, NAME, CC, QUOTE(RETURNS), QUOTE(ARGS), QUOTE(ATTRS))


Isn't it a problem to drop the SYMBOL part here? I thought that's what you were trying to fix on Windows.

It's not actually unused. Like for the other calling conventions in this file, the SYMBOL arg is dropped here but used in RuntimeEntrySymbols.cpp. I only had to add this macro here because SwiftCC wasn't previously being used on a global symbol.

This 'fix' for Windows is really a unification across all platforms by making sure the calling convention for these functions is guaranteed to be compatible with Swift.

Oops, didn't think to check for that. Thanks for the explanation.

gparker42 · 2017-12-06T22:50:45Z

Forcing SwiftCC has a drawback: it means the function can't currently be imported into Swift using the usual import machinery. None of these functions are imported that way, though, and I don't think the risk of wanting to add a new one is too large.

SwiftCC functions also are implied to use a different retain-count convention, but again I don't think that affects any of these functions.

gparker42 · 2017-12-06T22:51:47Z

include/swift/Runtime/Config.h

@@ -122,7 +122,7 @@
 #define SWIFT_LLVM_CC(CC) SWIFT_LLVM_CC_##CC

 // Currently, RuntimeFunction.def uses the following calling conventions:


While you're here, can you correct this misspelling? The file name is RuntimeFunctions.def.

gparker42 · 2017-12-06T23:32:08Z

include/swift/Runtime/HeapObject.h

-  TwoWordPair() = default;
-  TwoWordPair(A first, B second);
-
-  // FIXME: rdar://16257592 arm codegen doesn't call swift_allocBox correctly.


rdar://16257592 was closed long ago, so we're good here.

troughton · 2017-12-06T23:44:07Z

include/swift/Runtime/HeapObject.h

-  // in registers, so cram the result into an unsigned long long.
-  // Use an enum class with implicit conversions so we don't dirty C callers
-  // too much.
-#if __arm__ || __i386__ || defined(__CYGWIN__) || defined(_MSC_VER)


One consideration is that this could cause any of these platforms that tries to build without SWIFT_USE_SWIFTCALL (e.g. with an unsupported Clang) to silently break. Is there any reason that SWIFT_USE_SWIFTCALL can't be required now, so not supporting it is an error? Otherwise it might be worth adding back an #if block for only these platforms with an #error that the Swift calling convention must be enabled.

Good point. If we start requiring SWIFT_USE_SWIFTCALL then we ought to remove its other uses in Config.h and add the #error check there.

I see at least one reference to the C compiler being MSVC (swift/Basic/Compiler.h). I assume it has no swiftcall support. Do we actually support MSVC? Is anyone using it?

Formally requiring recent clang would be worth a heads-up notice or a discussion on swift-evolution.

Also, I see that the default definition of SWIFT_USE_SWIFTCALL in Config.h still disables it. Are you building locally with it enabled?

Yes; I've submitted a PR for Clang that means we can enable it on Windows.

As for MSVC: I know @hughbe was doing a lot of work around getting the compiler building under MSVC. However, if I'm not mistaken, SWIFT_USE_SWIFTCALL and its descendants are only used in the stdlib/runtime, which is normally built using the just-built Clang anyway. The requirement then is a little less strict: the stdlib/runtime must be built with a recent Clang - and I think that was already a requirement.

I think that distinction will work. Probably looks something like this:

Remove SWIFT_USE_SWIFTCALL from Config.h. (In the interim add some #if !SWIFT_USE_SWIFTCALL #error "SWIFT_USE_SWIFTCALL=0 is not supported" to catch anyone switching it externally.)

Use __has_attribute(swiftcall) to define the macros formerly controlled by SWIFT_USE_SWIFTCALL. Or possibly define them unconditionally, if they are never used in code that is included in the compiler's build.

Add __has_attribute(swiftcall) enforcement somewhere in runtime-only code. I don't know of any established place for such a thing.

troughton · 2017-12-07T04:00:15Z

I should note that apparently I missed the relevant tests before, and will need to update them to include the swiftcc marker before this gets merged.

troughton · 2017-12-07T07:38:05Z

After updating the SIL tests to account for the added swiftcc token, all tests pass locally on both macOS and Linux hosts.

The change to always use SwiftCC is in #13311; this PR and that can be merged in any order and then I'll rebase the other.

gparker42 · 2017-12-07T08:48:14Z

@swift-ci please test

swift-ci · 2017-12-07T08:49:41Z

Build failed
Swift Test Linux Platform
Git Sha - 0a123c7

swift-ci · 2017-12-07T08:52:47Z

Build failed
Swift Test OS X Platform
Git Sha - 0a123c7

troughton · 2017-12-07T09:28:23Z

Oops, sorry, that build failure's my mistake; I forgot to cherry pick a commit across to this branch.

gparker42 · 2017-12-08T00:20:12Z

@swift-ci please test

swift-ci · 2017-12-08T00:22:26Z

Build failed
Swift Test Linux Platform
Git Sha - 01ef8b2

swift-ci · 2017-12-08T00:26:24Z

Build failed
Swift Test OS X Platform
Git Sha - 01ef8b2

troughton · 2017-12-08T02:43:43Z

Tests seem to have passed despite the "Build failed" messages.

gparker42 · 2017-12-08T03:26:05Z

Right. It's a CI glitch. If you look at those failure messages they say that it's automatically retrying.

clang is miscompiling some swiftcall functions on armv7s. Stop using swiftcall in some places until it is fixed. Reverts c5bf2ec (swiftlang#13299). rdar://35973477

gparker42 · 2018-01-23T07:11:39Z

Reverted in #14079. clang is miscompiling the swiftcall version of _swift_allocBox_ on armv7s.

clang is miscompiling some swiftcall functions on armv7s. Stop using swiftcall in some places until it is fixed. Reverts c5bf2ec (#13299). rdar://35973477

…g#14079) clang is miscompiling some swiftcall functions on armv7s. Stop using swiftcall in some places until it is fixed. Reverts c5bf2ec (swiftlang#13299). rdar://35973477

) clang is miscompiling some swiftcall functions on armv7s. Stop using swiftcall in some places until it is fixed. Reverts c5bf2ec (#13299). rdar://35973477

gparker42 · 2018-01-30T23:45:02Z

We should be ready to put this back now. Thomas, do you want to take it?

troughton · 2018-01-31T01:16:19Z

Sure. I'll take a look at it in the next few days.

Remove TwoWordPair and use SwiftCC instead.

0a123c7

troughton commented Dec 6, 2017

View reviewed changes

jrose-apple reviewed Dec 6, 2017

View reviewed changes

jrose-apple requested a review from gparker42 December 6, 2017 17:29

gparker42 reviewed Dec 6, 2017

View reviewed changes

Minor cleanups to Config and Casting.

14e4930

gparker42 reviewed Dec 6, 2017

View reviewed changes

troughton commented Dec 6, 2017

View reviewed changes

Use more descriptive variable names for BoxPair.

7c7ac6a

troughton mentioned this pull request Dec 7, 2017

[runtime] Always use SwiftCC #13311

Merged

troughton added 4 commits December 7, 2017 17:23

Update IRGen tests for SwiftCC.

18b8588

Update IRGen SIL tests.

ca38f02

Fix macOS issues with TypeNamePair.

215592c

Merge branch 'twowordpair-swiftcc-tests' into twowordpair-swiftcc

01ef8b2

Update usages of BoxPair to match renaming.

d33a0cb

gparker42 merged commit c5bf2ec into swiftlang:master Dec 8, 2017

troughton deleted the twowordpair-swiftcc branch December 8, 2017 03:29

gparker42 mentioned this pull request Jan 23, 2018

[runtime] Reinstate TwoWordPair hack for swiftcall returns. #14079

Merged

gparker42 mentioned this pull request Jan 24, 2018

[5.0][runtime] Reinstate TwoWordPair hack for swiftcall returns. #14129

Merged

gparker42 mentioned this pull request Jan 27, 2018

[IRGen][runtime] Simplify runtime CCs and entry point ABIs #14175

Merged

troughton restored the twowordpair-swiftcc branch January 31, 2018 20:55

troughton mentioned this pull request Feb 2, 2018

[runtime] Remove TwoWordPair and use the Swift calling convention instead. #14373

Merged

troughton deleted the twowordpair-swiftcc branch April 18, 2018 04:07

		@@ -122,7 +122,7 @@
		#define SWIFT_LLVM_CC(CC) SWIFT_LLVM_CC_##CC

		// Currently, RuntimeFunction.def uses the following calling conventions:

[runtime] Remove TwoWordPair and use the Swift calling convention instead. #13299

[runtime] Remove TwoWordPair and use the Swift calling convention instead. #13299

Uh oh!

Conversation

troughton commented Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

troughton commented Dec 6, 2017

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!

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!

gparker42 commented Dec 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

troughton Dec 6, 2017 • 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!

gparker42 Dec 6, 2017 • 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!

troughton Dec 7, 2017 • 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!

troughton commented Dec 7, 2017

Uh oh!

troughton commented Dec 7, 2017

Uh oh!

gparker42 commented Dec 7, 2017

Uh oh!

swift-ci commented Dec 7, 2017

Uh oh!

swift-ci commented Dec 7, 2017

Uh oh!

troughton commented Dec 7, 2017

Uh oh!

gparker42 commented Dec 8, 2017

Uh oh!

swift-ci commented Dec 8, 2017

Uh oh!

swift-ci commented Dec 8, 2017

Uh oh!

troughton commented Dec 8, 2017

Uh oh!

gparker42 commented Dec 8, 2017

Uh oh!

gparker42 commented Jan 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gparker42 commented Jan 30, 2018

Uh oh!

troughton commented Jan 31, 2018

Uh oh!

Uh oh!

troughton commented Dec 6, 2017 •

edited

Loading

troughton Dec 6, 2017 •

edited

Loading

gparker42 Dec 6, 2017 •

edited

Loading

troughton Dec 7, 2017 •

edited

Loading

gparker42 commented Jan 23, 2018 •

edited

Loading