On Mon, Feb 8, 2016 at 9:23 PM, Xinliang David Li <davi...@google.com>
wrote:
> Wrong in the sense the the coverage result for the default operators
> (the line where they are declared) is marked as if they are not called
> which can be confusing to the user.
>
Presumably a user would have the same problem with implicit ops - the class
header/name would be marked as if there was code that was not called there?
- David
>
> David
>
> On Mon, Feb 8, 2016 at 9:09 PM, David Blaikie <dblai...@gmail.com> wrote:
> >
> >
> > On Mon, Feb 8, 2016 at 9:00 PM, Xinliang David Li <davi...@google.com>
> > wrote:
> >>
> >> On Mon, Feb 8, 2016 at 8:46 PM, David Blaikie <dblai...@gmail.com>
> wrote:
> >> >
> >> > On Mon, Feb 8, 2016 at 7:39 PM, Xinliang David Li <davi...@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.
> >
> >
> > wrong in what way? Both functions (explicitly or implicitly defaulted)
> will
> > be emitted, with line tables (looks like the = defaulted one is
> attributed
> > to the line where the = default was written, and the implicitly defaulted
> > one is attributed to wherever the class name is written)
> >
> > - David
> >
> >>
> >>
> >> thanks,
> >>
> >> David
> >>
> >> >
> >> >>
> >> >>
> >> >> thanks,
> >> >>
> >> >> David
> >> >>
> >> >> On Mon, Feb 8, 2016 at 5:23 PM, David Blaikie <dblai...@gmail.com>
> >> >> wrote:
> >> >> >
> >> >> >
> >> >> > On Mon, Feb 8, 2016 at 5:05 PM, Xinliang David Li
> >> >> > <davi...@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 <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
