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

[Xinliang David Li via cfe-commits]

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

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