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

Fix complex to not depend on C++; reimplement functions for client tests in terms of std::complex #193

Merged
merged 4 commits into from
May 4, 2020

Conversation

leekillough
Copy link
Contributor

@leekillough leekillough commented Apr 28, 2020

This supercedes PR# 191, which was not a full solution.

This only uses C in the hipblas.h header, unless __cplusplus is defined.

It re-implements the test functions in terms of std::complex, which has a guaranteed layout and hence reinterpret_cast<std::complex<float>&>(hipComplex) and reinterpret_cast<std::complex<double>&>(hipDoubleComplex) can be used to cast them into std::complex to use the functions in there (to avoid dependence on rocblas-complex-functions.h).

@TorreZuk
Copy link
Contributor

@amcamd @daineAMD Looks fine to me, not sure what failures are expected. So Andrew and Daine can you review today as on the list of blockers. Thanks

Copy link
Contributor

@daineAMD daineAMD left a comment

Choose a reason for hiding this comment

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

Looks good.

clients/common/cblas_interface.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@amcamd amcamd left a comment

Choose a reason for hiding this comment

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

LGTM as long as CI tests pass

@leekillough leekillough marked this pull request as ready for review May 3, 2020 06:01
@leekillough
Copy link
Contributor Author

It passes all tests now.

@leekillough
Copy link
Contributor Author

The original compilation errors after template... were caused by it not recognizing C++14 code. Variable templates are not allowed in C++11. This PR does not add C++14 code; C++14 code already exists in develop. I've tried changing the language level to C++14 several ways in CMakeLists.txt, but it doesn't always work. In my GFX908 test machine, this PR builds and passes all tests.

@leekillough
Copy link
Contributor Author

Something is different about the CI which needs investigating and fixing, which is causing those build failures. I can't reproduce them locally. The C++ code is fine; the errors, as with nearly all C++ template compilation errors, are misleading. It looks like the CI compiler is not picking up -std=c++14 even though it's specified in CMakeLists.txt.

To reproduce the failures on my own machine, I would need the right Docker container.

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