This is an archive of the discontinued LLVM Phabricator instance.

[ADT] Add bit_floor, bit_ceil, and bit_width to bit.h
ClosedPublic

Authored by kazu on Jan 19 2023, 10:36 PM.

Details

Summary

This patch adds C++20-style bit_floor, bit_ceil, and bit_width.

In a subsequent patch, I'm going to define PowerOf2Floor in
MathExtras.h in terms of bit_floor.

Unfortunately, PowerOf2Ceil isn't quite the same as bit_ceil because
PowerOf2Ceil(0) == 0, whereas bit_ceil(0) == 1.

MathExtras.h does not have a function directly corresponding to
bit_width, but Log2_32(X) + 1, which occurs in a few places, can be
replaced with bit_width(X).

Diff Detail

Event Timeline

kazu created this revision.Jan 19 2023, 10:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 19 2023, 10:36 PM
kazu requested review of this revision.Jan 19 2023, 10:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 19 2023, 10:36 PM
RKSimon added inline comments.Jan 20 2023, 3:50 AM
llvm/include/llvm/ADT/bit.h
245

bit_ceil can have UB behaviour for unrepresentable values - maybe at least mention this in the comment?

250

Does this need to be bit_width<T> or llvm::bit_width((T)(Value - 1u)) ?

All these functions are constexpr in C++20. Should we do the same?

All these functions are constexpr in C++20. Should we do the same?

It'd be nice, but IIRC there were a few problems on some compilers when we tried to do this for some of the previous additions (plus for instance bit_cast uses memcpy).

kazu updated this revision to Diff 490897.Jan 20 2023, 9:43 AM

I've added a comment about the undefined behavior.

I've specified the type like llvm::bit_width((T)(Value - 1u)).

kazu marked 2 inline comments as done.Jan 20 2023, 9:46 AM

All these functions are constexpr in C++20. Should we do the same?

It'd be nice, but IIRC there were a few problems on some compilers when we tried to do this for some of the previous additions (plus for instance bit_cast uses memcpy).

Yes, it would be nice, but for now, I'd focus on providing these functions named consistently with their respective corresponding C++20 functions.

RKSimon accepted this revision.Jan 20 2023, 9:56 AM

LGTM with one minor

llvm/unittests/ADT/BitTest.cpp
129

maybe add tests with the msb set for bit_width / bit_floor?

This revision is now accepted and ready to land.Jan 20 2023, 9:56 AM
This revision was landed with ongoing or failed builds.Jan 20 2023, 7:34 PM
This revision was automatically updated to reflect the committed changes.