Cudf: [DISCUSSION] Remove `bool8` type

[jrhemstad]

libcudf currently supports a column of boolean elements by using 1 byte per boolean element.

The built-in C++ bool type is not guaranteed to be 1 byte, i.e., sizeof(bool) == 1. Out of an abundance of caution, libcudf implements its own "fixed-width" boolean type called bool8 that is implemented on top of a uint8_t and implements a set of operators that attempt to emulate the built in bool type.

However, the bool8 type has caused many headaches in trying to use it like bool but running into issues with providing the proper conversion and arithmetic operators. I don't believe it is possible to make bool8 as simple to use as bool is.

Therefore, I propose we drop bool8 and instead just use the built-in bool type. For all modern systems, the size of bool is 1 byte. For an extra level of caution, we can add a static_assert(sizeof(bool) == 1) for the very unlikely event that someone tries to build on a system where a bool is not 8 bits.

[harrism]

You know I support this. :) Just for more explicit enumeration of "modern" systems:

NVCC: presumably same as host compiler, which is probably 1 byte on all supported host compilers. But for completeness:
Linux on x86: GCC and Clang both use 1 byte for bool
Linux on ARM: AFAICT this is the same as above
Linux on POWER: Can't find specific documentation about sizeof(bool) on GCC/Clang on POWER, but IBM's XL compiler optimization guide says 1 byte. (Not that we plan to support the XL compiler)
MSVC on X86: Since MSVC 5.0, MSVC uses 1 byte for bool
MacOS on x86: Clang uses 1 byte for bool

Interestingly, the PGI compiler guide doesn't mention bool: https://www.pgroup.com/resources/docs/19.5/x86/pgi-ref-guide/index.htm#data-types-c-cpp-scalars

All this said, I think removing bool8 and instead using the suggested static_assert would be a great thing to do.

[codereport]

Note ... I quickly ran into:

warning: incrementing a bool value is deprecated

In C++11 it was deprecated: https://timsong-cpp.github.io/cppwp/n3337/expr.pre.incr#1
In C++17 it will be fully removed: https://eel.is/c++draft/expr.pre.incr#1

This might not be an issue, as bool8 doesn't define the pre-increment operator ... but it might be. Will update when I know more.

[codereport]

@jrhemstad @harrism So over the last couple days in between PR reviews, I have been attempting this from a couple different angles - as very quickly you end up just resolving compile time errors. However, I found an approach that seemed to be working and I was making progress ... but then I hit a wall.

vector<bool>

I assume both of you are aware, but C++ has a vector<bool> specialization where bits are packed in order to save space, which unfortunately means it doesn't work the way would we would expect in many situations (see here for more info)

There are several solutions. The top recommended solution is to use std::deque but unfortunately that doesn't work for us because we need the .data() method which std::deque does not have. Others include (see full stack overflow post here):

• boost::container::vector
• struct wrapper -> currently we are doing this for other reasons
• use different type like char / int with same size for template type

I tried using boost::container::vector and it looks like it does work but it also seems that due to our pervasive use of std::vector, the usages of boost::container::vector would end up cascading and replacing many of _if not all_ of the std::vector usages.

What do people think? Any other solutions? Is removing bool8 worth losing std::vector for a certain percentage of use cases?

[harrism]

Hmm, I didn't know about this. That is just EVIL. Why would C++ do that? Surely it would be better to opt in to such an "optimization"?

Does libcudf specifically rely on vector features that trigger the bad behavior of vector<bool>? i.e. do we use &c[0] at all? I guess even if we didn't, using c.data() would not return us a pointer to an array of byte-sized bools anyway, would it?

We may need our wrapper after all...

[codereport]

Yes, I agree it is EVIL. There is a member of the community that goes by vector of bool because it is such a well known "failure" by the standards committee ... in hindsight everyone agrees it was a big mistake and definitely we should have just supplied that behaviour in a different data structure.

Specifically the piece of code that is encountering this problem is the following in column_utilities.hpp:

std::pair<std::vector<T>, std::vector<bitmask_type>> to_host(column_view c) {
  std::vector<T> host_data(c.size());
  CUDA_TRY(cudaMemcpy(host_data.data(), c.head<T>(), c.size() * sizeof(T), cudaMemcpyDeviceToHost));
  return { host_data, bitmask_to_host(c) };
}

which generates the following:

/home/choekstra/ws3/cudf/cpp/tests/utilities/column_utilities.hpp(102): error: argument of type "void" is incompatible with parameter of type "void *"
          detected during:
            instantiation of "std::pair<std::vector<T, std::allocator<T>>, std::vector<cudf::bitmask_type, std::allocator<cudf::bitmask_type>>> cudf::test::to_host<T>(cudf::column_view) [with T=__nv_bool]" 

Like I said, boost::container::vector is a solution, but not sure that is the direction we want to go. Or if there is an alternative solution.

[harrism]

Funny, that's the function I simplified the other day (irrelevant to this issue).

I'm not sure we can change the container -- that would require dropping use of vector basically everywhere, wouldn't it? That's why I said it may be simpler to keep our wrapper.

[jrhemstad]

Just use thrust::host_vector<bool> for to_host.

There shouldn't be anywhere else where we rely on the size of vector<bool>.

[harrism]

You mean we don't use it in testing? E.g. fixed_width_column_wrapper constructors?

[jrhemstad]

You mean we don't use it in testing? E.g. fixed_width_column_wrapper constructors?

If we just replace all use of std::vector with thrust::host_vector in testing (like this: https://github.com/rapidsai/cudf/blob/branch-0.11/cpp/tests/utilities/column_wrapper.hpp#L115) then we should be okay.

[codereport]

thrust::host_vector definitely works. And yes there other places we use it, I just modified the following code when working on streamcompaction tests:

template <typename Element, typename InputIterator>
rmm::device_buffer make_elements(InputIterator begin, InputIterator end) {
  thrust::host_vector<Element> elements(begin, end);
  return rmm::device_buffer{elements.data(), elements.size() * sizeof(Element)};
}

but there doesn't seem to be any problems.

[harrism]

I think we only need to place the templated vector cases, not the ones where we use a specific (non-bool) type. There are quite a few of the latter in column_wrapper.hpp.

[kkraus14]

Fixed by #3581