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

Use C++20 function std::popcount #4389

Closed
wants to merge 6 commits into from

Conversation

a-noni-mousse
Copy link
Contributor

High Level Overview of Change

Remove the custom code and special compiler functions and use C++20 function std::popcount and make inline for optimization of the code. The changed code is good when compiled and produces same or even better assembly code which you can see on the compiler explorer

Context of Change

Type of Change

  • Refactor (non-breaking change that only restructures code)

Copy link
Contributor

@nbougalis nbougalis left a comment

Choose a reason for hiding this comment

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

Looks OK, modulo the MacOS compilation issues, and I like that it uses std instead of intrinsics or hand-rolled code. I think someone with MacOS can check if newer versions of XCode support std::popcount and if so, whether it makes sense to upgrade the CI.

One option might be to leave the popcnt16 function in place for now (perhaps inlining it in the header file?) as a thin wrapper around std::popcount for non MacOS platforms, and a call to __builtin_popcount for MacOS.

@a-noni-mousse
Copy link
Contributor Author

OK i can makethe change

@intelliot intelliot added this to the 1.10.1 milestone Mar 1, 2023
@intelliot
Copy link
Collaborator

CI has reported failures; please review them.

Copy link
Contributor

@HowardHinnant HowardHinnant left a comment

Choose a reason for hiding this comment

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

This change is required to compile on macOS: HowardHinnant@ab5e386

Explanation: The expression isBranch_ | (1 << m) promotes to int which is then sent to std::popcount. But popcount has a constraint that its argument be an unsigned integral type. Adding the u suffix makes the expression unsigned.

@intelliot
Copy link
Collaborator

@a-noni-mousse please review the comments above.

@a-noni-mousse a-noni-mousse force-pushed the patch18 branch 2 times, most recently from e03b62a to 23b140d Compare May 5, 2023 05:46
@a-noni-mousse
Copy link
Contributor Author

I have fixed the comments from Hooward and Nic.

@intelliot
Copy link
Collaborator

@a-noni-mousse for future reference - there's no need to force push, as this PR will be squashed when it is merged.

Keeping historical commits may help reviewers to more easily see how the PR has changed over time.

Copy link
Contributor

@nbougalis nbougalis left a comment

Choose a reason for hiding this comment

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

Looks fine.

@intelliot intelliot modified the milestones: 1.11.0, 1.12 May 19, 2023
Copy link
Contributor

@HowardHinnant HowardHinnant left a comment

Choose a reason for hiding this comment

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

I'm getting several errors on a non-unity build that look like this:

In file included from /Users/howardhinnant/Development/rippled/src/ripple/app/consensus/RCLConsensus.cpp:20:
In file included from /Users/howardhinnant/Development/rippled/src/ripple/app/consensus/RCLConsensus.h:23:
In file included from /Users/howardhinnant/Development/rippled/src/ripple/app/consensus/RCLCensorshipDetector.h:24:
In file included from /Users/howardhinnant/Development/rippled/src/ripple/shamap/SHAMap.h:30:
/Users/howardhinnant/Development/rippled/src/ripple/shamap/SHAMapInnerNode.h:213:12: error: use of undeclared identifier 'popcnt16'
    return popcnt16(isBranch_);
           ^

Not positive, but I suspect we're looking at a cyclic include dependency which is preventing the declaration of popcnt16 appearing before its use.

@a-noni-mousse
Copy link
Contributor Author

Ah I edit the code on the github and that's what I get! Thank you i will fix this @HowardHinnant

@intelliot
Copy link
Collaborator

Suggested commit message for squashed commit:

