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.

Sign up for GitHub

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

Jump to bottom

Switch to swift calling conv #7237

Merged
merged 2 commits into from
Feb 14, 2017
Merged

Switch to swift calling conv #7237

merged 2 commits into from
Feb 14, 2017

Conversation

aschwaighofer
Copy link
Contributor

@aschwaighofer aschwaighofer commented Feb 3, 2017 •
edited by najacque
Loading

Switch over to using the swift calling convention for swift functions

  • Use the generic type lowering algorithm described in
    "docs/CallingConvention.rst#physical-lowering" to map from IRGen's explosion
    type to the type expected by the Swift ABI.

  • Change IRGen to use the swift calling convention (swiftcc) for native swift
    functions.
    Use the 'swiftself' attribute on self parameters and for closures contexts.
    Use the 'swifterror' parameter for swift error parameters.

  • Change functions in the runtime that are called as native swift functions to use
    the swift calling convention.

rdar://problem/19979055

@aschwaighofer
Copy link
Contributor Author

@swift-ci Please smoke test

@jrose-apple
Copy link
Contributor

I think this deserves a full test!

@aschwaighofer
Copy link
Contributor Author

@jrose-apple Totally agree :-)

@aschwaighofer
Copy link
Contributor Author

@swift-ci Please smoke test linux platform

@aschwaighofer
Copy link
Contributor Author

@jrose-apple There is some local testing that I want to do before this should go in. At this time I will at the latest also run full test.

@gottesmm
Copy link
Contributor

gottesmm commented Feb 4, 2017

In fact, I would even say a full clean test since you are making some cmake changes.

@aschwaighofer
Copy link
Contributor Author

@swift-ci Please smoke test linux platform

@aschwaighofer
Copy link
Contributor Author

@swift-ci Please smoke test

@aschwaighofer
Copy link
Contributor Author

@swift-ci Please smoke test os x platform

@aschwaighofer
Copy link
Contributor Author

@swift-ci Please smoke test linux

@aschwaighofer
Copy link
Contributor Author

@swift-ci Please smoke test os x platform

@aschwaighofer
Copy link
Contributor Author

@swift-ci Please smoke test linux platform

@aschwaighofer
Copy link
Contributor Author

@swift-ci Please clean test

@swift-ci
Copy link
Contributor

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 16c38985978b3e7da785880cf87d346f7a0e1e8d
Test requested by - @aschwaighofer

@swift-ci
Copy link
Contributor

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 16c38985978b3e7da785880cf87d346f7a0e1e8d
Test requested by - @aschwaighofer

@aschwaighofer
Copy link
Contributor Author

@swift-ci Please smoke test os x platform

@aschwaighofer
Copy link
Contributor Author

@swift-ci Please smoke test

@aschwaighofer
Use the swift calling convention for swift functions
39fa2f0 
Use the generic type lowering algorithm described in
"docs/CallingConvention.rst#physical-lowering" to map from IRGen's explosion
type to the type expected by the ABI.

Change IRGen to use the swift calling convention (swiftcc) for native swift
functions.

Use the 'swiftself' attribute on self parameters and for closures contexts.

Use the 'swifterror' parameter for swift error parameters.

Change functions in the runtime that are called as native swift functions to use
the swift calling convention.

rdar://19978563
@aschwaighofer
Conditionalize usage of llvm::CallingConv::Swift on SWIFT_USE_SWIFTCA…
6c35202 
…LL macro
@aschwaighofer
Copy link
Contributor Author

@swift-ci Please smoke test

This was referenced Feb 14, 2017
Closed
Closed
@aschwaighofer
Copy link
Contributor Author

@swift-ci Please smoke test linux platform

@aschwaighofer aschwaighofer merged commit ff4b055 into swiftlang:master Feb 14, 2017
@jrose-apple
Copy link
Contributor

Did this ever pass the full tests? I assume so because there were no comments after the Wrong SHA failures, but it'd be nice to know for sure.

@jrose-apple
Copy link
Contributor

Also, were there any major benchmark changes from this?

@aschwaighofer
Copy link
Contributor Author

Yes it passed the full test:

https://ci.swift.org/job/swift-PR-osx/5542/
https://ci.swift.org/job/swift-PR-Linux/5575/

