Skip to content

Conversation

@QiJune
Copy link
Member

@QiJune QiJune commented Dec 22, 2017

Fix #6769

@QiJune QiJune changed the title refine OpKernelKey refine OpKernelType Dec 22, 2017
int library_type = static_cast<int>(key.library_type_);

size_t seed = 0;
hash_combine(seed, place);
Copy link
Contributor

Choose a reason for hiding this comment

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

how about combine these four fields as one integer, then make a hash_combine function call?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can not simply combine these four fields together. HashCombine is the function to do such work.

Copy link
Contributor

Choose a reason for hiding this comment

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

我的意思是,四次HashCombine调用开销可以省略。

KernelType种类并不多,不会发生碰撞,hash只是让分布均匀一下。
防止碰撞可以这样

constexpr int SHIFT = 8; // every kind of type less than 2^8
int data_type = static_cast<int>(key.data_type_) + 1<<(SHIFT);
int data_layout = static_cast<int>(key.data_layout_) + 1 <<(SHIFT + 1);
int library_type = static_cast<int>(key.library_type_) + 1 << (SHIFT + 2);
int kernel_id = data_type + data_layout + library_type + place.which();
std::hash<int> hasher;
return hasher(kernel_id);

c-why-is-boosthash-combine-the-best-way-to-combine-hash-values
*/
template <class T>
inline void hash_combine(std::size_t& seed, const T& v) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Google Style.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

c-why-is-boosthash-combine-the-best-way-to-combine-hash-values
*/
template <class T>
inline void HashCombine(std::size_t& seed, const T& v) {
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@dzhwinter dzhwinter left a comment

Choose a reason for hiding this comment

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

This PR is a core part of our KernelKey design. We can merge it ASAP, and I will give a small fix.
@QiJune @jacquesqiao

@dzhwinter dzhwinter merged commit 37e9626 into PaddlePaddle:develop Dec 24, 2017
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