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

[Xinliang David Li via cfe-commits]

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.

>
>>
>> >
>> >>
>> >> +};
>> >> +
>> >> +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.

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
>> >>
>> >
>
>