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

Fixes to avoid multiple definitions for thread pools and suppress compiler warnings. #1

Merged
merged 7 commits into from Jan 6, 2022

Conversation

cheshyre
Copy link

When using the ips4o parallel sort via normal Makefiles, I ran into multiple definition errors when compiling associated with the various *ThreadPool Impl structs. I inlined all the associated functions where the definition was separated from the declaration, which resolved the errors. This is the first commit in the PR.

In addition, I attempted to suppress most of the (presumably spurious) compiler warnings when including the ips4o headers. (I did this for -Wall -Wextra -Wpedantic, but I typically don't use pedantic so I don't actually care about the warnings associated with __int128.) The remaining warning, as far as I get them at least, is associated with classifier.h:131:

./lib/ips4o/include/ips4o/classifier.hpp:131:28: warning: suggest parentheses around ‘&&’ within ‘||’ [-Wparentheses]

I attempted to add parentheses and such to make this warning go away, but the compiler seems to be hung up on something here. I don't fully understand the control flow of this function, but if you can explain it to me or have some ideas regarding the warning, I would be happy to take another look at the warning and try and suppress it.

Thanks for your time and feedback!

using atomic_type = unsigned __int128;
// __extension__ suppresses the pedantic warning about __int128
// not being ISO C++ compliant
__extension__ using atomic_type = unsigned __int128;
Copy link

Choose a reason for hiding this comment

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

Unless I'm mistaken, you can also use __uint128_t to avoid the pedantic warning without having to use __extension__.

Copy link
Author

Choose a reason for hiding this comment

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

It seems that it does, and a quick Google suggests that it supported by at least Clang, GCC, and Intel's compiler. I don't know enough about compilers and support to say for certain that this is a safe replacement, but if it is then it might be preferable to __extension__.

Copy link

Choose a reason for hiding this comment

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

I'm unfortunately no more knowledgeable, I just know that the use of __int128_t and __uint128_t is somewhat widespread and have the advantage of being "normal" from the language POV, just another type which happens to have compiler support.

Copy link
Owner

@ips4o ips4o 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 your recommendation. However, I would prefer a cast to int, the type of log_buckets_.

Copy link
Owner

@ips4o ips4o left a comment

Choose a reason for hiding this comment

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

I would prefer to remove the unused function parameters.

@cheshyre
Copy link
Author

cheshyre commented Jan 4, 2022

I went through and adopted the suggestions:

  • The static cast to avoid signed-unsiged comparison is now to int instead of std::int64_t.
  • Unused parameters have been removed and the function calls have been adjusted. Similarly, unused values have been completely removed. (This should be tested by somebody that knows the code well because I do not know whether getting the shared pointer on line 480 of parallel.hpp had any important side effects.
    In my testing, the sort still works correctly in serial and parallel contexts, but I'm not sure if there is a more comprehensive test suite that could be run.

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.

None yet

3 participants