<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Feb 8, 2016 at 3:46 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 3:35 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
><br>
> On Mon, Feb 8, 2016 at 3:21 PM, Xinliang David Li <<a href="mailto:davidxl@google.com">davidxl@google.com</a>><br>
> wrote:<br>
>><br>
>> On Mon, Feb 8, 2016 at 3:17 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>> ><br>
>> ><br>
>> > On Mon, Feb 8, 2016 at 12:07 PM, Xinliang David Li <<a href="mailto:davidxl@google.com">davidxl@google.com</a>><br>
>> > wrote:<br>
>> >><br>
>> >> On Mon, Feb 8, 2016 at 11:39 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>><br>
>> >> 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<br>
>> >> >> x86_64-unknown-linux-gnu<br>
>> >> >> -main-file-name def-assignop.cpp -o - -emit-llvm<br>
>> >> >> -fprofile-instrument=clang<br>
>> >> >> | FileCheck --check-prefix=PGOGEN %s<br>
>> >> >> +// RUN: %clang_cc1 -x c++ -std=c++11 %s -triple<br>
>> >> >> x86_64-unknown-linux-gnu<br>
>> >> >> -main-file-name def-assignop.cpp -o - -emit-llvm<br>
>> >> >> -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>
>> >> Will change.<br>
>> >><br>
>> >> ><br>
>> >> > Also, neither of these are the move /constructor/, just the move<br>
>> >> > operator.<br>
>> >> > Not sure if Vedant just used the wrong terminology, or whether it's<br>
>> >> > worth<br>
>> >> > testing the move/copy ctors too, to check that they do the right<br>
>> >> > thing<br>
>> >> > as<br>
>> >><br>
>> >> I added tests for copy ctors, and plan to add move ctor test soon.<br>
>> >><br>
>> >> > well. (if all of these things use the same codepath, I don't see a<br>
>> >> > great<br>
>> >> > benefit in having separate tests for them (but you can add them here<br>
>> >> > if<br>
>> >> > you<br>
>> >> > like) - I'm just suggesting a manual verification in case those need<br>
>> >> > a<br>
>> >> > separate fix<br>
>> >><br>
>> >> 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>
>> ><br>
>> ><br>
>> > I wouldn't necessarily bother testing multiple instances of the same<br>
>> > codepath (so the copy and move ctor for example) - but 2 instances is no<br>
>> > big<br>
>> > deal (if there were several more, I might be inclined to just test one<br>
>> > as a<br>
>> > representative sample). I don't mind either way, though. The number is<br>
>> > small<br>
>> > & the test cases are arguably distinct.<br>
>><br>
>> Sorry I disagree with your general statement here. I treat such test<br>
>> cases as 'black box testing' that do not know about the internal<br>
>> implementation (code path). It may or may not share the same code path<br>
>> today -- same is true in the future.<br>
><br>
><br>
> While there's merit in both approaches, practically speaking it seems<br>
> difficult to test in that way in general - any feature could interact with<br>
> any other.<br>
<br>
</div></div>The language features are well specified -- so writing small test<br>
cases to cover them is a general accepted way of testing.<br></blockquote><div><br></div><div>I'm not sure I follow the distinction you're drawing between the middle end optimization tests and the features you're testing here. If the features are relatively independent, even within the same API/feature area, they're generally tested independently (even two features within a single middle end optimization - a test case is written to ensure that, say, ArgumentPromotion correctly handles debug info, and another that it correctly handles inalloca (or fp80, etc - just looking at test/Transforms/ArgumentPromotion) - but we don't test the matrix of combinations of these features)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
>The LLVM regression suite is far more narrowly targeted than that<br>
> - we don't test combinations of optimizations, for example - we test each<br>
> optimization in isolation. The same would be true of two independent<br>
> features on an interface such as this, I think.<br>
<br>
</span>This is a weakness of the test system -- a problem at a different dimension.<br></blockquote><div><br></div><div>If we want to have a discussion about the LLVM community testing methodology, that might be best taken up on llvm-dev (and clang-dev). But for now, I'd ask that tests in the lit regression suite are generally as isolated as possible and test one thing at a time.</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>
>> ><br>
>> >><br>
>> >> ><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>
>> >> 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>
>> ><br>
>> ><br>
>> > Hmm - are you sure there's no common codepath that would cover the<br>
>> > explicitly defaulted or implicitly defaulted ops together in one go?<br>
>><br>
>> Sorry I am not sure what you mean here.<br>
> Is there some part of Clang that is responsible for generating both<br>
> implicitly defaulted and explicitly defaulted move/copy ops that could<br>
> handle this case, rather than apparently handling the implicit and explicit<br>
> cases separately (it seems they're being handled separately if the implicit<br>
> case worked before and you added code (rather than moving code) to fix the<br>
> explicit case - it sounds like we now have two bits of code, one for<br>
> implicit and one for explicit - perhaps there's a single bit of code that we<br>
> could write that would handle both?)<br>
<br>
</span>The codegen paths are different -- otherwise as you commented, the<br>
implicit case would have been broken too.<br>
<br>
Refactoring FE code to handle both is probably beyond the scope of<br>
this fix.  Having a good test case here will exactly help avoid<br>
regression if that happens in the future.<br>
<span class="HOEnZb"><font color="#888888"><br>
David<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
><br>
> - David<br>
><br>
>><br>
>><br>
>> David<br>
>><br>
>> ><br>
>> >><br>
>> >><br>
>> >> > Or just any<br>
>> >> > compiler-generated ones? (you could drop these lines if it's about<br>
>> >> > any<br>
>> >> > compiler-generated ones, might be simpler/more obvious that it's not<br>
>> >> > about<br>
>> >> > the "= default" feature)<br>
>> >><br>
>> >> Other compiler generated ones are handled differently.<br>
>> >><br>
>> >> thanks,<br>
>> >><br>
>> >> David<br>
>> >><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<br>
>> >> >> including<br>
>> >> >> the<br>
>> >> >> +  // defaulted copy and move operators: A::operator=<br>
>> >> >> +  // COVMAP: @__llvm_coverage_mapping = {{.*}} { { i32, i32, i32,<br>
>> >> >> i32<br>
>> >> >> },<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<br>
>> >> > 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>
>> ><br>
>> ><br>
><br>
><br>
</div></div></blockquote></div><br></div></div>