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

[David Blaikie via cfe-commits]

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

> On Mon, Feb 8, 2016 at 3:17 PM, David Blaikie <dblaikie at gmail.com> wrote:
> >
> >
> > 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.
>
> Sorry I disagree with your general statement here. I treat such test
> cases as 'black box testing' that do not know about the internal
> implementation (code path). It may or may not share the same code path
> today -- same is true in the future.
>

While there's merit in both approaches, practically speaking it seems
difficult to test in that way in general - any feature could interact with
any other. The LLVM regression suite is far more narrowly targeted than
that - we don't test combinations of optimizations, for example - we test
each optimization in isolation. The same would be true of two independent
features on an interface such as this, I think.


>
> >
> >>
> >> >
> >> >>
> >> >> +};
> >> >> +
> >> >> +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?
>
> Sorry I am not sure what you mean here.
>

Is there some part of Clang that is responsible for generating both
implicitly defaulted and explicitly defaulted move/copy ops that could
handle this case, rather than apparently handling the implicit and explicit
cases separately (it seems they're being handled separately if the implicit
case worked before and you added code (rather than moving code) to fix the
explicit case - it sounds like we now have two bits of code, one for
implicit and one for explicit - perhaps there's a single bit of code that
we could write that would handle both?)

- David


>
> David
>
> >
> >>
> >>
> >> > 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/781afa9f/attachment.html>