Skip to content

Fix compile error on s390x in shims/lock.h #235

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.

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

Conversation

vivkong
Copy link
Contributor

@vivkong vivkong commented Apr 3, 2017

s390x has a strong memory model and does not require membarrier.

This is the same as #234 but for the 3.1 branch

Copy link
Contributor

@MadCoder MadCoder left a comment

Choose a reason for hiding this comment

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

How strong is s390x ? Similar to intel? The crux is that load have to all have an acquire barrier by default to be allowed to do that'

@vivkong
Copy link
Contributor Author

vivkong commented Apr 3, 2017

Yes it's comparable to Intel (see Table 5 in http://www.rdrop.com/~paulmck/scalability/paper/whymb.2009.04.05a.pdf).

src/shims/lock.h Outdated
// On Intel, any load is a load-acquire, so we don't need to be fancy
// s390x has a strong memory model
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd comment "same for s390x"

Do we want to throw in s390 too here? Or is that too obsolete as an architecture?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ya I think we only have s390x in Swift so I'd like to stick with that. Thanks for the review!

@vivkong vivkong force-pushed the fix_compiler_error_swift31 branch from eef18e6 to 86d036d Compare April 3, 2017 18:43
@MadCoder MadCoder merged commit db6a8f6 into swiftlang:swift-3.1-branch Apr 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants