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.

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

C++11 atomics and other attempts at portability #150

Merged
merged 17 commits into from
Aug 7, 2015
Merged

Conversation

jeffhammond
Copy link
Member

I am well aware that I can commit directly, but I want someone to review these commits for correctness and make sure they run through Jenkins.

@jeffhammond
Copy link
Member Author

@justusc Is there any way that you can review this and merge if appropriate? I recall seeing on the Wiki that you are the maintainer for the parallel runtime.

I have no urgent need for this but it would be nice to close this issue now while it is still in cache for both of us :-)

@justusc
Copy link
Member

justusc commented Aug 4, 2015

The code and CI tests look OK. The only thing that makes me nervous about the commit is that it does not check specific compiler versions. GCC 4.8, Intel 13, and Clang 3.3 are the minimum required versions (I think). This avoid problems with GCC 4.7 on BG/Q.

@naromero77
Copy link
Contributor

naromero77 commented Aug 4, 2015

Please do not make it harder to compile MADNESS on BG/Q. We may have newer
version of GCC on BG/Q in the near future, but this will be if William
Scullin gets it working.

@robertjharrison
Copy link
Contributor

robertjharrison commented Aug 4, 2015

I assume this all works with a current version of LLVM/clang? I am asking
for Power8/9 compatibility ... XLC is also of interest but as long as clang
works we should be OK

http://llvm.org/devmtg/2014-04/PDFs/Talks/Euro-LLVM-2014-Weigand.pdf

@robertjharrison
Copy link
Contributor

robertjharrison commented Aug 4, 2015

I agree with Nick that we need to make sure things keep working on the BG.

Also, rather than checking for compiler versions should we not be checking
for features using the usual autoconf scripts ... this seems to be less
error prone and is consistent with how we are managing other functionality.

@jeffhammond
Copy link
Member Author

@justusc Thanks. I will add the specific compiler version checks to which you allude. I know how to do that. Thanks for the reminder.

@naromero77 I will add a check so that this code is never used on Blue Gene/Q. Note that this is already covered if I test for GCC 4.8 but I will be explicitly with !defined(__bgq__) to be sure.

@robertjharrison Because C++11 compiler features are known to be buggy, it is not sufficient to do compilation-only configure testing. I will write the preprocessor solution. Someone else is welcome to create an configure solution to replace it.

@robertjharrison Unfortunately, my POWER9 order has not yet been fulfilled :-) As for POWER8, I don't know, but I don't see why C++11 atomics should be any worse than GCC atomics. I have some knowledge of TBB on POWER8 little-endian that may be relevant but I prefer to discuss it offline (because I am not sure about the cause of what is observed and do not want to falsely pin the blame on IBM).

justusc added a commit that referenced this pull request Aug 7, 2015
C++11 atomics and other attempts at portability
@justusc justusc merged commit 208dfa6 into master Aug 7, 2015
@justusc justusc deleted the generic-arch branch January 3, 2016 18:23
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.

5 participants