This is an archive of the discontinued LLVM Phabricator instance.

Clean up newly created <bit> header
ClosedPublic

Authored by mclow.lists on Aug 16 2018, 4:49 PM.

Details

Reviewers
ldionne
EricWF
Summary

Still NFC here.

  1. Take the 9 functions that each had an #ifndef _LIBCPP_COMPILER_MSVC .. #else .. endif block in them and make just two blocks, each with 9 functions. Much easier to read, but makes for a terrible diff.
  1. Change the return type of __ctz and __clz from the same type as the parameter to int. I would have preferred unsigned, but that's not what P0553 gave us. I reviewed all the call sites for these functions, and for __ctz it was always immediately static_cast-ed to another type, and __clz it made no difference either.
  1. Change the name __pop_count to __popcount to match the name that we're going to add from P0553.
  1. Rename the local variable in the windows code from where to __where. Shame on someone ;-)

Diff Detail

Event Timeline

mclow.lists created this revision.Aug 16 2018, 4:49 PM
mclow.lists added inline comments.
include/bit
113

Missed this one. Should be int

ldionne accepted this revision.Aug 17 2018, 6:46 AM

LGTM with the __clz you missed.

include/bit
61

Funny spacing between __builtin_popcount and (__x)

This revision is now accepted and ready to land.Aug 17 2018, 6:46 AM
lebedev.ri added inline comments.
include/bit
117

Like i commented in the original review, this should probably be

// Search from *M*SB to *L*SB for first set bit.
mclow.lists added inline comments.Aug 17 2018, 7:42 AM
include/bit
117

I think my preference would be to just remove the comment.

mclow.lists added inline comments.Aug 17 2018, 7:58 AM
include/bit
61

It's actually there to line up with the ones below.
(Though now that I look, I didn't do that for the ones above)

Clean up the windows code a bit - though I don't think is used - since I don't think it will compile.

include/bit
96

I'm not sure this code is ever used - since how can this compile?

craig.topper added inline comments.
include/bit
151–152

MSVC blindly uses the popcnt instruction whenever it sees this intrinsic. So this only works on Nehalem and newer Intel CPUs. And Barcelona and newer AMD CPUs. This is why llvm uses a bit math version of popcnt for MSVC in include/llvm/Support/MathExtras.h

161–162

This doesn't exist in 32-bit MSVC.

@craig.topper - that's existing code; I'm not changing it.
If we have a test bot that I can test this against, I'm happy to update it.

@craig.topper - that's existing code; I'm not changing it.
If we have a test bot that I can test this against, I'm happy to update it.

I'm not really sure that this code is actually used anywhere.

mclow.lists closed this revision.Sep 19 2018, 10:10 AM

Landed as r340049