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

Adapt datapar generate #5594

Merged
merged 11 commits into from
Nov 8, 2021
Merged

Conversation

srinivasyadav18
Copy link
Contributor

Fixes #2333

Proposed Changes

  • Moves sequential generate to detail
  • Adapts datapar generate
  • Splits existing units tests for generate into header and source
  • Adapts unit tests for datapar generate

hkaiser
hkaiser previously approved these changes Oct 7, 2021
Copy link
Member

@hkaiser hkaiser left a comment

Choose a reason for hiding this comment

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

Otherwise, LGTM, thanks!

@srinivasyadav18
Copy link
Contributor Author

Please don't merge the PR. I am still working on it.
Need to fix this https://github.com/STEllAR-GROUP/hpx/pull/5594/checks?check_run_id=3803128093.
And add static assert in datapar generate and update tests.

@srinivasyadav18 srinivasyadav18 marked this pull request as draft October 13, 2021 16:37
@hkaiser
Copy link
Member

hkaiser commented Oct 31, 2021

We've had some discussion on how to resolve the problem that the user supplied function (object) has to be generic (i.e. callable for a single value and a vector register) without expecting an argument that could be used to deduce the required return type. We have two possible solutions

  • always call the vectorized version of the function object and manually take apart the returned value if needed (during execution of prefix/postfix loops).
  • rely on specifying the required template type explicitly

The former solution is non-conforming, however, as the generator function might be called more often than expected. The latter solution requires for the user to supply a function (object) that was written in a specific way, for instance:

// either provide a function object with a templated `operator()()`
struct f
{
    template <typename T>
    T operator()() const
    {
        return T();
    }
};

// or a lambda with an explicit template parameter (note: C++20)
auto l = []<typename T>() -> T { return T(); };

int main() {

    f foo;

    foo.operator()<int>();
    l.operator()<int>();
    
    return 0;
}

Also, for the latter, global functions are not usable.

While the second solution is still preferable (it is conforming), I'm uncertain if the limitations imposed on the function object are too restrictive or if the solution is acceptable in terms of usability.

@msimberg
Copy link
Contributor

msimberg commented Nov 1, 2021

While the second solution is still preferable (it is conforming), I'm uncertain if the limitations imposed on the function object are too restrictive or if the solution is acceptable in terms of usability.

I also think the second option is preferable. One can always wrap a global function in a lambda/struct if needed.

@srinivasyadav18
Copy link
Contributor Author

srinivasyadav18 commented Nov 1, 2021

We've had some discussion on how to resolve the problem that the user supplied function (object) has to be generic (i.e. callable for a single value and a vector register) without expecting an argument that could be used to deduce the required return type. We have two possible solutions

  • always call the vectorized version of the function object and manually take apart the returned value if needed (during execution of prefix/postfix loops).
  • rely on specifying the required template type explicitly

The former solution is non-conforming, however, as the generator function might be called more often than expected. The latter solution requires for the user to supply a function (object) that was written in a specific way, for instance:

// either provide a function object with a templated `operator()()`
struct f
{
    template <typename T>
    T operator()() const
    {
        return T();
    }
};

// or a lambda with an explicit template parameter (note: C++20)
auto l = []<typename T>() -> T { return T(); };

int main() {

    f foo;

    foo.operator()<int>();
    l.operator()<int>();
    
    return 0;
}

Also, for the latter, global functions are not usable.

While the second solution is still preferable (it is conforming), I'm uncertain if the limitations imposed on the function object are too restrictive or if the solution is acceptable in terms of usability.

Oops!, Did not read this sorry!, I think i pushed the changes for first method. I will look into second option and will try to implement it.

Apply clang-format
Fix CPO for sequential generate
Replace int with std::size_t in Vector pack for generate tests
Include missing headers
@msimberg msimberg added split: docs PR targets documentation split: local PR targets local functionality labels Nov 3, 2021
srinivasyadav18 added 2 commits November 4, 2021 11:57
Remove sequential tests
Replace generate test function with functor
aurianer
aurianer previously approved these changes Nov 4, 2021
Copy link
Contributor

@aurianer aurianer left a comment

Choose a reason for hiding this comment

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

LGTM thanks a lot!

@hkaiser
Copy link
Member

hkaiser commented Nov 6, 2021

@srinivasyadav18 which of the builders are actually running the vectorized tests?

Copy link
Member

