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

Trivial relocation of c-v-ref-array types #6313

Closed

Conversation

isidorostsa
Copy link
Contributor

This PR is an addition to the earlier #6264

It allows peeling away type qualifiers to make sure that declaring type T as trivially relocatable will declare the following types too:

const T, volatile T, const volatile T, T&, T&&, T[], T[n]

@jenkins-cscs
Copy link
Collaborator

Can one of the admins verify this patch?

@isidorostsa isidorostsa changed the title Trivial relocation of cvref-array types Trivial relocation of c-v-ref-array types Jul 27, 2023
hkaiser
hkaiser previously approved these changes Jul 27, 2023
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!

@isidorostsa
Copy link
Contributor Author

LGTM, thanks!

Thanks for reviewing, I am surprised there is not a simpler way to specialize all versions of a type at once

@hkaiser
Copy link
Member

hkaiser commented Jul 27, 2023

LGTM, thanks!

Thanks for reviewing, I am surprised there is not a simpler way to specialize all versions of a type at once

Well, there is, to some extent. You could write:

    namespace detail {

        template <typename T>
        struct is_trivially_relocatable<T> : std::is_trivially_copyable<T>
        {
        };
    }

    template <typename T>
    struct is_trivially_relocatable<T> : detail::is_trivially_relocatable<std::decay_t<T>>
    {
    };

In this case the user type specializations (the macros) would have to to map onto hpx::detail::is_trivially_relocatable instead of hpx::is_trivially_relocatable. However I'm not sure if this is a good idea.

@isidorostsa
Copy link
Contributor Author

Oh that looks a lot cleaner, but I agree we could do without two is_trivially_relocatable traits :P

Also just noting that std::decay_t would not work for this case because we want to transform T[n] -> T instead of T[n] -> T*

@hkaiser
Copy link
Member

hkaiser commented Jul 28, 2023

@isidorostsa so let's go ahead with it as is, should we?

@isidorostsa
Copy link
Contributor Author

Let me add some tests first and I think we are good

@StellarBot
Copy link

Performance test report

HPX Performance

Comparison

BENCHMARKFORK_JOIN_EXECUTORPARALLEL_EXECUTORSCHEDULER_EXECUTOR
For Each(=)??-

Info

PropertyBeforeAfter
HPX Datetime2023-05-10T12:07:53+00:002023-07-28T21:26:07+00:00
HPX Commitdcb54150d4f47e
Datetime2023-05-10T14:50:18.616050-05:002023-07-28T16:36:32.321990-05:00
Envfile
Clusternamerostamrostam
Compiler/opt/apps/llvm/13.0.1/bin/clang++ 13.0.1/opt/apps/llvm/13.0.1/bin/clang++ 13.0.1
Hostnamemedusa08.rostam.cct.lsu.edumedusa08.rostam.cct.lsu.edu

Comparison

BENCHMARKNO-EXECUTOR
Future Overhead - Create Thread Hierarchical - Latch=

Info

PropertyBeforeAfter
HPX Datetime2023-05-10T12:07:53+00:002023-07-28T21:26:07+00:00
HPX Commitdcb54150d4f47e
Datetime2023-05-10T14:52:35.047119-05:002023-07-28T16:38:47.858923-05:00
Envfile
Clusternamerostamrostam
Compiler/opt/apps/llvm/13.0.1/bin/clang++ 13.0.1/opt/apps/llvm/13.0.1/bin/clang++ 13.0.1
Hostnamemedusa08.rostam.cct.lsu.edumedusa08.rostam.cct.lsu.edu

Comparison

BENCHMARKFORK_JOIN_EXECUTOR_DEFAULT_FORK_JOIN_POLICY_ALLOCATORPARALLEL_EXECUTOR_DEFAULT_PARALLEL_POLICY_ALLOCATORSCHEDULER_EXECUTOR_DEFAULT_SCHEDULER_EXECUTOR_ALLOCATOR
Stream Benchmark - Add=(=)(=)
Stream Benchmark - Scale(=)=(=)
Stream Benchmark - Triad(=)(=)(=)
Stream Benchmark - Copy(=)(=)(=)

Info

PropertyBeforeAfter
HPX Datetime2023-05-10T12:07:53+00:002023-07-28T21:26:07+00:00
HPX Commitdcb54150d4f47e
Datetime2023-05-10T14:52:52.237641-05:002023-07-28T16:39:04.983307-05:00
Envfile
Clusternamerostamrostam
Compiler/opt/apps/llvm/13.0.1/bin/clang++ 13.0.1/opt/apps/llvm/13.0.1/bin/clang++ 13.0.1
Hostnamemedusa08.rostam.cct.lsu.edumedusa08.rostam.cct.lsu.edu

Explanation of Symbols

SymbolMEANING
=No performance change (confidence interval within ±1%)
(=)Probably no performance change (confidence interval within ±2%)
(+)/(-)Very small performance improvement/degradation (≤1%)
+/-Small performance improvement/degradation (≤5%)
++/--Large performance improvement/degradation (≤10%)
+++/---Very large performance improvement/degradation (>10%)
?Probably no change, but quite large uncertainty (confidence interval with ±5%)
??Unclear result, very large uncertainty (±10%)
???Something unexpected…

@hkaiser
Copy link
Member

hkaiser commented Jul 31, 2023

@isidorostsa once you fix the clang-format issues, this seems to be good to go.

@isidorostsa
Copy link
Contributor Author

@isidorostsa once you fix the clang-format issues, this seems to be good to go.

Will do, but let's discuss a bit on Thursday before merging

@hkaiser
Copy link
Member

hkaiser commented Aug 5, 2023

@isidorostsa there is still some clang-format problem, could you have a look, please?

@isidorostsa
Copy link
Contributor Author

This will be integrated into #6314

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.

4 participants