[libcxx-commits] [PATCH] D100730: [libc++] Simplify debug iterators, and fix a couple of non-explicit constructors.

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/include/list:297
     _LIBCPP_INLINE_VISIBILITY
     explicit __list_iterator(__link_pointer __p, const void* __c) _NOEXCEPT
         : __ptr_(__p)
----------------
Quuxplusone wrote:
> Quuxplusone wrote:
> > EricWF wrote:
> > > We're now always passing another parameter to the constructor, eating a register.
> > > We don't want to pay that cost in non-debug mode.
> > > 
> > > There was a reason this code was written in this ugly manner.
> > @EricWF: What compiler/target are you concerned about? I'm sure Clang/LLVM on x86 is smart enough to inline these inline functions — I can work on getting evidence for that — but I'm guessing you had something more obscure in mind?
> @EricWF:
> Hm, I see, in non-debug mode, `clang++ -O1` isn't quite smart enough to avoid eating that register. (At `-O2` and up, it is smart enough, and produces identical codegen.)
> OTOH, [std::vector::__make_iter](https://github.com/llvm/llvm-project/blob/2d0f1fa/libcxx/include/vector#L820-L823) has existed since 2011-or-earlier, and produces the exact same symptom: it eats one more register (at `-O1` or lower, in non-debug mode) than it really needs to. I verified this by adding an ifdef that "manually inlines" all the calls to `__make_iter(__p)`, and observing the difference in codegen.
> 
> Another idea I had was to add a `std::list::__make_iter` method analogous to `vector`'s, but then make it `static` when not in debug mode:
> ```
> #if _LIBCPP_DEBUG_LEVEL == 2
>     iterator __make_iter(pointer __p) { return iterator(__p, this); }
> #else
>     static iterator __make_iter(pointer __p) { return iterator(__p); }
> #endif
> ```
> However, obviously this would give worse codegen at `-O0` than we have today, because it's introducing a layer of indirection. And it still isn't //quite// as good at `-O1` as "manually inlining" `__make_iter`. (I tried out a `static` version of `vector::__make_iter` and could still see some tiny diffs relative to the "manually inlined" version.)
> 
> In short: You're right, this hurts register allocation at `-O1`.
> 
> However, I continue to think that people who care about optimization should really be using `-O2`, and that we //should// make this PR's change to simplify the code of <list> and bring its debug iterators into line with <vector> and <string>.
> 
> Here's a playground where you can (for the next few weeks at least — until I update my P1144 branch) play around with the various possibilities I've implemented via ifdefs: https://godbolt.org/z/jWhfPGjcf
@ericwf ping for your thoughts? My playground is still up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100730