Skip to content

Conversation

@HazelCullom
Copy link
Contributor

No description provided.

@HazelCullom HazelCullom requested a review from cwsmith October 12, 2022 15:37
Copy link
Contributor

@cwsmith cwsmith left a comment

Choose a reason for hiding this comment

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

Looks good. There is one small change in the driver and a comment about a future cleanup/revision to the interface.

@HazelCullom HazelCullom merged commit 685c791 into main Oct 12, 2022
template <class ExecutionSpace, class MemorySpace, class T, int width, int vecLen>
template <class ExecutionSpace, class MemorySpace, class... Ts>
class CabSliceFactory {
static constexpr int vecLen = Impl::PerformanceTraits<ExecutionSpace>::vector_length/8;
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the division by 8? Is the vector_length reported in bits?

Can you point me at where Cabana uses this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I divided it by eight because when I changed to this vector length instead of the user defined one, the stride calculation got messed up, being eight times larger than it should have been which gave me errors. I have just revised the calculation to be correct and not divide the vector length by 8.

Cabana uses this here as a default template parameter for the AoSoA
https://github.com/ECP-copa/Cabana/blob/1c44a7fef24b61f73e629ddd2084c4d05b0ec977/core/src/Cabana_AoSoA.hpp#L146:~:text=int%20VectorLength%20%3D%20Impl,DeviceType%3A%3Aexecution_space%3E%3A%3Avector_length


int main(int argc, char* argv[]) {
// AoSoA parameters
const int vecLen = 4;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this still be here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope. It will be removed in the next version

auto slice_wrapper3 = cabSliceFactory.makeSliceCab<3>();

// simd_parallel_for setup
Cabana::SimdPolicy<vecLen, ExecutionSpace> simd_policy(0, num_tuples);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we eliminate the vecLen here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

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.

3 participants