<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Feb 8, 2016 at 4:31 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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span class="">On Mon, Feb 8, 2016 at 4:05 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
><br>
> On Mon, Feb 8, 2016 at 3:58 PM, Xinliang David Li <<a href="mailto:davidxl@google.com">davidxl@google.com</a>><br>
> wrote:<br>
>><br>
>> To be clear, you are suggesting breaking the test into two (one for<br>
>> copy, and one for move) ? I am totally fine with that.<br>
><br>
><br>
> Nah, no need to split the test case - we try to keep the number of test<br>
> files down (& group related tests into a single file) to reduce test<br>
> execution time (a non-trivial about of check time is spent in process<br>
> overhead).<br>
><br>
>><br>
>>   I thought you<br>
>> suggested removing the testing of move/op case because they might<br>
>> share the same code path (clang's implementation) as the copy/op.<br>
><br>
><br>
> I was suggesting that two cases is no big deal whether you test both or test<br>
> one if they're the same codepath - if there were 5/many more things that<br>
> shared the same codepath, I'd generally suggest testing a representative<br>
> sample (possibly just a single one) rather than testing every client of the<br>
> same code.<br>
><br>
> Feel free to leave the two here as-is. (though if we're talking about test<br>
> granularity, it's probably worth just putting these cases in the same<br>
> file/type/etc as the ctor cases you mentioned were already covered)<br>
<br>
</span>There is a balance somewhere:<br>
1) for small test cases like this, the overhead mainly comes from test<br>
set up cost -- adding additional incremental test in the same file<br>
probably almost comes for free (in terms of cost). However<br>
2) having too many cases grouped together also reduces the<br>
debuggability when some test fails.<br></blockquote><div><br></div><div>Yep, for sure. In this case, testing the ctors and assignment ops in one file's probably not a bad tradeoff (you can see how Clang groups its tests - a file per language feature in many cases, exploring the myriad ways the feature can be used - this doesn't always work spectacularly (when you can't order the IR emission to happen mostly in the order that the source is written (rather being interleaved))<br><br>Anyway, up to you - that part isn't something I'm terribly worried about either way.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span class=""><br>
><br>
> & I'm still curious/wondering if there's a common codepath that would<br>
> provide a simpler fix/code that solved both implicit and explicitly<br>
> defaulted ops.<br>
<br>
</span>I may take a look at that when I find time -- but there is no guarantee :)<br></blockquote><div><br>A quick test of putting "assert(false)" in emitImplicitAssignmentOperatorBody and running Clang over this code:<br><br><div>struct foo {</div><div>  foo &operator=(const foo &);</div><div>};</div><div><br></div><div>struct bar {</div><div>  foo f;</div><div>};</div><div><br></div><div>auto (bar::*x)(const bar&) = &bar::operator=;<br><br>Fires the assertion - this seems to me to indicate that the codepath you changed is used for both the explicitly (based on the change fixing your test case) and implicitly defaulted (based on my test case) cases.<br><br>Is it possible that you end up with duplicate counters by accident in this path, then? Or at least that whatever codepath was handling the implicitly defaulted ones is now redundant with this one?<br><br>Actually, so far as I can tell this doesn't work for implicitly defaulted move ops (the above test case doesn't have an add pgocount in it) - perhaps I'm missing something/doing it wrong? or was just not communicating clearly regarding explicit versus implicitly defaulted special members.<br><br>- Dave</div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
thanks,<br>
<br>
David<br>
<div class=""><div class="h5"><br>
<br>
<br>
><br>
> - Dave<br>
><br>
>><br>
>><br>
>> thanks,<br>
>><br>
>> David<br>
>><br>
>> On Mon, Feb 8, 2016 at 3:52 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>> ><br>
>> ><br>
>> > On Mon, Feb 8, 2016 at 3:46 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:35 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>><br>
>> >> wrote:<br>
>> >> ><br>
>> >> ><br>
>> >> > On Mon, Feb 8, 2016 at 3:21 PM, Xinliang David Li<br>
>> >> > <<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>><br>
>> >> >> wrote:<br>
>> >> >> ><br>
>> >> >> ><br>
>> >> >> > On Mon, Feb 8, 2016 at 12:07 PM, Xinliang David Li<br>
>> >> >> > <<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<br>
>> >> >> >> <<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>
>> >> >> >> >><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<br>
>> >> >> >> > move<br>
>> >> >> >> > operator.<br>
>> >> >> >> > Not sure if Vedant just used the wrong terminology, or whether<br>
>> >> >> >> > it's<br>
>> >> >> >> > worth<br>
>> >> >> >> > testing the move/copy ctors too, to check that they do the<br>
>> >> >> >> > right<br>
>> >> >> >> > thing<br>
>> >> >> >> > as<br>
>> >> >> >><br>
>> >> >> >> I added tests for copy ctors, and plan to add move ctor test<br>
>> >> >> >> soon.<br>
>> >> >> >><br>
>> >> >> >> > well. (if all of these things use the same codepath, I don't<br>
>> >> >> >> > see a<br>
>> >> >> >> > great<br>
>> >> >> >> > benefit in having separate tests for them (but you can add them<br>
>> >> >> >> > here<br>
>> >> >> >> > if<br>
>> >> >> >> > you<br>
>> >> >> >> > like) - I'm just suggesting a manual verification in case those<br>
>> >> >> >> > need<br>
>> >> >> >> > a<br>
>> >> >> >> > separate fix<br>
>> >> >> >><br>
>> >> >> >> the ctor and assignment op do not share the same path -- the ctor<br>
>> >> >> >> path<br>
>> >> >> >> is working as expected without the fix -- or do you mean there is<br>
>> >> >> >> no<br>
>> >> >> >> need to cover both copy and move variants?<br>
>> >> >> ><br>
>> >> >> ><br>
>> >> >> > I wouldn't necessarily bother testing multiple instances of the<br>
>> >> >> > same<br>
>> >> >> > codepath (so the copy and move ctor for example) - but 2 instances<br>
>> >> >> > is<br>
>> >> >> > no<br>
>> >> >> > big<br>
>> >> >> > deal (if there were several more, I might be inclined to just test<br>
>> >> >> > one<br>
>> >> >> > as a<br>
>> >> >> > representative sample). I don't mind either way, though. The<br>
>> >> >> > number<br>
>> >> >> > 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<br>
>> >> >> 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<br>
>> >> > with<br>
>> >> > any other.<br>
>> >><br>
>> >> The language features are well specified -- so writing small test<br>
>> >> cases to cover them is a general accepted way of testing.<br>
>> ><br>
>> ><br>
>> > I'm not sure I follow the distinction you're drawing between the middle<br>
>> > end<br>
>> > optimization tests and the features you're testing here. If the features<br>
>> > are<br>
>> > relatively independent, even within the same API/feature area, they're<br>
>> > generally tested independently (even two features within a single middle<br>
>> > end<br>
>> > optimization - a test case is written to ensure that, say,<br>
>> > ArgumentPromotion<br>
>> > correctly handles debug info, and another that it correctly handles<br>
>> > inalloca<br>
>> > (or fp80, etc - just looking at test/Transforms/ArgumentPromotion) - but<br>
>> > we<br>
>> > don't test the matrix of combinations of these features)<br>
>> ><br>
>> >><br>
>> >><br>
>> >> >The LLVM regression suite is far more narrowly targeted than that<br>
>> >> > - we don't test combinations of optimizations, for example - we test<br>
>> >> > 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>
>> >> This is a weakness of the test system -- a problem at a different<br>
>> >> dimension.<br>
>> ><br>
>> ><br>
>> > If we want to have a discussion about the LLVM community testing<br>
>> > methodology, that might be best taken up on llvm-dev (and clang-dev).<br>
>> > But<br>
>> > for now, I'd ask that tests in the lit regression suite are generally as<br>
>> > isolated as possible and test one thing at a time.<br>
>> ><br>
>> >><br>
>> >><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<br>
>> >> >> >> > ops?<br>
>> >> >> >><br>
>> >> >> >> yes -- explicitly defaulted. There are some test coverage already<br>
>> >> >> >> for<br>
>> >> >> >> implicitly declared ctors (but not assignment op -- probably<br>
>> >> >> >> 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<br>
>> >> >> > 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<br>
>> >> > could<br>
>> >> > handle this case, rather than apparently handling the implicit and<br>
>> >> > explicit<br>
>> >> > cases separately (it seems they're being handled separately if the<br>
>> >> > implicit<br>
>> >> > case worked before and you added code (rather than moving code) to<br>
>> >> > fix<br>
>> >> > 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<br>
>> >> > that we<br>
>> >> > could write that would handle both?)<br>
>> >><br>
>> >> 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>
>> >><br>
>> >> David<br>
>> >><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<br>
>> >> >> >> > about<br>
>> >> >> >> > any<br>
>> >> >> >> > compiler-generated ones, might be simpler/more obvious that<br>
>> >> >> >> > it's<br>
>> >> >> >> > 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,<br>
>> >> >> >> >> 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<br>
>> >> >> >> > 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<br>
>> >> >> >> > code<br>
>> >> >> >> > change/fix.<br>
>> >> >> >> ><br>
>> >> >> >> ><br>
>> >> >> >> >><br>
>> >> >> >> >> +  return 0;<br>
>> >> >> >> >> +}<br>
>> >> >> >> >> Index: lib/CodeGen/CGClass.cpp<br>
>> >> >> >> >><br>
>> >> >> >> >><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>
>> ><br>
>> ><br>
><br>
><br>
</div></div></blockquote></div><br></div></div>