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 aligned allocator definition for C++20 compiler supports #438

Merged
merged 19 commits into from
May 19, 2023

Conversation

mlxd
Copy link
Member

@mlxd mlxd commented Apr 18, 2023

Context: The allocator definition is incompatible with many C++20 compilers (MacOS on CF, G++ 13+). This PR updates the allocator and associated use to be compliant and pass clang-tidy validations.

Description of the Change: Update allocator definition and use.

Benefits: Fixes build issues on modern compilers.

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.

@codecov
Copy link

codecov bot commented Apr 18, 2023

Codecov Report

Merging #438 (4ef0598) into master (8c6657e) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #438   +/-   ##
=======================================
  Coverage   99.82%   99.82%           
=======================================
  Files          50       50           
  Lines        4667     4667           
=======================================
  Hits         4659     4659           
  Misses          8        8           
Impacted Files Coverage Δ
pennylane_lightning/src/util/Memory.hpp 100.00% <ø> (ø)
pennylane_lightning/_version.py 100.00% <100.00%> (ø)
pennylane_lightning/src/simulator/Observables.hpp 100.00% <100.00%> (ø)

@chaeyeunpark
Copy link
Contributor

Seems like the CI is currently using appleclang.

@mlxd
Copy link
Member Author

mlxd commented Apr 18, 2023

Seems like the CI is currently using appleclang.

I added a copy of the older CI https://github.com/PennyLaneAI/pennylane-lightning/actions/runs/4734291625/jobs/8403582700?pr=438
Still fails with the changes.

@chaeyeunpark
Copy link
Contributor

Do we want to use Appleclang? or brew installed newer clang-16?

@mlxd
Copy link
Member Author

mlxd commented Apr 19, 2023

Do we want to use Appleclang? or brew installed newer clang-16?

Ideally we should be able to build with both, but it is preferable to favour brew install clang-16 here, as it will also be comparable on the conda forge ecosystem. The older clang-16 build is checked in as a separate CI yaml file with the word alloc appended to differentiate from the current MacOS clang builder.

@chaeyeunpark
Copy link
Contributor

chaeyeunpark commented Apr 19, 2023

I don't think we can support applclang as it does not support most of the C++20 features we are using yet. Still, the CI is currently using appleclang. In https://github.com/PennyLaneAI/pennylane-lightning/actions/runs/4734291625/jobs/8403584114?pr=438, you can see

    -- The CXX compiler identification is AppleClang 13.0.0.13000029

@vincentmr
Copy link
Contributor

FYI, the alligned_alloc issue I had in my PLL-Kokkos feedstock PR was resolved by this patch. I am wondering whether we should refactor the setup.py files to make less assumptions about what is found and not in the environment, and pass what is needed through CMAKE_ARGS, say. Forcing compilers, libs, programs and versions in the Actions files could make it more obvious when some defaults change.

@vincentmr
Copy link
Contributor

Build issue seems fixed on Conda-Forge. Thanks guys!

@mlxd mlxd changed the title Update aligned allocator definition for MacOS support Update aligned allocator definition for C++20 compiler supports May 16, 2023
@mlxd mlxd marked this pull request as ready for review May 16, 2023 20:44
@mlxd mlxd requested review from vincentmr and AmintorDusko May 16, 2023 20:51
Copy link
Contributor

@vincentmr vincentmr 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 to me! I derived a patch from this that is already in prod on CF.

Copy link
Contributor

@AmintorDusko AmintorDusko left a comment

Choose a reason for hiding this comment

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

I have some comments and questions.

Copy link
Contributor

@AmintorDusko AmintorDusko left a comment

Choose a reason for hiding this comment

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

Thank you for the fix, and for addressing my questions.

@AmintorDusko
Copy link
Contributor

@mlxd, please remember to update the changelog.

@mlxd mlxd merged commit 008664b into master May 19, 2023
@mlxd mlxd deleted the allocator_redef branch May 19, 2023 19:16
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.

4 participants