Unfortunately, I had to rebase after that.

@aschwaighofer
Copy link
Contributor Author

I did not measure significant changes in local testing.

@jrose-apple
Copy link
Contributor

jrose-apple commented Feb 15, 2017 •
edited
Loading

Looks like this broke the optimized tests: https://ci.swift.org/job/oss-swift_tools-RA_stdlib-RD_test-simulator/2738/

******************** TEST 'Swift(macosx-x86_64) :: Sanitizers/tsan-ignores-arc-locks.swift' FAILED ********************
Script:
--
xcrun --toolchain default --sdk /Applications/Xcode-beta.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk /Users/buildnode/jenkins/workspace/oss-swift_tools-RA_stdlib-RD_test-simulator/Ninja-ReleaseAssert/swift-macosx-x86_64/bin/swiftc -target x86_64-apple-macosx10.9  -module-cache-path '/var/folders/_8/79jmzf2142z2xydc_01btlx00000gn/T/swift-testsuite-clang-module-cachepeMyEm' -F /Applications/Xcode-beta.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/../../../Developer/Library/Frameworks -Xlinker -rpath -Xlinker /Applications/Xcode-beta.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/../../../Developer/Library/Frameworks  -swift-version 3 -O -sanitize=thread /Users/buildnode/jenkins/workspace/oss-swift_tools-RA_stdlib-RD_test-simulator/swift/test/Sanitizers/tsan-ignores-arc-locks.swift -o /Users/buildnode/jenkins/workspace/oss-swift_tools-RA_stdlib-RD_test-simulator/Ninja-ReleaseAssert/swift-macosx-x86_64/test-macosx-x86_64/Sanitizers/Output/tsan-ignores-arc-locks.swift.tmp_binary
TSAN_OPTIONS=ignore_interceptors_accesses=1:halt_on_error=1 /Users/buildnode/jenkins/workspace/oss-swift_tools-RA_stdlib-RD_test-simulator/Ninja-ReleaseAssert/swift-macosx-x86_64/test-macosx-x86_64/Sanitizers/Output/tsan-ignores-arc-locks.swift.tmp_binary
--
Exit Code: 1

Command Output (stderr):
--
swifterror value can only be loaded and stored from, or as a swifterror argument!
%swift.error** %3
  %103 = bitcast %swift.error** %3 to i8*
LLVM ERROR: Broken function found, compilation aborted!

@aschwaighofer
Copy link
Contributor Author

Taking a look

@gottesmm
Copy link
Contributor

@shahmishal @jrose-apple I think the os x smoke tests are fast enough that we should consider running the optimized tests on OS X.

Thoughts?

@jrose-apple
Copy link
Contributor

Thoughts: "a PR is the wrong place to have this discussion". :-)

@jrose-apple
Copy link
Contributor

Arnold also ran a regular set of tests. Those don't do optimized either.

@gottesmm
Copy link
Contributor

@jrose-apple Sure. I was more just putting it out there before I send out an email

@aschwaighofer
Copy link
Contributor Author

:( I think the thread sanitizer inserts uses of the swifterror value. I will have to fix that.

@aschwaighofer
Copy link
Contributor Author

Upstream patch in the works ...

@shahmishal
Copy link
Member

@gottesmm #2476 I was trying to enable it for Linux, however we have few test failing.

@aschwaighofer
Copy link
Contributor Author

llvm/stable picked up the fix. Should be fixed as soon as the failing bot picks up the fix.

rudkx
rudkx reviewed Feb 23, 2017
View reviewed changes
lib/IRGen/GenFunc.cpp
// last parameter that fits into a register as swiftself.
// We should get this optimization back using the @convention(closure) whose
// box argument should just be swift self.
if (false &&
Copy link
Contributor

Choose a reason for hiding this comment

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

This is resulting in a build warning about unreachable code.

Can you (ideally) remove this code and open a bug to restore the behavior that you'd like to eventually see, or alternately follows he advice in the note we emit (to add parens around false to silence the warning).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#7702
https://bugs.swift.org/browse/SR-4041

@bob-wilson bob-wilson mentioned this pull request Mar 24, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rudkx rudkx rudkx left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@aschwaighofer @jrose-apple @gottesmm @swift-ci @shahmishal @rudkx