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

[David Blaikie via cfe-commits]

On Mon, Feb 8, 2016 at 12:07 PM, Xinliang David Li <davidxl at google.com>
wrote:

> On Mon, Feb 8, 2016 at 11:39 AM, David Blaikie <dblaikie at gmail.com> wrote:
> >
> >
> > 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)
>
> Will change.
>
> >
> > 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
>
> I added tests for copy ctors, and plan to add move ctor test soon.
>
> > 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
>
> the ctor and assignment op do not share the same path -- the ctor path
> is working as expected without the fix -- or do you mean there is no
> need to cover both copy and move variants?
>

I wouldn't necessarily bother testing multiple instances of the same
codepath (so the copy and move ctor for example) - but 2 instances is no
big deal (if there were several more, I might be inclined to just test one
as a representative sample). I don't mind either way, though. The number is
small & the test cases are arguably distinct.


> >
> >>
> >> +};
> >> +
> >> +struct A {
> >> +  A &operator=(const A &) = default;
> >
> >
> > Is the fix/codepath specifically about explicitly defaulted ops?
>
> yes -- explicitly defaulted. There are some test coverage already for
> implicitly declared ctors (but not assignment op -- probably worth
> adding some testing too).
>

Hmm - are you sure there's no common codepath that would cover the
explicitly defaulted or implicitly defaulted ops together in one go?


>
> > 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)
>
> Other compiler generated ones are handled differently.
>
> thanks,
>
> David
>
> >
> >>
> >> +  // 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/362cbb07/attachment-0001.html>