@hkaiser hkaiser left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@srinivasyadav18
Copy link
Contributor Author

srinivasyadav18 commented Nov 6, 2021

g the vectorized t

gcc-11-debug and gcc-11-release

@hkaiser
Copy link
Member

hkaiser commented Nov 6, 2021

g the vectorized t

gcc-11-debug and gcc-11-release

Thanks! I'm wondering how the test can actually pass on that platform now. The tests use a non-templated lambda (e.g. https://github.com/STEllAR-GROUP/hpx/pull/5594/files#diff-94177c5266a2c92106d9f6e2640962f9bf6289153a24edee615122d7b10dbc4aR35), but the code is invoking the templated operator() (e.g. https://github.com/STEllAR-GROUP/hpx/pull/5594/files#diff-c8d8a6871e54681264b783160ea771babc0a46e669dafa47c5419841dda7a09bR49). Are you sure the tests are running?

@srinivasyadav18
Copy link
Contributor Author

g the vectorized t

gcc-11-debug and gcc-11-release

Thanks! I'm wondering how the test can actually pass on that platform now. The tests use a non-templated lambda (e.g. https://github.com/STEllAR-GROUP/hpx/pull/5594/files#diff-94177c5266a2c92106d9f6e2640962f9bf6289153a24edee615122d7b10dbc4aR35), but the code is invoking the templated operator() (e.g. https://github.com/STEllAR-GROUP/hpx/pull/5594/files#diff-c8d8a6871e54681264b783160ea771babc0a46e669dafa47c5419841dda7a09bR49). Are you sure the tests are running?

I updated with the changes to test files in this 12a2853 commit.

@hkaiser
Copy link
Member

hkaiser commented Nov 6, 2021

g the vectorized t

gcc-11-debug and gcc-11-release

Thanks! I'm wondering how the test can actually pass on that platform now. The tests use a non-templated lambda (e.g. https://github.com/STEllAR-GROUP/hpx/pull/5594/files#diff-94177c5266a2c92106d9f6e2640962f9bf6289153a24edee615122d7b10dbc4aR35), but the code is invoking the templated operator() (e.g. https://github.com/STEllAR-GROUP/hpx/pull/5594/files#diff-c8d8a6871e54681264b783160ea771babc0a46e669dafa47c5419841dda7a09bR49). Are you sure the tests are running?

I updated with the changes to test files in this 12a2853 commit.

Hmmm. From what I can see those changes are not on the PR, are they?

@srinivasyadav18
Copy link
Contributor Author

g the vectorized t

gcc-11-debug and gcc-11-release

Thanks! I'm wondering how the test can actually pass on that platform now. The tests use a non-templated lambda (e.g. https://github.com/STEllAR-GROUP/hpx/pull/5594/files#diff-94177c5266a2c92106d9f6e2640962f9bf6289153a24edee615122d7b10dbc4aR35), but the code is invoking the templated operator() (e.g. https://github.com/STEllAR-GROUP/hpx/pull/5594/files#diff-c8d8a6871e54681264b783160ea771babc0a46e669dafa47c5419841dda7a09bR49). Are you sure the tests are running?

I updated with the changes to test files in this 12a2853 commit.

Hmmm. From what I can see those changes are not on the PR, are they?

The changes are present.
Previously there was only test source files generate.cpp and generate_n.cpp in tests/unit/algorithms.
I had split these 2 .cpp source files into .hpp and .cpp i.e generate.cpp and generate.hpp in bbd2ebc.
Later in this 12a2853 commit I created seperate generate.cpp and generate.hpp in tests/unit/datapar_algorithms because we need seperate implementations for datapar tests.

You can find the templated functions for datapar tests in then end of commit differences from all commits i.e in tests/unit/datapar_algorithms/.

@hkaiser
Copy link
Member

hkaiser commented Nov 7, 2021

Let's get this merged, then. Thanks!

@msimberg
Copy link
Contributor

msimberg commented Nov 8, 2021

g the vectorized t

gcc-11-debug and gcc-11-release

In principle also CircleCI (build-and-test). However, for some reason which I still don't understand that never runs for @srinivasyadav18... It runs for other forked repositories, so it's not that. @srinivasyadav18 have you done something to upset CircleCI? ;)

@msimberg msimberg merged commit eb83655 into STEllAR-GROUP:master Nov 8, 2021
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.

Implement datapar for parallel algorithms
4 participants