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.

Sign up for GitHub

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

Jump to bottom

Try using c++20 throughout catalyst #1229

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Try using c++20 throughout catalyst #1229

wants to merge 7 commits into from
+11 −8

Conversation

paul0403
Copy link
Contributor

Context:
Try using c++20 throughout catalyst

@paul0403 paul0403 added reviewer:require-wheels Pull Requests will need wheel building job successful before being merged author:build-wheels Run the wheel building workflows on this Pull Request labels Oct 24, 2024
@paul0403 paul0403 requested review from mlxd and a team October 24, 2024 20:54
@github-actions GitHub Actions
Copy link
Contributor

Hello. You may have forgotten to update the changelog!
Please edit doc/releases/changelog-dev.md on your branch 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.

@joeycarter joeycarter mentioned this pull request Oct 24, 2024
Merged
paul0403
paul0403 commented Oct 24, 2024
View reviewed changes
mlir/CMakeLists.txt
@@ -4,7 +4,8 @@ project(catalyst LANGUAGES CXX C)
set(CMAKE_BUILD_WITH_INSTALL_NAME_DIR ON)
set(CMAKE_EXPORT_COMPILE_COMMANDS ON)

set(CMAKE_CXX_STANDARD 17 CACHE STRING "C++ standard to conform to")
set(CMAKE_CXX_STANDARD 20)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

put back cache

paul0403
paul0403 commented Oct 24, 2024
View reviewed changes
mlir/lib/Quantum/Transforms/remove_chained_self_inverse.cpp
@@ -52,6 +53,8 @@ struct RemoveChainedSelfInversePass
LLVM_DEBUG(dbgs() << "remove chained self inverse pass"
<< "\n");

llvm::errs() << std::numbers::pi << "\n";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remember to remove this "unit test"!

@dime10
Copy link
Contributor

dime10 commented Nov 12, 2024

What is the motivation, and status, for this pr?

@joeycarter
Copy link
Contributor

What is the motivation, and status, for this pr?

This first came up because we had hard-coded in the value of pi somewhere and noticed that mathematical constants are now included in the <numbers> header as of C++20: https://en.cppreference.com/w/cpp/numeric/constants.

It turns out mathematical constants are already included in llvm::numbers, so we don't need C++20 for this purpose alone. However, updating to C++20 would open up some new, useful language features like std::format, Concepts, and many others.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mlxd mlxd Awaiting requested review from mlxd

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
author:build-wheels Run the wheel building workflows on this Pull Request reviewer:require-wheels Pull Requests will need wheel building job successful before being merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@paul0403 @dime10 @joeycarter