[PATCH] D16947: [PGO] assignment operator does not get profile data

[David Blaikie via cfe-commits]

On Mon, Feb 8, 2016 at 9:25 AM, David Li via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> davidxl updated this revision to Diff 47217.
> davidxl added a comment.
>
> Simplified test case suggested by Vedant.
>
>
> http://reviews.llvm.org/D16947
>
> Files:
>   lib/CodeGen/CGClass.cpp
>   test/Profile/def-assignop.cpp
>
> Index: test/Profile/def-assignop.cpp
> ===================================================================
> --- test/Profile/def-assignop.cpp
> +++ test/Profile/def-assignop.cpp
> @@ -0,0 +1,32 @@
> +// RUN: %clang_cc1 -x c++ -std=c++11 %s -triple x86_64-unknown-linux-gnu
> -main-file-name def-assignop.cpp -o - -emit-llvm -fprofile-instrument=clang
> | FileCheck --check-prefix=PGOGEN %s
> +// RUN: %clang_cc1 -x c++ -std=c++11 %s -triple x86_64-unknown-linux-gnu
> -main-file-name def-assignop.cpp -o - -emit-llvm -fprofile-instrument=clang
> -fcoverage-mapping | FileCheck --check-prefix=COVMAP %s
> +
> +struct B {
> +  void operator=(const B &b) {}
> +  void operator=(const B &&b) {}
>

Probably best to make these canonical to avoid confusion:

B &operator=(const B&);
B &operator=(B&&);

(& they don't need definitions - just declarations)

Also, neither of these are the move /constructor/, just the move operator.
Not sure if Vedant just used the wrong terminology, or whether it's worth
testing the move/copy ctors too, to check that they do the right thing as
well. (if all of these things use the same codepath, I don't see a great
benefit in having separate tests for them (but you can add them here if you
like) - I'm just suggesting a manual verification in case those need a
separate fix)


> +};
> +
> +struct A {
> +  A &operator=(const A &) = default;
>

Is the fix/codepath specifically about explicitly defaulted ops? Or just
any compiler-generated ones? (you could drop these lines if it's about any
compiler-generated ones, might be simpler/more obvious that it's not about
the "= default" feature)


> +  // PGOGEN: define {{.*}}@_ZN1AaSERKS_(
> +  // PGOGEN: %pgocount = load {{.*}} @__profc__ZN1AaSERKS_
> +  // PGOGEN: {{.*}}add{{.*}}%pgocount, 1
> +  // PGOGEN: store{{.*}}@__profc__ZN1AaSERKS_
> +  A &operator=(A &&) = default;

+  // PGOGEN: define {{.*}}@_ZN1AaSEOS_
> +  // PGOGEN: %pgocount = load {{.*}} @__profc__ZN1AaSEOS_
> +  // PGOGEN: {{.*}}add{{.*}}%pgocount, 1
> +  // PGOGEN: store{{.*}}@__profc__ZN1AaSEOS_
> +
> +  // Check that coverage mapping includes 6 function records including the
> +  // defaulted copy and move operators: A::operator=
> +  // COVMAP: @__llvm_coverage_mapping = {{.*}} { { i32, i32, i32, i32 },
> [5 x <{{.*}}>],
> +  B b;
> +};
> +
> +int main() {
> +  A a1, a2;
> +  a1 = a2;
> +  a2 = static_cast<A &&>(a1);
>

An option, though not necessarily better, would be to just take the address
of the special members:

auto (B::*x)(const B&) = &bar::operator=;
auto (B::*x)(B&&) = &bar::operator=;

In short, what I'm picturing, in total:

struct A {
  A &operator=(const A&);
  A &operator=(A&&);
};

struct B {
  A a;
};

auto (B::*x)(const B&) = &B::operator=;
auto (B::*x)(B&&) = &B::operator=;

Also, this test should probably be in clang, since it's a clang code
change/fix.



> +  return 0;
> +}
> Index: lib/CodeGen/CGClass.cpp
> ===================================================================
> --- lib/CodeGen/CGClass.cpp
> +++ lib/CodeGen/CGClass.cpp
> @@ -1608,6 +1608,7 @@
>
>    LexicalScope Scope(*this, RootCS->getSourceRange());
>
> +  incrementProfileCounter(RootCS);
>    AssignmentMemcpyizer AM(*this, AssignOp, Args);
>    for (auto *I : RootCS->body())
>      AM.emitAssignment(I);
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160208/813c0dad/attachment.html>