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

[Aditya Kumar via Phabricator via libcxx-commits]

hiraditya added inline comments.


================
Comment at: libcxx/benchmarks/ContainerBenchmarks.h:30-34
+__attribute__((noinline))
+int CopyContainer(Container v1) {
+    auto v = v1;
+    return 0;
+}
----------------
philnik wrote:
> 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. 
sgtm. the modified test (with DoNotOptimizeData) keeps the vector alive.


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

https://reviews.llvm.org/D147741