refactor: use C++20 function std::popcount (#4389)

- Replace custom popcnt16 implementation with std::popcount from C++20
- Maintain compatibility with older compilers and MacOS by providing a
  conditional compilation fallback to __builtin_popcount and a lookup
  table method
- Move and inline related functions within SHAMapInnerNode for
  performance and readability

@manojsdoshi - be sure to squash this PR when merging, as requested above.

@intelliot intelliot added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Aug 16, 2023
@intelliot intelliot removed this from the 1.13 milestone Aug 16, 2023
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
- Replace custom popcnt16 implementation with std::popcount from C++20
- Maintain compatibility with older compilers and MacOS by providing a
  conditional compilation fallback to __builtin_popcount and a lookup
  table method
- Move and inline related functions within SHAMapInnerNode for
  performance and readability
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
- Replace custom popcnt16 implementation with std::popcount from C++20
- Maintain compatibility with older compilers and MacOS by providing a
  conditional compilation fallback to __builtin_popcount and a lookup
  table method
- Move and inline related functions within SHAMapInnerNode for
  performance and readability
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
- Replace custom popcnt16 implementation with std::popcount from C++20
- Maintain compatibility with older compilers and MacOS by providing a
  conditional compilation fallback to __builtin_popcount and a lookup
  table method
- Move and inline related functions within SHAMapInnerNode for
  performance and readability
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
- Replace custom popcnt16 implementation with std::popcount from C++20
- Maintain compatibility with older compilers and MacOS by providing a
  conditional compilation fallback to __builtin_popcount and a lookup
  table method
- Move and inline related functions within SHAMapInnerNode for
  performance and readability
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
- Replace custom popcnt16 implementation with std::popcount from C++20
- Maintain compatibility with older compilers and MacOS by providing a
  conditional compilation fallback to __builtin_popcount and a lookup
  table method
- Move and inline related functions within SHAMapInnerNode for
  performance and readability
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
- Replace custom popcnt16 implementation with std::popcount from C++20
- Maintain compatibility with older compilers and MacOS by providing a
  conditional compilation fallback to __builtin_popcount and a lookup
  table method
- Move and inline related functions within SHAMapInnerNode for
  performance and readability

Signed-off-by: Manoj Doshi <mdoshi@ripple.com>
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
- Replace custom popcnt16 implementation with std::popcount from C++20
- Maintain compatibility with older compilers and MacOS by providing a
  conditional compilation fallback to __builtin_popcount and a lookup
  table method
- Move and inline related functions within SHAMapInnerNode for
  performance and readability

Signed-off-by: Manoj Doshi <mdoshi@ripple.com>
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
- Replace custom popcnt16 implementation with std::popcount from C++20
- Maintain compatibility with older compilers and MacOS by providing a
  conditional compilation fallback to __builtin_popcount and a lookup
  table method
- Move and inline related functions within SHAMapInnerNode for
  performance and readability

Signed-off-by: Manoj Doshi <mdoshi@ripple.com>
This was referenced Aug 18, 2023
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 19, 2023
- Replace custom popcnt16 implementation with std::popcount from C++20
- Maintain compatibility with older compilers and MacOS by providing a
  conditional compilation fallback to __builtin_popcount and a lookup
  table method
- Move and inline related functions within SHAMapInnerNode for
  performance and readability
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 19, 2023
- Replace custom popcnt16 implementation with std::popcount from C++20
- Maintain compatibility with older compilers and MacOS by providing a
  conditional compilation fallback to __builtin_popcount and a lookup
  table method
- Move and inline related functions within SHAMapInnerNode for
  performance and readability
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 19, 2023
- Replace custom popcnt16 implementation with std::popcount from C++20
- Maintain compatibility with older compilers and MacOS by providing a
  conditional compilation fallback to __builtin_popcount and a lookup
  table method
- Move and inline related functions within SHAMapInnerNode for
  performance and readability

Signed-off-by: Manoj Doshi <mdoshi@ripple.com>
@manojsdoshi manojsdoshi mentioned this pull request Aug 19, 2023
ximinez pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 19, 2023
- Replace custom popcnt16 implementation with std::popcount from C++20
- Maintain compatibility with older compilers and MacOS by providing a
  conditional compilation fallback to __builtin_popcount and a lookup
  table method
- Move and inline related functions within SHAMapInnerNode for
  performance and readability

Signed-off-by: Manoj Doshi <mdoshi@ripple.com>
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 21, 2023
- Replace custom popcnt16 implementation with std::popcount from C++20
- Maintain compatibility with older compilers and MacOS by providing a
  conditional compilation fallback to __builtin_popcount and a lookup
  table method
- Move and inline related functions within SHAMapInnerNode for
  performance and readability

Signed-off-by: Manoj Doshi <mdoshi@ripple.com>
@intelliot intelliot added this to the 1.12 milestone Sep 8, 2023
ckeshava pushed a commit to ckeshava/rippled that referenced this pull request Sep 22, 2023
- Replace custom popcnt16 implementation with std::popcount from C++20
- Maintain compatibility with older compilers and MacOS by providing a
  conditional compilation fallback to __builtin_popcount and a lookup
  table method
- Move and inline related functions within SHAMapInnerNode for
  performance and readability

Signed-off-by: Manoj Doshi <mdoshi@ripple.com>
ckeshava pushed a commit to ckeshava/rippled that referenced this pull request Sep 25, 2023
- Replace custom popcnt16 implementation with std::popcount from C++20
- Maintain compatibility with older compilers and MacOS by providing a
  conditional compilation fallback to __builtin_popcount and a lookup
  table method
- Move and inline related functions within SHAMapInnerNode for
  performance and readability

Signed-off-by: Manoj Doshi <mdoshi@ripple.com>
@intelliot intelliot added Perf Test Desired (Optional) RippleX Perf Team should look at this PR. The PR will not necessarily wait for testing to finish and removed Performance/Resource Improvement labels Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. Perf Test Desired (Optional) RippleX Perf Team should look at this PR. The PR will not necessarily wait for testing to finish Tech Debt Non-urgent improvements
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants