[libcxx-commits] [PATCH] D68365: [libc++] Implement P1004R2 (constexpr std::vector)

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__bit_reference:358-361
+    if (__libcpp_is_constant_evaluated())
+        std::fill_n(std::__to_address(__first.__seg_), 0, __nw);
+    else
+        _VSTD::memset(_VSTD::__to_address(__first.__seg_), 0, __nw * sizeof(__storage_type));
----------------
EricWF wrote:
> var-const wrote:
> > ldionne wrote:
> > > I think we should always call `std::fill_n` here instead. Indeed, both are equivalent, but `std::fill_n` is nicer for various reasons.
> > > 
> > > I know `fill_n` doesn't end up calling `memset` directly, however the compiler will end up calling `memset` (when optimized). IIRC, @var-const was actually about to add this optimization to `fill_n` explicitly, when we realized it didn't have an impact on the code generation. @var-const can you confirm?
> > > 
> > > Also, I think we need to add a comment in `std::fill_n` that explains this. I think it can be done in this patch since we're switching from `memset` to `fill_n`.
> > > 
> > > This comment applies elsewhere in this patch too.
> > I can confirm that working on https://reviews.llvm.org/D118329, I found that the compiler would replace `uninitialized_fill{,_n}` with calls to `memset` in an optimized build, which made it unnecessary to write that optimization explicitly. I would expect `fill_n` to work similarly, but I have only tried with the uninitialized algorithms. FWIW, I have a lingering suspicion that there might be some corner cases (related to aliasing, perhaps) where the compiler would fail to do the optimization, but it was absolutely reliable in all the (simple) cases I checked.
> @ldionne @var-const Can we get a godbolt showing a call to fill_n that gets lowered to memset?
https://godbolt.org/z/je1PGzGds

There is a test for `0` size after the patch, because we do that in `fill_n`. We might or might not want to look into it, but this can be tackled as a separate issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68365