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

Mon Feb 8 15:52:31 PST 2016

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
> >> >> >>
> >> >> >
> >> >
> >> >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160208/0b748962/attachment.html>