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

Enable examples for SYCL #3691

Merged
merged 2 commits into from
Jan 14, 2021
Merged

Conversation

masterleinad
Copy link
Contributor

The return type for extent is size_t.
Again, I would prefer using #3615 instead of commenting all the printfs.

Copy link
Member

@crtrott crtrott left a comment

Choose a reason for hiding this comment

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

I am not happy with the changes to explicit size_t. Folks should not use size_t for loop counters since its often bad for performance. Rather cast the return of extent. These examples generally should reflect best practices.

@masterleinad
Copy link
Contributor Author

I am not happy with the changes to explicit size_t. Folks should not use size_t for loop counters since its often bad for performance. Rather cast the return of extent. These examples generally should reflect best practices.

Doesn't it make more sense then to let extent return ViewType::size_type?

@dalg24
Copy link
Member

dalg24 commented Jan 7, 2021

I had the same initial reaction than you but then I look at View::extent implementation and changed my mind

template <typename iType>
KOKKOS_INLINE_FUNCTION constexpr
typename std::enable_if<std::is_integral<iType>::value, size_t>::type
extent(const iType& r) const noexcept {
return m_map.extent(r);
}

@masterleinad
Copy link
Contributor Author

I guess this is a bigger discussion that we might not need to have here. For now, I just cast the upper bounds instead.

@masterleinad
Copy link
Contributor Author

Ready from my side. Needs more reviews.

Copy link
Member

@Rombur Rombur 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.

Again, I would prefer using #3615 instead of commenting all the printfs.

I disagree here. I don't think we want users to use that macro.

@masterleinad
Copy link
Contributor Author

I disagree here. I don't think we want users to use that macro.

Yes, that's true.

@crtrott crtrott merged commit 3fe9e4f into kokkos:develop Jan 14, 2021
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.

None yet

4 participants