search

lemire / FastPFor

The FastPFOR C++ library: Fast integer compression
Apache License 2.0
849 stars 124 forks source link

Demonstrate code unrolling with templates #30

Closed romange closed 6 years ago

romange commented 6 years ago

Hi @lemire . Please take a look at this PR. It's not intended for merging at his moment but merely demonstrates how templates can reduce amount of code in this file without compromising the performance. Rfastunpack1, Rfastunpack2, R__fastunpack3 are equivalent to their original counter-parts but are one-liners. I verified with gcc -O3 -mtune=core2 -std=c++17 -S -masm=intel bitpacking.cpp that the asm code generated is the same in both versions.

searchivarius commented 6 years ago

Very clever. My concern would be that it makes code less understandable. Perhaps, it's not such a bad thing given how much shorter it's now.

lemire commented 6 years ago

Very impressive C++ work.

;-)

romange commented 6 years ago

Yes, sfinae is borderly understandable though here it's not that complicated. But I understand the concerns. Would you like to keep the original code?

lemire commented 6 years ago

@romange Sorry I did not get back to you regarding your question. I think I did write a reply back but never hit the "comment" button. I did not mean to be rude. I think that your work is fantastic and well worth including.

romange commented 6 years ago

Hey Daniele, all is good :) and you are not rude at all. i just did not want to push something that you think is unneeded. But if you think it's worth including i will try to send the whole PR sometime next week.

Have a great weekend)

On Aug 25, 2017 10:59 PM, "Daniel Lemire" notifications@github.com wrote:

@romange https://github.com/romange Sorry I did not get back to you regarding your question. I think I did write a reply back but never hit the "comment" button. I did not mean to be rude. I think that your work is fantastic and well worth including.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/lemire/FastPFor/pull/30#issuecomment-325021290, or mute the thread https://github.com/notifications/unsubscribe-auth/ADgSiLjBC4qkt9D6DLxIMwgqf5upZJ8vks5sbyeXgaJpZM4O_dFx .

dnbaker commented 6 years ago

It's nice to see that the compiler's constant-folding can substitute when if constexpr isn't yet supported.