[libcxx-commits] [PATCH] D142063: [libc++][ranges] implement `std::ranges::split_view` (WIP on testing, targeting llvm 16)

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/__ranges/split_view.h:126-129
+  _LIBCPP_NO_UNIQUE_ADDRESS iterator_t<_View> __cur_            = iterator_t<_View>();
+  _LIBCPP_NO_UNIQUE_ADDRESS subrange<iterator_t<_View>> __next_ = subrange<iterator_t<_View>>();
+  split_view<_View, _Pattern>* __parent_                        = nullptr;
+  bool __trailing_empty_                                        = false;
----------------
This makes it possible to pack the `bool` into the `subrange`.


================
Comment at: libcxx/include/__ranges/split_view.h:120
+private:
+  bool __trailing_empty_                                        = false;
+  split_view<_View, _Pattern>* __parent_                        = nullptr;
----------------
huixie90 wrote:
> philnik wrote:
> > var-const wrote:
> > > ldionne wrote:
> > > > philnik wrote:
> > > > > huixie90 wrote:
> > > > > > var-const wrote:
> > > > > > > philnik wrote:
> > > > > > > > Having this first seems like a bad idea. On 64-bit platforms this results in 7 bytes of padding. Maybe it would make sense to bit-pack this into the `split_view<_View, _Pattern>*`?
> > > > > > > Also I don't think we have to follow the order in the standard, but there it's the last member variable.
> > > > > > Yes I will move the bool to the end
> > > > > > Could you please elaborate on bit-packing? 
> > > > > We can make sure that `split_view` is always at least 2 byte aligned (which it is in most cases anyways), which guarantees that the low bit of the pointer is always zero. Since we know that the low bit is always zero, we can use it as a flag. We just have to bit-and the pointer if we want to dereference it. I have an implementation of a utility class for this purpose in D140278, but that's currently not `constexpr`-friendly and needs some more work in general. Using it would essentially mean that we can't ship `split_view` in LLVM16, but I think QoI is more important than having `split_view` now.
> > > > I agree we should try to use `pointer_int_pair` here. One thing we could do is ship `split_view` in LLVM 16, but ship (just that view) as experimental, still. We could guard just that view with `_LIBCPP_ENABLE_EXPERIMENTAL`.
> > > > 
> > > > Edit: Thinking about it some more, I think we'll always need to `reinterpret_cast` stuff around to make this work. It's not clear to me that it'll ever work inside `constexpr` (quite unfortunately). If we don't think we have a reasonable path forward to make it work inside constexpr, it probably doesn't make sense to artificially delay this "just because". @philnik , unless you can think of a way to make `pointer_int_pair` `constexpr` in the future and you think it is likely to materialize, I wouldn't delay this.
> > > We don't do this optimization in a very similar case of `lazy_split_view::outer-iterator` (neither do the other implementations, for `lazy_split_view` or `split_view`). More importantly, is this optimization worth doing? It adds a runtime penalty on many invocations of `operator++` and `operator==`, while I'm not sure if the benefit is worth it -- it makes the iterator type occupy less space, but it still won't fit into a register, and it doesn't seem likely that users are going to store it often. It seems like it would penalize the most common use case for the sake of a more niche one.
> > > 
> > > @philnik , unless you can think of a way to make `pointer_int_pair` `constexpr` in the future and you think it is likely to materialize, I wouldn't delay this.
> > 
> > While not super efficient, we can always just allocate a chunk of memory during constant evaluation to store a pointer and int.
> > 
> > > We don't do this optimization in a very similar case of `lazy_split_view::outer-iterator` (neither do the other implementations, for `lazy_split_view` or `split_view`). More importantly, is this optimization worth doing? It adds a runtime penalty on many invocations of `operator++` and `operator==`, while I'm not sure if the benefit is worth it -- it makes the iterator type occupy less space, but it still won't fit into a register, and it doesn't seem likely that users are going to store it often. It seems like it would penalize the most common use case for the sake of a more niche one.
> > 
> > I would expect the penalty to be extremely small. The operation itself is very fast on modern machines, and the compiler should be able to hoist the bit-and out of a loop. Here is a simple example: https://godbolt.org/z/Y7T1Ge6zW
> @philnik 
> > While not super efficient, we can always just allocate a chunk of memory during constant evaluation to store a pointer and int.
> What if it is a `constexpr` variable in `namespace` scope? Presumably you can't leak the memory allocation in the constexpr context?
Right, that doesn't work for constant initialization. Then I don't think it's possible without compiler magic, which I don't think is viable for now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142063/new/

https://reviews.llvm.org/D142063