-
Notifications
You must be signed in to change notification settings - Fork 69
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
Remove namespace alpaka::block and some inner namespaces #1182
Conversation
5e37c6a
to
d706fdf
Compare
d706fdf
to
a82489f
Compare
54e3624
to
9832e1d
Compare
//! Defines block operation functors. | ||
namespace blockOp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we cool with this? I did not want to rename to alpaka::op
because this namespace already contains the atomic operations and then users could expect to be able to call:
alpaka::atomicOp<alpaka::op::LogicalAnd>()
which is an error, because LogicalAnd
is a block operation and not an atomic operation. This would work:
alpaka::atomicOp<alpaka::op::And>()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it make more sense to rename alpaka::op::And
to alpaka::atomicAnd
and then remove the entire op
namespace? We wouldn't need blockOp
then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would very much just have explicit atomic functions, such as: alpaka::atomicAnd(acc, T* addr, T value) -> T
. So we cannot name the functor that implements the atomic operation the same. We could name it alpaka::AtomicAnd
though. And alpaka::atomicAnd()
just calls alpaka::atomicOp<alpaka::AtomicAnd>()
.
Afterwards, we can rename alpaka::blockop::LogicalAnd
into alpaka::BlockAnd
, which is then used as alpaka::syncBlockThreadsPredicate<alpaka::BlockAnd>(acc, predicate)
.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also make the case again that there should be namespaces for the functors like alpaka::atomic::And
and alpaka::block::And
. However, I would leave the user API functions outside, so a user calls alpaka::atomicOp<alpaka::atomic::And>()
and alpaka::syncBlockThreadsPredicate<alpaka::block::And>(acc, predicate)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am very much for introducing renamings like alpaka::atomicAnd
(not in this PR of course). However, with this naming in mind, I find having alpaka::AtomicAnd
to denote the operation confusing. As normally in such situations we follow the naming that capitalized means functor and small case means the corresponding function, that would not be the case here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is a renaming of the atomic functions: #1185
alpaka::AtomicAnd
would be the functor. The function using the functor is called alpaka::atomicOp<Functor>(...)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but then this function is also renamed as alpaka::atomicAnd()
, which is connected but does not exactly match the meaning of alpaka::AtomicAnd
? Or did I get your idea wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current state of alpaka before this PR:
- we have functors like
alpaka::op::And
that can be used as template argument to functions likealpaka::atomicOp
. - we have functors like
alpaka::block::op::And
that can be used as template argument to functions likealpaka::block::syncBlockThreadsPredicate
The state of alpaka after this PR:
- we have functors like
alpaka::op::And
that can be used as template argument to functions likealpaka::atomicOp
. - we have functors like
alpaka::blockOp::And
that can be used as template argument to functions likealpaka::syncBlockThreadsPredicate
The state of alpaka after this PR and PR #1185:
- we have functors like
alpaka::op::And
that can be used as template argument to functions likealpaka::atomicOp
. - additionally, we have functions like
alpaka::atomicAnd
that just callalpaka::atomicOp<alpaka::op::And>
- we have functors like
alpaka::blockOp::And
that can be used as template argument to functions likealpaka::syncBlockThreadsPredicate
The state of alpaka after this PR and PR #1185 and renaming the atomic functors:
- we have functors like
alpaka::AtomicAnd
that can be used as template argument to functions likealpaka::atomicOp
. - additionally, we have functions like
alpaka::atomicAnd
that just callalpaka::atomicOp<alpaka::AtomicAnd>
- we have functors like
alpaka::blockOp::And
that can be used as template argument to functions likealpaka::syncBlockThreadsPredicate
The state of alpaka after this PR and PR #1185 and renaming the atomic functors and also renaming the block sync functors:
- we have functors like
alpaka::AtomicAnd
that can be used as template argument to functions likealpaka::atomicOp
. - additionally, we have functions like
alpaka::atomicAnd
that just callalpaka::atomicOp<alpaka::AtomicAnd>
- we have functors like
alpaka::BlockAnd
that can be used as template argument to functions likealpaka::syncBlockThreadsPredicate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for a detailed explanation. Turned out, I was still confused about some things and did not realize it. Nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created PR #1186 for the remaining changes.
See #1034
Also renames the namespace
alpaka::block::op
toalpaka::blockOp
to not be confused withalpaka::op
containing atomic operators. I think we should debate this!