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.

Sign up for GitHub

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

Fix clang 17 compilation #394

Merged
merged 2 commits into from
Dec 21, 2023
Merged

Fix clang 17 compilation #394

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e5c9282
Fix error compiling tuple_size in clang 17.
AustinSchuh Nov 16, 2023
8c9798b
Make member access explicitly qualified for clang 17
AustinSchuh Nov 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions include/cuco/detail/pair/pair.inl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,6 @@ __host__ __device__ constexpr bool operator==(cuco::pair<T1, T2> const& lhs,

} // namespace cuco

namespace thrust {
#include <cuco/detail/pair/tuple_helpers.inl>
} // namespace thrust
Comment on lines -53 to -55
Copy link
Member

Choose a reason for hiding this comment

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

@sleeepyjack

Just a reminder to ourselves, thrust::pair is the same as cuda::std::pair so the current pair trait logic can be simplified.


namespace cuda::std {
#include <cuco/detail/pair/tuple_helpers.inl>
} // namespace cuda::std
4 changes: 2 additions & 2 deletions include/cuco/detail/static_map.inl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,7 +443,7 @@ __device__
"insert_and_find is not supported for unpackable data on pre-Volta GPUs.");
#endif

auto current_slot{initial_slot(insert_pair.first, hash)};
auto current_slot{this->initial_slot(insert_pair.first, hash)};
Copy link
Member

@PointKernel PointKernel Dec 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm trying to understand the logic here since we have many places missing this-> but how can they be built properly?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would be genuinely surprised if this is the only place where we forgot to use this->. So yeah, not sure why clang is complaining here but not in the other cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My suspicion is that this is the only code I am calling in my application and exercising. I need to go dig in and get you a real answer though, not a guess.

Copy link
Member

Choose a reason for hiding this comment

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

this is the only code I am calling in my application and exercising.

Make sense.

@sleeepyjack we can merge the current PR as it is and fix other uncovered issues in your clang CI PR. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm fine with that, especially if it unblocks @AustinSchuh 's work for now.


while (true) {
key_type const existing_key = current_slot->first.load(cuda::std::memory_order_relaxed);
Expand Down Expand Up @@ -514,7 +514,7 @@ __device__

// if we couldn't insert the key, but it wasn't a duplicate, then there must
// have been some other key there, so we keep looking for a slot
current_slot = next_slot(current_slot);
current_slot = this->next_slot(current_slot);
}
}

Expand Down
Loading