[libcxx-commits] [PATCH] D147741: [libc++, std::vector] call the optimized version of __uninitialized_allocator_copy for trivial types

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik accepted this revision.
philnik added a comment.
This revision is now accepted and ready to land.

LGTM % nit.



================
Comment at: libcxx/benchmarks/ContainerBenchmarks.h:45
+  for (auto _ : st) {
+    c1 = c2;
+  }
----------------
For good measure I'd add a `DoNotOptimize` after the assignment.


================
Comment at: libcxx/benchmarks/ContainerBenchmarks.h:30-34
+__attribute__((noinline))
+int CopyContainer(Container v1) {
+    auto v = v1;
+    return 0;
+}
----------------
hiraditya wrote:
> philnik wrote:
> > It looks like this doesn't do what it's supposed to: https://godbolt.org/z/vdqGrcM3P. At least with your new version, the container never get copied. You have to add `DoNotOptimize(v)` or something similar, so the optimizer doesn't just remove the code. Then you can also drop the `__attribute__((noinline))`. Same applies to `CopyContainerInto`. I would just remove the functions and make the copy construction and assignment inline.
> the intent is to show that redundant copy of vector is removed because of the new changes. this was the motivation behind creating the bug. inlining this in the caller should have the same effect. if we try to prevent the deletion, that could be a separate test case? let me know your preference, i'm fine either way.
I don't think it makes sense to test the removal of redundant code. IMO the interesting part is forwarding real calls to memmove, since that is the way more likely situation in the wild. Removal of redundant code is just a nice side effect from my point of view. 


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

https://reviews.llvm.org/D147741