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

Update GCC version and move to C++20 #295

Merged
merged 42 commits into from
May 31, 2022
Merged

Update GCC version and move to C++20 #295

merged 42 commits into from
May 31, 2022

Conversation

chaeyeunpark
Copy link
Contributor

@chaeyeunpark chaeyeunpark commented May 19, 2022

Before submitting

Please complete the following checklist when submitting a PR:

  • All new features must include a unit test.
    If you've fixed a bug or added code that should be tested, add a test to the
    tests directory!

  • All new functions and code must be clearly commented and documented.
    If you do make documentation changes, make sure that the docs build and
    render correctly by running make docs.

  • Ensure that the test suite passes, by running make test.

  • Add a new entry to the .github/CHANGELOG.md file, summarizing the
    change, and including a link back to the PR.

  • Ensure that code is properly formatted by running make format.

When all the above are checked, delete everything above the dashed
line and fill in the pull request template.


Context:

Description of the Change:

Benefits:

Possible Drawbacks:

Related GitHub Issues:

@github-actions
Copy link
Contributor

Hello. You may have forgotten to update the changelog!
Please edit .github/CHANGELOG.md with:

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

@github-actions
Copy link
Contributor

github-actions bot commented May 19, 2022

Test Report (C++) on Ubuntu

           1 files  ±    0             1 suites  ±0   1s ⏱️ ±0s
       862 tests  -     3         862 ✔️  -     3  0 💤 ±0  0 ±0 
228 524 runs   - 239  228 524 ✔️  - 239  0 💤 ±0  0 ±0 

Results for commit 85f1420. ± Comparison against base commit 6e9cb1c.

♻️ This comment has been updated with latest results.

@codecov
Copy link

codecov bot commented May 19, 2022

Codecov Report

Merging #295 (85f1420) into master (6e9cb1c) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #295      +/-   ##
==========================================
- Coverage   99.44%   99.42%   -0.02%     
==========================================
  Files          30       30              
  Lines        3410     3300     -110     
==========================================
- Hits         3391     3281     -110     
  Misses         19       19              
Impacted Files Coverage Δ
...ennylane_lightning/src/gates/DynamicDispatcher.cpp 100.00% <ø> (ø)
pennylane_lightning/src/gates/GateUtil.cpp 100.00% <ø> (ø)
...nylane_lightning/src/simulator/StateVectorBase.hpp 100.00% <ø> (ø)
pennylane_lightning/_version.py 100.00% <100.00%> (ø)
...ennylane_lightning/src/algorithms/JacobianProd.hpp 100.00% <100.00%> (ø)
...ng/src/gates/cpu_kernels/GateImplementationsLM.hpp 100.00% <100.00%> (ø)
...ng/src/gates/cpu_kernels/GateImplementationsPI.hpp 100.00% <100.00%> (ø)
pennylane_lightning/src/util/BitUtil.hpp 100.00% <100.00%> (ø)
pennylane_lightning/src/util/ConstantUtil.hpp 100.00% <100.00%> (ø)
pennylane_lightning/src/util/Util.hpp 100.00% <100.00%> (ø)
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e9cb1c...85f1420. Read the comment docs.

@chaeyeunpark chaeyeunpark changed the title [WIP] Change GCC version in Linux CIs [WIP] Update GCC version and add experimental C++20 May 19, 2022
@chaeyeunpark chaeyeunpark changed the title [WIP] Update GCC version and add experimental C++20 [WIP] Update GCC version and move to C++20 May 19, 2022
@chaeyeunpark
Copy link
Contributor Author

Moving to GCC10 is smooth (but going to 11 must be harder due to manylinux2014 image used by cibuildwheel). As GCC10 already supports lots of new C++20 features, this PR also tests some of them (bit utility functions such as std::popcount and some more constexpr functions) and it works (even with appleclang and MSVC).

@chaeyeunpark chaeyeunpark marked this pull request as ready for review May 20, 2022 01:09
@chaeyeunpark chaeyeunpark changed the title [WIP] Update GCC version and move to C++20 Update GCC version and move to C++20 May 20, 2022
@chaeyeunpark chaeyeunpark requested a review from maliasadi May 20, 2022 01:15
@chaeyeunpark chaeyeunpark requested a review from mlxd May 20, 2022 01:24
Copy link
Member

@mlxd mlxd left a comment

Choose a reason for hiding this comment

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

Thanks for this @chaeyeunpark
I have a few questions/comments.

.github/workflows/benchmarks.yml Outdated Show resolved Hide resolved
.github/workflows/format.yml Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
pennylane_lightning/src/util/BitUtil.hpp Show resolved Hide resolved
pennylane_lightning/src/util/Util.hpp Show resolved Hide resolved
Copy link
Member

@maliasadi maliasadi left a comment

Choose a reason for hiding this comment

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

Awesome work @chaeyeunpark! Are you going to commit these changes (move to C++20 and gcc-10) for the coming release?

.github/workflows/benchmarks.yml Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
@@ -66,11 +64,12 @@ endif()
################################################################################

add_executable(compile_time_tests compile_time_tests.cpp)
target_link_libraries(compile_time_tests lightning_gates lightning_utils lightning_simulator)
target_link_libraries(compile_time_tests lightning_compile_options lightning_gates lightning_utils lightning_simulator)

set(TEST_SOURCES CreateAllWires.cpp
Copy link
Member

Choose a reason for hiding this comment

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

You may also update check_kernels_are_available in TestAvailableKernels.hpp.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch. Thanks!

pennylane_lightning/src/tests/CMakeLists.txt Show resolved Hide resolved
pennylane_lightning/src/tests/Test_Util.cpp Outdated Show resolved Hide resolved
pennylane_lightning/src/tests/Test_Util.cpp Outdated Show resolved Hide resolved
@chaeyeunpark chaeyeunpark requested review from maliasadi and mlxd May 30, 2022 15:05
Copy link
Member

@mlxd mlxd left a comment

Choose a reason for hiding this comment

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

Awesome work @chaeyeunpark
I've nothing to add. I'm happy to approve.

@@ -310,7 +312,7 @@ jobs:
- name: Install lightning.qubit device
run: |
cd main
python setup.py build_ext --define="ENABLE_KOKKOS=ON"
python setup.py build_ext -i --define="ENABLE_KOKKOS=ON;CMAKE_CXX_COMPILER=$(which g++-$GCC_VERSION)"
Copy link
Member

Choose a reason for hiding this comment

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

Nice, I didn't know we could cascade them like this!

CMakeLists.txt Show resolved Hide resolved
# Check GCC version
if ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU")
if (CMAKE_CXX_COMPILER_VERSION VERSION_LESS 10.0)
message(FATAL_ERROR "GCC version must be at least 10.0")
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

@maliasadi maliasadi left a comment

Choose a reason for hiding this comment

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

Great work @chaeyeunpark! Happy to approve.

@chaeyeunpark
Copy link
Contributor Author

Thanks @mlxd and @maliasadi! I am still sure that there are some more parts in the codebase that need to be converted into C++20, which may be addressed in subsequent PRs.

@chaeyeunpark chaeyeunpark merged commit a113c30 into master May 31, 2022
@chaeyeunpark chaeyeunpark deleted the change_gcc_ver branch May 31, 2022 17:59
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.

3 participants