Skip to content

Solve some of the fallout of new refcount representation on Windows build #7762

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
Merged
merged 1 commit into from
Mar 13, 2017
Merged

Solve some of the fallout of new refcount representation on Windows build #7762

merged 1 commit into from
Mar 13, 2017

Conversation

hughbe
Copy link
Contributor

@hughbe hughbe commented Feb 25, 2017 •
edited
Loading

The main meat of this PR:

The implementation of atomic<T> on Windows ,MSVC (with both visual c++ and clang compiler), requires the following


template<class _Ty>
 struct atomic
  : _Atomic_base<_Ty, sizeof (_Ty)>
 { // template that manages values of _Ty atomically
 static_assert(is_trivially_copyable<_Ty>::value,
  "atomic<T> requires T to be trivially copyable.");
}

I'm currently getting a failure here on Windows, possibly due to this line here (RefCount.h, L434):

  RefCountBitsT(RefCountBitsT<RefCountIsInline> newbits) {}

I'm fairly sure the Visual C++ compiler thinks this is a copy constructor, so fails the check.

@gparker42

@hughbe hughbe changed the title Solve some (and investigate) of the effect of new refcount representation on Windows build Solve some (and investigate) of the fallout of new refcount representation on Windows build Feb 25, 2017
jckarter
jckarter reviewed Feb 25, 2017
View reviewed changes
Copy link
Contributor

@jckarter jckarter left a comment

Choose a reason for hiding this comment

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

We've switched over to the Swift calling convention, so you're going to need to build the runtime with clang. Not sure this has any benefit.

@hughbe
Copy link
Contributor Author

hughbe commented Feb 25, 2017

I should clarify - this isn't about building the runtime with clang, this is about building the compiler with MSVC. The irgen module imports certain parts of the runtime such as refCount.h and Metadata.h so these files have to be msvc - compatible

@hughbe hughbe changed the title Solve some (and investigate) of the fallout of new refcount representation on Windows build Solve some of the fallout of new refcount representation on Windows build Feb 25, 2017
@jckarter
Copy link
Contributor

@swift-ci Please benchmark

compnerd
compnerd reviewed Feb 26, 2017
View reviewed changes
stdlib/public/SwiftShims/RefCount.h Outdated
@@ -432,7 +431,14 @@ class RefCountBitsT {
}

LLVM_ATTRIBUTE_ALWAYS_INLINE
RefCountBitsT(RefCountBitsT<RefCountIsInline> newbits) {
RefCountBitsT(int dummy, RefCountBitsT<RefCountIsInline> newbits) {
Copy link
Member

Choose a reason for hiding this comment

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

Does this work if you do something more clever like:

RefCOuntBitsT(RefCountBitsT<RefCountIsInline> newBits, int dummy = 0);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried this at first, and unfortunately it does not: MSVC get's confused, and starts complaining about ambiguous initializer lists - this happens anywhere where the dummy is the second parameter

Copy link
Contributor Author

@hughbe hughbe Feb 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

This is actually a Clang bug - GCC and MSVC correctly reject this code! The problem here is that we are actually declaring a copy constructor, at least for the situation where refcountIsInline==true.

We should definitely fix this, but in a nicer way.

http://bugs.llvm.org/show_bug.cgi?id=32074

Copy link
Contributor Author

Choose a reason for hiding this comment

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

template <bool refcountIsInline>
class RefCountBitsT {
    RefCountBitsT(RefCountBitsT<true> newbits) { }
};

class SideTableRefCountBits : public RefCountBitsT<false> {
    SideTableRefCountBits(RefCountBitsT<true> newbits)
        : RefCountBitsT<false>(newbits) {}
};

If anyone's got some suggestions that don't involve a dummy, let me know.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could write the inline to out-of-line conversion as a static function and remove this initializer / copy constructor. It might be used infrequently enough to be reasonable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i found it hard to convert this into a static function as it is called by a constructor in a sub class (that doesn't have access to private state)

I went for passing a pointer instead of the actual value. Let me know what you all think

stdlib/public/SwiftShims/RefCount.h Outdated
@@ -725,7 +731,13 @@ class RefCounts {
#if !__LP64__
// FIXME: hack - something somewhere is assuming a 3-word header on 32-bit
// See also other fixmes marked "small header for 32-bit"
uintptr_t unused __attribute__((unavailable));
#if __has_attribute(unavailable)
Copy link
Member

Choose a reason for hiding this comment

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

Can this be hoisted out to the header with all the other macros for the shims?

@hughbe
Copy link
Contributor Author

hughbe commented Mar 8, 2017

@swift-ci please smoke test

@hughbe
Copy link
Contributor Author

hughbe commented Mar 13, 2017 •
edited
Loading

Can I get a quick review :) This fix is cleaner than using a dummy and works with all compilers

jckarter
jckarter reviewed Mar 13, 2017
View reviewed changes
stdlib/public/SwiftShims/RefCount.h Outdated
@@ -433,10 +433,13 @@ class RefCountBitsT {
}

LLVM_ATTRIBUTE_ALWAYS_INLINE
RefCountBitsT(RefCountBitsT<RefCountIsInline> newbits) {
RefCountBitsT(RefCountBitsT<RefCountIsInline> *newbitsPtr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can it be a const RefCountBitsT<RefCountIsInline> *?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That seems to work, let's see what Clang says

@hughbe
Copy link
Contributor Author

hughbe commented Mar 13, 2017

@swift-ci please smoke test and merge

1 similar comment
@hughbe
Copy link
Contributor Author

hughbe commented Mar 13, 2017

@swift-ci please smoke test and merge

@swift-ci swift-ci merged commit 9581354 into swiftlang:master Mar 13, 2017
@hughbe hughbe deleted the refcnt-fixes branch March 13, 2017 17:39
@Endilll Endilll mentioned this pull request Jul 25, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@compnerd compnerd compnerd left review comments

@jckarter jckarter jckarter left review comments

@gparker42 gparker42 gparker42 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.
5 participants
@hughbe @jckarter @compnerd @gparker42 @swift-ci