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

[Xinliang David Li via cfe-commits]

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 <dblaikie at gmail.com> wrote:
>
>
> On Mon, Feb 8, 2016 at 4:31 PM, Xinliang David Li <davidxl at google.com>
> wrote:
>>
>> On Mon, Feb 8, 2016 at 4:05 PM, David Blaikie <dblaikie at gmail.com> wrote:
>> >
>> >
>> > On Mon, Feb 8, 2016 at 3:58 PM, Xinliang David Li <davidxl at 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 <dblaikie at gmail.com>
>> >> wrote:
>> >> >
>> >> >
>> >> > On Mon, Feb 8, 2016 at 3:46 PM, Xinliang David Li
>> >> > <davidxl at google.com>
>> >> > wrote:
>> >> >>
>> >> >> On Mon, Feb 8, 2016 at 3:35 PM, David Blaikie <dblaikie at gmail.com>
>> >> >> wrote:
>> >> >> >
>> >> >> >
>> >> >> > On Mon, Feb 8, 2016 at 3:21 PM, Xinliang David Li
>> >> >> > <davidxl at google.com>
>> >> >> > wrote:
>> >> >> >>
>> >> >> >> On Mon, Feb 8, 2016 at 3:17 PM, David Blaikie
>> >> >> >> <dblaikie at gmail.com>
>> >> >> >> wrote:
>> >> >> >> >
>> >> >> >> >
>> >> >> >> > On Mon, Feb 8, 2016 at 12:07 PM, Xinliang David Li
>> >> >> >> > <davidxl at google.com>
>> >> >> >> > wrote:
>> >> >> >> >>
>> >> >> >> >> 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?
>> >> >> >> >
>> >> >> >> >
>> >> >> >> > 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-commits at lists.llvm.org
>> >> >> >> >> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>> >> >> >> >> >>
>> >> >> >> >> >
>> >> >> >> >
>> >> >> >> >
>> >> >> >
>> >> >> >
>> >> >
>> >> >
>> >
>> >
>
>