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

Align and minimize One #323

Merged
merged 4 commits into from
Jul 8, 2021

Conversation

bernhardmgruber
Copy link
Member

Fixes #267.

include/llama/Array.hpp Show resolved Hide resolved
@@ -94,17 +136,19 @@ namespace llama
{
Array<T, N + 1> r{};
r[0] = v;
for (auto i = 0; i < N; i++)
r[i + 1] = a[i];
if constexpr (N > 0)
Copy link
Member

Choose a reason for hiding this comment

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

We probably want some curly braces behind the if. I think some compilers even warn about this. Also in the other instances if you change this.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a stylistic question, which I strongly oppose. I know the argument that adding braces could avoid accidentially misindenting a statement, believing it belongs to the if statement. However, clang actually detects such misalignment. Furthermore, our code base is clang-formatted so we would immediately notice. So the only thing braces add here is an extra LOC, spreading the code a little vertically. And I prefer to the code to be compact. I also know clang-tidy can add these braces automatically, so at some point we will have the vote and then we can see what happens :)

include/llama/mapping/One.hpp Outdated Show resolved Hide resolved
include/llama/mapping/One.hpp Outdated Show resolved Hide resolved
include/llama/mapping/AoS.hpp Outdated Show resolved Hide resolved
include/llama/mapping/One.hpp Outdated Show resolved Hide resolved
/// minimize this padding.
/// \see One
template <typename ArrayDims, typename RecordDim>
using MinAlignedOne = One<ArrayDims, RecordDim, true, FlattenRecordDimMinimizePadding>;
Copy link
Member

Choose a reason for hiding this comment

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

Is One<ArrayDims, RecordDim, false, FlattenRecordDimMinimizePadding> missing on purpose?

Copy link
Member Author

Choose a reason for hiding this comment

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

That is a valid mapping, but I do not see the use case. One<..., true, ...> adds padding to respect alignment, so adding field permutations make sense, to play with this padding.
One<..., false, ...> does not add padding, so permuting the fields has a far smaller effect. So I want to advertise it less.

include/llama/mapping/One.hpp Outdated Show resolved Hide resolved
@bernhardmgruber bernhardmgruber merged commit 5c38539 into alpaka-group:develop Jul 8, 2021
@bernhardmgruber bernhardmgruber deleted the minpadone branch July 8, 2021 14:44
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.

One with minimum padding
2 participants