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

Add lockfree atomic uint128_type CAS for x86 #6521

Open
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

Dysrhythmic
Copy link

Fixes #

Proposed Changes

  • add a lockfree implementation of CAS methods for atomic 128-bit integers for x86
    • add a feature test for if 128 bit atomics are lockfree, and if so define HPX_HAVE_CXX11_STD_ATOMIC_128BIT_LOCKFREE
    • split off the uint128_type struct from tagged_ptr_pair.hpp into its own header file uint128_type.hpp for increased modularity
    • if HPX_HAVE_CXX11_STD_ATOMIC_128BIT_LOCKFREE isn't defined, use specialized methods in lockfree_uint128_type.hpp
  • add unit test hpx/libs/core/concurrency/tests/unit/lockfree_128uint.cpp
    • no errors upon running: ctest --output-on-failure -R tests.unit.modules.concurrency.lockfree_128uint
    • output from ./bin/lockfree_128uint_test indicates the implementation is working as expected

Any background context you want to provide?

Next steps will be:

  • add implementations for other architectures
  • use compiler directives to determine which implementation to use

Checklist

Not all points below apply to all pull requests.

  • I have added a new feature and have added tests to go along with it.
  • I have fixed a bug and have added a regression test.
  • I have added a test using random numbers; I have made sure it uses a seed, and that random numbers generated are valid inputs for the tests.

@jenkins-cscs
Copy link
Collaborator

Can one of the admins verify this patch?

Copy link

codacy-production bot commented Jul 18, 2024

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
-0.04% 88.10%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (f4ff6e8) 232431 198777 85.52%
Head commit (0833ee7) 201540 (-30891) 172287 (-26490) 85.49% (-0.04%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#6521) 42 37 88.10%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more

@hkaiser
Copy link
Member

hkaiser commented Jul 24, 2024

Thanks for working on this! There are some compilation and inspect/formatting problems reported by the CIs. Would you be able to address those?

@hkaiser
Copy link
Member

hkaiser commented Jul 31, 2024

The Windows CI's are unhappy for some reason (even the one using the Clang frontend). Could you please have a look?

@Dysrhythmic
Copy link
Author

The macOS CI tests seem to be failing after running cmake --build build --target all, with the error pointing to the files:
hpx/libs/core/concurrency/tests/unit/tagged_ptr.cpp
and
hpx/libs/core/testing/include/hpx/modules/testing.hpp
I did not alter either of these files (and it looks like it's been a long time since anyone has), so I am investigating how the changes I made could be affecting them. Neither file uses lockfree_uint128_type.hpp, uint128_type.hpp, or tagged_ptr_pair.hpp, which are the main changes I made.

Error for quick reference:

In file included from /Users/runner/work/hpx/hpx/libs/core/concurrency/tests/unit/tagged_ptr.cpp:9:
/Users/runner/work/hpx/hpx/libs/core/testing/include/hpx/modules/testing.hpp:81:25: error: comparison of integers of different signs: 'const unsigned long' and 'const int' [-Werror,-Wsign-compare]
                if (!(t == u))
                      ~ ^  ~
/Users/runner/work/hpx/hpx/libs/core/concurrency/tests/unit/tagged_ptr.cpp:28:9: note: in instantiation of function template specialization 'hpx::util::detail::fixture::check_equal<unsigned long, int>' requested here
        HPX_TEST_EQ(i.get_tag(), 1);
        ^
/Users/runner/work/hpx/hpx/libs/core/testing/include/hpx/modules/testing.hpp:226:5: note: expanded from macro 'HPX_TEST_EQ'
    HPX_TEST_EQ_(__VA_ARGS__)                                                  \
    ^
/Users/runner/work/hpx/hpx/libs/core/testing/include/hpx/modules/testing.hpp:231:9: note: expanded from macro 'HPX_TEST_EQ_'
        HPX_PP_CAT(HPX_TEST_EQ_, HPX_PP_NARGS(__VA_ARGS__))(__VA_ARGS__))      \
        ^
/Users/runner/work/hpx/hpx/libs/core/preprocessor/include/hpx/preprocessor/cat.hpp:28:26: note: expanded from macro 'HPX_PP_CAT'
#define HPX_PP_CAT(a, b) HPX_PP_CAT_I(a, b)
                         ^
note: (skipping 1 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all)
<scratch space>:232:1: note: expanded from here
HPX_TEST_EQ_2
^
/Users/runner/work/hpx/hpx/libs/core/testing/include/hpx/modules/testing.hpp:234:5: note: expanded from macro 'HPX_TEST_EQ_2'
    HPX_TEST_EQ_IMPL(::hpx::util::detail::global_fixture(), expr1, expr2)
    ^
/Users/runner/work/hpx/hpx/libs/core/testing/include/hpx/modules/testing.hpp:239:13: note: expanded from macro 'HPX_TEST_EQ_IMPL'
    fixture.check_equal(__FILE__, __LINE__, HPX_ASSERT_CURRENT_FUNCTION,       \
            ^
1 error generated.

I was able to run the same command locally on my Macbook and it completed without error. I currently have it running cmake --build build --target tests to see if that passes locally as well.

@hkaiser
Copy link
Member

hkaiser commented Aug 28, 2024

@Dysrhythmic I think the solution to the compilation problem would be to create a new type while derving from std::atomic:

namespace hpx::lockfree {

    struct uint128_atomic : std::atomic<uint128_type> {

        // implement your overloaded functions here
    };
}

@hkaiser
Copy link
Member

hkaiser commented Sep 25, 2024

@Dysrhythmic I still believe that the new uint128_atomic type -- while it is part of this PR -- is not used anywhere in the change set. Could you verify this, please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants