ha! somehow I kept thinking you are referring to implicit declared ctors. From your test case, it is seems that the implicit copy/move op is also broken and is fixed by this patch too. That means a missing test case to me. Will update the case when verified.
thanks, David On Mon, Feb 8, 2016 at 4:58 PM, David Blaikie <dblai...@gmail.com> wrote: > > > On Mon, Feb 8, 2016 at 4:31 PM, Xinliang David Li <davi...@google.com> > wrote: >> >> On Mon, Feb 8, 2016 at 4:05 PM, David Blaikie <dblai...@gmail.com> wrote: >> > >> > >> > On Mon, Feb 8, 2016 at 3:58 PM, Xinliang David Li <davi...@google.com> >> > wrote: >> >> >> >> To be clear, you are suggesting breaking the test into two (one for >> >> copy, and one for move) ? I am totally fine with that. >> > >> > >> > Nah, no need to split the test case - we try to keep the number of test >> > files down (& group related tests into a single file) to reduce test >> > execution time (a non-trivial about of check time is spent in process >> > overhead). >> > >> >> >> >> I thought you >> >> suggested removing the testing of move/op case because they might >> >> share the same code path (clang's implementation) as the copy/op. >> > >> > >> > I was suggesting that two cases is no big deal whether you test both or >> > test >> > one if they're the same codepath - if there were 5/many more things that >> > shared the same codepath, I'd generally suggest testing a representative >> > sample (possibly just a single one) rather than testing every client of >> > the >> > same code. >> > >> > Feel free to leave the two here as-is. (though if we're talking about >> > test >> > granularity, it's probably worth just putting these cases in the same >> > file/type/etc as the ctor cases you mentioned were already covered) >> >> There is a balance somewhere: >> 1) for small test cases like this, the overhead mainly comes from test >> set up cost -- adding additional incremental test in the same file >> probably almost comes for free (in terms of cost). However >> 2) having too many cases grouped together also reduces the >> debuggability when some test fails. > > > 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)) > > Anyway, up to you - that part isn't something I'm terribly worried about > either way. > >> >> >> > >> > & I'm still curious/wondering if there's a common codepath that would >> > provide a simpler fix/code that solved both implicit and explicitly >> > defaulted ops. >> >> I may take a look at that when I find time -- but there is no guarantee :) > > > A quick test of putting "assert(false)" in > emitImplicitAssignmentOperatorBody and running Clang over this code: > > struct foo { > foo &operator=(const foo &); > }; > > struct bar { > foo f; > }; > > auto (bar::*x)(const bar&) = &bar::operator=; > > 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. > > 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? > > 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. > > - Dave > >> >> >> thanks, >> >> David >> >> >> >> > >> > - Dave >> > >> >> >> >> >> >> thanks, >> >> >> >> David >> >> >> >> On Mon, Feb 8, 2016 at 3:52 PM, David Blaikie <dblai...@gmail.com> >> >> wrote: >> >> > >> >> > >> >> > On Mon, Feb 8, 2016 at 3:46 PM, Xinliang David Li >> >> > <davi...@google.com> >> >> > wrote: >> >> >> >> >> >> On Mon, Feb 8, 2016 at 3:35 PM, David Blaikie <dblai...@gmail.com> >> >> >> wrote: >> >> >> > >> >> >> > >> >> >> > On Mon, Feb 8, 2016 at 3:21 PM, Xinliang David Li >> >> >> > <davi...@google.com> >> >> >> > wrote: >> >> >> >> >> >> >> >> On Mon, Feb 8, 2016 at 3:17 PM, David Blaikie >> >> >> >> <dblai...@gmail.com> >> >> >> >> wrote: >> >> >> >> > >> >> >> >> > >> >> >> >> > On Mon, Feb 8, 2016 at 12:07 PM, Xinliang David Li >> >> >> >> > <davi...@google.com> >> >> >> >> > wrote: >> >> >> >> >> >> >> >> >> >> On Mon, Feb 8, 2016 at 11:39 AM, David Blaikie >> >> >> >> >> <dblai...@gmail.com> >> >> >> >> >> wrote: >> >> >> >> >> > >> >> >> >> >> > >> >> >> >> >> > On Mon, Feb 8, 2016 at 9:25 AM, David Li via llvm-commits >> >> >> >> >> > <llvm-comm...@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. >> >> >> > >> >> >> > >> >> >> > While there's merit in both approaches, practically speaking it >> >> >> > seems >> >> >> > difficult to test in that way in general - any feature could >> >> >> > interact >> >> >> > with >> >> >> > any other. >> >> >> >> >> >> The language features are well specified -- so writing small test >> >> >> cases to cover them is a general accepted way of testing. >> >> > >> >> > >> >> > 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) >> >> > >> >> >> >> >> >> >> >> >> >The LLVM regression suite is far more narrowly targeted than that >> >> >> > - we don't test combinations of optimizations, for example - we >> >> >> > test >> >> >> > each >> >> >> > optimization in isolation. The same would be true of two >> >> >> > independent >> >> >> > features on an interface such as this, I think. >> >> >> >> >> >> This is a weakness of the test system -- a problem at a different >> >> >> dimension. >> >> > >> >> > >> >> > 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. >> >> > >> >> >> >> >> >> >> >> >> > >> >> >> >> >> >> >> >> >> >> >> >> > >> >> >> >> >> >> >> >> >> >> > >> >> >> >> >> >> >> >> >> >> >> >> +}; >> >> >> >> >> >> + >> >> >> >> >> >> +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. >> >> >> > Is there some part of Clang that is responsible for generating >> >> >> > both >> >> >> > implicitly defaulted and explicitly defaulted move/copy ops that >> >> >> > could >> >> >> > handle this case, rather than apparently handling the implicit and >> >> >> > explicit >> >> >> > cases separately (it seems they're being handled separately if the >> >> >> > implicit >> >> >> > case worked before and you added code (rather than moving code) to >> >> >> > fix >> >> >> > the >> >> >> > explicit case - it sounds like we now have two bits of code, one >> >> >> > for >> >> >> > implicit and one for explicit - perhaps there's a single bit of >> >> >> > code >> >> >> > that we >> >> >> > could write that would handle both?) >> >> >> >> >> >> The codegen paths are different -- otherwise as you commented, the >> >> >> implicit case would have been broken too. >> >> >> >> >> >> Refactoring FE code to handle both is probably beyond the scope of >> >> >> this fix. Having a good test case here will exactly help avoid >> >> >> regression if that happens in the future. >> >> >> >> >> >> David >> >> >> >> >> >> > >> >> >> > - David >> >> >> > >> >> >> >> >> >> >> >> >> >> >> >> 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-comm...@lists.llvm.org >> >> >> >> >> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits >> >> >> >> >> >> >> >> >> >> >> > >> >> >> >> > >> >> >> >> > >> >> >> > >> >> >> > >> >> > >> >> > >> > >> > > > _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits