<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Feb 8, 2016 at 12:07 PM, Xinliang David Li <span dir="ltr"><<a href="mailto:davidxl@google.com" target="_blank">davidxl@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Mon, Feb 8, 2016 at 11:39 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
><br>
> On Mon, Feb 8, 2016 at 9:25 AM, David Li via llvm-commits<br>
> <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
>><br>
>> davidxl updated this revision to Diff 47217.<br>
>> davidxl added a comment.<br>
>><br>
>> Simplified test case suggested by Vedant.<br>
>><br>
>><br>
>> <a href="http://reviews.llvm.org/D16947" rel="noreferrer" target="_blank">http://reviews.llvm.org/D16947</a><br>
>><br>
>> Files:<br>
>> lib/CodeGen/CGClass.cpp<br>
>> test/Profile/def-assignop.cpp<br>
>><br>
>> Index: test/Profile/def-assignop.cpp<br>
>> ===================================================================<br>
>> --- test/Profile/def-assignop.cpp<br>
>> +++ test/Profile/def-assignop.cpp<br>
>> @@ -0,0 +1,32 @@<br>
>> +// RUN: %clang_cc1 -x c++ -std=c++11 %s -triple x86_64-unknown-linux-gnu<br>
>> -main-file-name def-assignop.cpp -o - -emit-llvm -fprofile-instrument=clang<br>
>> | FileCheck --check-prefix=PGOGEN %s<br>
>> +// RUN: %clang_cc1 -x c++ -std=c++11 %s -triple x86_64-unknown-linux-gnu<br>
>> -main-file-name def-assignop.cpp -o - -emit-llvm -fprofile-instrument=clang<br>
>> -fcoverage-mapping | FileCheck --check-prefix=COVMAP %s<br>
>> +<br>
>> +struct B {<br>
>> + void operator=(const B &b) {}<br>
>> + void operator=(const B &&b) {}<br>
><br>
><br>
> Probably best to make these canonical to avoid confusion:<br>
><br>
> B &operator=(const B&);<br>
> B &operator=(B&&);<br>
><br>
> (& they don't need definitions - just declarations)<br>
<br>
</div></div>Will change.<br>
<span class=""><br>
><br>
> Also, neither of these are the move /constructor/, just the move operator.<br>
> Not sure if Vedant just used the wrong terminology, or whether it's worth<br>
> testing the move/copy ctors too, to check that they do the right thing as<br>
<br>
</span>I added tests for copy ctors, and plan to add move ctor test soon.<br>
<span class=""><br>
> well. (if all of these things use the same codepath, I don't see a great<br>
> benefit in having separate tests for them (but you can add them here if you<br>
> like) - I'm just suggesting a manual verification in case those need a<br>
> separate fix<br>
<br>
</span>the ctor and assignment op do not share the same path -- the ctor path<br>
is working as expected without the fix -- or do you mean there is no<br>
need to cover both copy and move variants?<br></blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class="">><br>
>><br>
>> +};<br>
>> +<br>
>> +struct A {<br>
>> + A &operator=(const A &) = default;<br>
><br>
><br>
> Is the fix/codepath specifically about explicitly defaulted ops?<br>
<br>
</span>yes -- explicitly defaulted. There are some test coverage already for<br>
implicitly declared ctors (but not assignment op -- probably worth<br>
adding some testing too).<br></blockquote><div><br></div><div>Hmm - are you sure there's no common codepath that would cover the explicitly defaulted or implicitly defaulted ops together in one go?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> Or just any<br>
> compiler-generated ones? (you could drop these lines if it's about any<br>
> compiler-generated ones, might be simpler/more obvious that it's not about<br>
> the "= default" feature)<br>
<br>
</span>Other compiler generated ones are handled differently.<br>
<br>
thanks,<br>
<br>
David<br>
<div class="HOEnZb"><div class="h5"><br>
><br>
>><br>
>> + // PGOGEN: define {{.*}}@_ZN1AaSERKS_(<br>
>> + // PGOGEN: %pgocount = load {{.*}} @__profc__ZN1AaSERKS_<br>
>> + // PGOGEN: {{.*}}add{{.*}}%pgocount, 1<br>
>> + // PGOGEN: store{{.*}}@__profc__ZN1AaSERKS_<br>
>> + A &operator=(A &&) = default;<br>
>><br>
>> + // PGOGEN: define {{.*}}@_ZN1AaSEOS_<br>
>> + // PGOGEN: %pgocount = load {{.*}} @__profc__ZN1AaSEOS_<br>
>> + // PGOGEN: {{.*}}add{{.*}}%pgocount, 1<br>
>> + // PGOGEN: store{{.*}}@__profc__ZN1AaSEOS_<br>
>> +<br>
>> + // Check that coverage mapping includes 6 function records including<br>
>> the<br>
>> + // defaulted copy and move operators: A::operator=<br>
>> + // COVMAP: @__llvm_coverage_mapping = {{.*}} { { i32, i32, i32, i32 },<br>
>> [5 x <{{.*}}>],<br>
>> + B b;<br>
>> +};<br>
>> +<br>
>> +int main() {<br>
>> + A a1, a2;<br>
>> + a1 = a2;<br>
>> + a2 = static_cast<A &&>(a1);<br>
><br>
><br>
> An option, though not necessarily better, would be to just take the address<br>
> of the special members:<br>
><br>
> auto (B::*x)(const B&) = &bar::operator=;<br>
> auto (B::*x)(B&&) = &bar::operator=;<br>
><br>
> In short, what I'm picturing, in total:<br>
><br>
> struct A {<br>
> A &operator=(const A&);<br>
> A &operator=(A&&);<br>
> };<br>
><br>
> struct B {<br>
> A a;<br>
> };<br>
><br>
> auto (B::*x)(const B&) = &B::operator=;<br>
> auto (B::*x)(B&&) = &B::operator=;<br>
><br>
> Also, this test should probably be in clang, since it's a clang code<br>
> change/fix.<br>
><br>
><br>
>><br>
>> + return 0;<br>
>> +}<br>
>> Index: lib/CodeGen/CGClass.cpp<br>
>> ===================================================================<br>
>> --- lib/CodeGen/CGClass.cpp<br>
>> +++ lib/CodeGen/CGClass.cpp<br>
>> @@ -1608,6 +1608,7 @@<br>
>><br>
>> LexicalScope Scope(*this, RootCS->getSourceRange());<br>
>><br>
>> + incrementProfileCounter(RootCS);<br>
>> AssignmentMemcpyizer AM(*this, AssignOp, Args);<br>
>> for (auto *I : RootCS->body())<br>
>> AM.emitAssignment(I);<br>
>><br>
>><br>
>><br>
>> _______________________________________________<br>
>> llvm-commits mailing list<br>
>> <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
>> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
>><br>
><br>
</div></div></blockquote></div><br></div></div>