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

Mon Feb 8 21:00:57 PST 2016

On Mon, Feb 8, 2016 at 8:46 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
> On Mon, Feb 8, 2016 at 7:39 PM, Xinliang David Li <davidxl at google.com>
> wrote:
>>
>> I took a look at the problem. The implicitly defaulted operators
>> should not be instrumented as specified -- I actually I just added the
>> new test case for that (checking profile counter not generated) right
>> after my previous reply and it still passes with this patch. The
>> reason is that those operators have 'implicit' bit set, and profile
>> instrumentation in FE is implemented in two stages: 1) counter
>> assignment; 2) actual transformation.  For methods with implicit bit
>> set, step 1) is skipped as designed, so step 2) simply becomes a
>> no-op.
>>
>> In short, the test case still needs explicit '=default', and the
>> implicit case is covered elsewhere.
>
>
> OK, thanks for the explanation!
>
> Why is that the case, though - why would an implicitly default function be
> any different from a profile (& profile-guided-optimization) perspective
> from an explicitly defaulted one?

There are two factors to consider -- PGO and coverage testing.
Implicitly declared functions are usually small/single BB so
instrumenting them does not bring too much benefit (they will be
inlined most of the cases any way) while increasing instrumentation
overhead. They are not needed for coveraging test either (as there are
no source lines to cover). This is a good tuning heuristic in most
cases, but can get wrong sometimes (IR based late instrumentation is
more focused on performance thus not depending on this tuning).

Explicitly defaulted ones are different in the sense that if they are
not instrumented, the coverage result will be wrong.

thanks,

David

>
>>
>>
>> thanks,
>>
>> David
>>
>> On Mon, Feb 8, 2016 at 5:23 PM, David Blaikie <dblaikie at gmail.com> wrote:
>> >
>> >
>> > On Mon, Feb 8, 2016 at 5:05 PM, Xinliang David Li <davidxl at google.com>
>> > wrote:
>> >>
>> >> ha! somehow I kept thinking you are referring to implicit declared
>> >> ctors.
>> >
>> >
>> > Ah, glad we figured out the disconnect - thanks for bearing with me!
>> >
>> >>
>> >>
>> >> 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.
>> >
>> >
>> > Again, this is a case where I'd probably just simplify the test, as I
>> > asked
>> > earlier in the thread (I asked if it mattered if the op was explicitly
>> > or
>> > implicitly defaulted (& your response: "> 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).")
>> >
>> > So I'd just simplify the test by removing the "= default" - I don't
>> > think
>> > there's value in testing both the explicit default and implicit default
>> > if
>> > it's just the default-y-ness that's relevant here. Otherwise we could
>> > end up
>> > testing all sorts of ways of writing/interacting with dtors which
>> > wouldn't
>> > be relevant to the code/fix/etc.
>> >
>> > This seems like the obvious test for the behavior:
>> >
>> > struct foo {
>> >   // non-trivial ops
>> >   foo &operator=(const foo&);
>> >   foo &operator=(foo&&);
>> > };
>> >
>> > struct bar {
>> >   foo f; // or derive bar from foo, but I think the member version is
>> > simpler
>> > };
>> >
>> > // force emission of bar's implicit special members, one way or another:
>> > bar &(bar::*x)(const bar&) = &bar::operator=;
>> > bar &(bar::*x)(bar&&) = &bar::operator=;
>> >
>> > (or just call them as you had in your test case - but that produces more
>> > code, etc in the module, extra functions/profile counters/etc)
>> >
>> > - Dave
>> >
>> >>
>> >>
>> >> 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
>> >> >> >> >> >> >> >>
>> >> >> >> >> >> >> >
>> >> >> >> >> >> >
>> >> >> >> >> >> >
>> >> >> >> >> >
>> >> >> >> >> >
>> >> >> >> >
>> >> >> >> >
>> >> >> >
>> >> >> >
>> >> >
>> >> >
>> >
>> >
>
>