This is an archive of the discontinued LLVM Phabricator instance.

[llvm] Move bit counting functions to bit.h (NFC)
ClosedPublic

Authored by kazu on Jan 18 2023, 8:35 PM.

Details

Summary

This patch provides C++20-style countl_zero, countr_zero, countl_one,
and countl_one in bit.h. Existing functions like countLeadingZeros
become wrappers around the new functions.

Note that I cannot quite declare countLeadingZeros as:

template <class T> using countLeadingZeros = countl_zero<T>;

because countl_zero returns int, whereas countLeadingZeros returns
unsigned.

Diff Detail

Event Timeline

kazu created this revision.Jan 18 2023, 8:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 18 2023, 8:35 PM
kazu requested review of this revision.Jan 18 2023, 8:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 18 2023, 8:35 PM

Is the plan to migrate callers over to this eventually, and remove the countNnn wrappers?

llvm/include/llvm/ADT/bit.h
44–54

Instead of this, perhaps we could write small/simple wrappers, avoiding the need to forward declare things from platform headers? & possibly wrapping the basic operations/differences between the platforms in a more direct way, then exposing these higher level primitives based on those abstractions?

kazu added a comment.Jan 18 2023, 9:50 PM

Is the plan to migrate callers over to this eventually, and remove the countNnn wrappers?

Yes.

llvm/include/llvm/ADT/bit.h
44–54

Do you mean something like this?

namespace detail {
#if __has_builtin(__builtin_clz) || defined(__GNUC__)
inline constexpr bool has_clz = true;
inline unsigned clz(unsigned Val) {
  return __builtin_clz(Val)
}
#else defined(_MSC_VER)
inline constexpr bool has_clz = true;
inline unsigned clz(unsigned Val) {
  unsigned long Index;
  _BitScanForward(&Index, Val);
  return Index;
}
#else
inline constexpr bool has_clz = false;
#endif
}  // namespace detail

Then implement llvm::countl_zero with llvm::detail::has_clz and llvm::detail:clz?

If so, I like this idea, but I don't see how we can avoid forward-declaring things. The comment says intrin.h is very expensive although I don't know whether the comment is still applicable today or how old the comment is.

Thanks @kazu!

Please can you add test coverage to ADT/BitTest.cpp ? By the looks of it we never added any equivalent to Support/MathExtrasTest.cpp :(

llvm/include/llvm/ADT/bit.h
44–54

intrin.h is still (or possibly more) expensive to include, so avoiding use in a common header like this would be best.

46

MathExtras.h ?

llvm/include/llvm/Support/MathExtras.h
22

Can you confirm are all of these still needed?

dblaikie added inline comments.Jan 19 2023, 10:45 AM
llvm/include/llvm/ADT/bit.h
44–54

I was thinking of an out of line solution - where the use of __builtin_clz and _BitScanForward are in a .cpp file, not in a header. I guess then performance is so bad that LTO is required to get to something reasonable? Maybe that's OK?

kazu updated this revision to Diff 490690.Jan 19 2023, 5:33 PM

I've moved the forward declarations of _BitScan* functions to bit.h
instead of copying them.

I've also added unit tests.

I've removed #include "llvm/Support/Compiler.h" in MathExtras.h as we
don't need it according to include-what-you-use.

kazu marked 5 inline comments as done.Jan 19 2023, 5:38 PM

Please take a look. Thanks!

llvm/include/llvm/ADT/bit.h
44–54

Hmm. I am not sure if I want to go as far as requiring users to have LTO.

I think what to do with these forward declarations is somewhat orthogonal to moving these bit counting functions to bit.h.

46

I've updated the comment.

llvm/include/llvm/Support/MathExtras.h
22

include-what-you-use tells me that I can remove:

#include "llvm/Support/Compiler.h"

at least on a x86_64 Linux machine.

We still need all the others.

dblaikie accepted this revision.Jan 19 2023, 7:05 PM
dblaikie added inline comments.
llvm/include/llvm/ADT/bit.h
44–54

Oh, sorry, fair enough - if it's just moving things around they might be unfortunate but it's no worse and not bad enough to relitigate. Sorry for the distraction.

This revision is now accepted and ready to land.Jan 19 2023, 7:05 PM
This revision was automatically updated to reflect the committed changes.
kazu marked 3 inline comments as done.
vzakhari added inline comments.
llvm/include/llvm/Support/MathExtras.h
17

This removal breaks the build with GCC-9*, because __has_builtin seems to be available starting GCC-10. Now we are missing this useful code from Compiler.h:

#ifndef __has_builtin
# define __has_builtin(x) 0
#endif

Can you please bring the include back to both MathExtras.h and bit.h?

vzakhari added inline comments.Jan 19 2023, 10:12 PM
llvm/include/llvm/Support/MathExtras.h
17
kazu marked 2 inline comments as done.Jan 19 2023, 10:50 PM
kazu added inline comments.
llvm/include/llvm/Support/MathExtras.h
17

I just checked in:

https://github.com/llvm/llvm-project/commit/dddcf3014aa094d3ecd0430511a5c10045fefbd6

Thank you for reporting the problem!

thakis added a subscriber: thakis.Jan 20 2023, 4:38 AM

This breaks building on windows: http://45.33.8.238/win/73448/step_4.txt

Please take a look and revert for now if it takes a while to fix.

I guess forward declarations should be moved to global namespace?

kazu added a comment.Jan 20 2023, 1:03 PM

Thank you for taking care of this!