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

[Xinliang David Li via cfe-commits]

On Tue, Feb 9, 2016 at 11:30 AM, David Blaikie  wrote:
>
>
> On Tue, Feb 9, 2016 at 11:26 AM, Xinliang David Li 
> wrote:
>>
>> On Tue, Feb 9, 2016 at 11:14 AM, David Blaikie  wrote:
>> >
>> >
>> > On Mon, Feb 8, 2016 at 9:23 PM, Xinliang David Li 
>> > 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?
>>
>> that would be confusing though -- as data of many implicitly declared
>> functions will be lumped together and user won't know what that is .
>
>
> Presumably it's still going to be confusing, though - the line table will
> record that line and the counter won't be there, so the header of the type
> will be marked in red & a user worried about coverage may go through some
> contortions to try to discover and cover that code, no? (even though it may
> already be covered & is just being reported incorrectly due to their being
> no counters)

coverage mapping does not use the debug line table for this purpose --
it encodes line info in source regions of its own format. Marking
class header as read will be super confusing if it happens (but does
not).

Further discussion is welcome, but I am going to commit this change soon.

thanks,

David

>
>>
>>
>> David
>>
>> >
>> > - David
>> >
>> >>
>> >>
>> >> David
>> >>
>> >> On Mon, Feb 8, 2016 at 9:09 PM, David Blaikie 
>> >> wrote:
>> >> >
>> >> >
>> >> > On Mon, Feb 8, 2016 at 9:00 PM, Xinliang David Li
>> >> > 
>> >> > wrote:
>> >> >>
>> >> >> On Mon, Feb 8, 2016 at 8:46 PM, David Blaikie 
>> >> >> wrote:
>> >> >> >
>> >> >> > On Mon, Feb 8, 2016 at 7:39 PM, Xinliang David Li
>> >> >> > 
>> >> >> > 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
>> >> >> >> 
>> >> >> >> wrote:
>> >> >> >> >
>> >> >> >> >
>> >> >> >> > On Mon, Feb 8, 2016 at 5:05 PM, Xinliang David Li
>> >> >> >> > 
>> >> >> >> > wrote:
>> >> >> >> >>
>> >> >> >> >> ha! somehow I kept thinking you are referring to implicit
>> >> >> >> >> declared
>> >> >> >> >> ctors.
>> >> 

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

[Xinliang David Li via cfe-commits]

On Tue, Feb 9, 2016 at 11:14 AM, David Blaikie  wrote:
>
>
> On Mon, Feb 8, 2016 at 9:23 PM, Xinliang David Li 
> 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?

that would be confusing though -- as data of many implicitly declared
functions will be lumped together and user won't know what that is .

David

>
> - David
>
>>
>>
>> David
>>
>> On Mon, Feb 8, 2016 at 9:09 PM, David Blaikie  wrote:
>> >
>> >
>> > On Mon, Feb 8, 2016 at 9:00 PM, Xinliang David Li 
>> > wrote:
>> >>
>> >> On Mon, Feb 8, 2016 at 8:46 PM, David Blaikie 
>> >> wrote:
>> >> >
>> >> > On Mon, Feb 8, 2016 at 7:39 PM, Xinliang David Li
>> >> > 
>> >> > 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 
>> >> >> wrote:
>> >> >> >
>> >> >> >
>> >> >> > On Mon, Feb 8, 2016 at 5:05 PM, Xinliang David Li
>> >> >> > 
>> >> >> > 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
>> 

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

[David Blaikie via cfe-commits]

On Mon, Feb 8, 2016 at 9:23 PM, Xinliang David Li 
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  wrote:
> >
> >
> > On Mon, Feb 8, 2016 at 9:00 PM, Xinliang David Li 
> > wrote:
> >>
> >> On Mon, Feb 8, 2016 at 8:46 PM, David Blaikie 
> wrote:
> >> >
> >> > On Mon, Feb 8, 2016 at 7:39 PM, Xinliang David Li  >
> >> > 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 
> >> >> wrote:
> >> >> >
> >> >> >
> >> >> > On Mon, Feb 8, 2016 at 5:05 PM, Xinliang David Li
> >> >> > 
> >> >> > 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 =(const foo&);
> >> >> >   foo =(foo&&);
> >> >> > };
> >> >> >
> >> >> > struct bar {
> >> >> >   foo f; // or derive bar from foo, but I think the member version
> is
> 

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

[David Li via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL260270: [PGO] Fix issue: explicitly defaulted assignop is 
not profiled (authored by davidxl).

Changed prior to commit:
  http://reviews.llvm.org/D16947?vs=47239=47351#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D16947

Files:
  cfe/trunk/lib/CodeGen/CGClass.cpp
  cfe/trunk/test/Profile/def-assignop.cpp

Index: cfe/trunk/test/Profile/def-assignop.cpp
===
--- cfe/trunk/test/Profile/def-assignop.cpp
+++ cfe/trunk/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 {
+  B& operator=(const B );
+  B& operator=(const B &);
+};
+
+struct A {
+  A =(const A &) = default;
+  // PGOGEN: define {{.*}}@_ZN1AaSERKS_(
+  // PGOGEN: %pgocount = load {{.*}} @__profc__ZN1AaSERKS_
+  // PGOGEN: {{.*}}add{{.*}}%pgocount, 1
+  // PGOGEN: store{{.*}}@__profc__ZN1AaSERKS_
+  A =(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 }, [3 x 
<{{.*}}>],
+  B b;
+};
+
+int main() {
+  A a1, a2;
+  a1 = a2;
+  a2 = static_cast(a1);
+  return 0;
+}
Index: cfe/trunk/lib/CodeGen/CGClass.cpp
===
--- cfe/trunk/lib/CodeGen/CGClass.cpp
+++ cfe/trunk/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);


Index: cfe/trunk/test/Profile/def-assignop.cpp
===
--- cfe/trunk/test/Profile/def-assignop.cpp
+++ cfe/trunk/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 {
+  B& operator=(const B );
+  B& operator=(const B &);
+};
+
+struct A {
+  A =(const A &) = default;
+  // PGOGEN: define {{.*}}@_ZN1AaSERKS_(
+  // PGOGEN: %pgocount = load {{.*}} @__profc__ZN1AaSERKS_
+  // PGOGEN: {{.*}}add{{.*}}%pgocount, 1
+  // PGOGEN: store{{.*}}@__profc__ZN1AaSERKS_
+  A =(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 }, [3 x <{{.*}}>],
+  B b;
+};
+
+int main() {
+  A a1, a2;
+  a1 = a2;
+  a2 = static_cast(a1);
+  return 0;
+}
Index: cfe/trunk/lib/CodeGen/CGClass.cpp
===
--- cfe/trunk/lib/CodeGen/CGClass.cpp
+++ cfe/trunk/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);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Blaikie via cfe-commits]

dblaikie added inline comments.


Comment at: cfe/trunk/test/Profile/def-assignop.cpp:27
@@ +26,3 @@
+
+int main() {
+  A a1, a2;

This doesn't need to be main or have an int return. Just make it a void 
function (with some generic name) & drop the "return 0" to keep it simple.


Repository:
  rL LLVM

http://reviews.llvm.org/D16947



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Blaikie via cfe-commits]

On Tue, Feb 9, 2016 at 12:07 PM, David Li via llvm-commits <
llvm-comm...@lists.llvm.org> wrote:

> This revision was automatically updated to reflect the committed changes.
> Closed by commit rL260270: [PGO] Fix issue: explicitly defaulted assignop
> is not profiled (authored by davidxl).
>

In general, if you send something out for review it's best to wait for
explicit sign-off before committing. Sorry I hadn't taken a moment to look
at the clang test you had added - again, got a bit lost in the noise of the
discussion.


>
> Changed prior to commit:
>   http://reviews.llvm.org/D16947?vs=47239=47351#toc
>
> Repository:
>   rL LLVM
>
> http://reviews.llvm.org/D16947
>
> Files:
>   cfe/trunk/lib/CodeGen/CGClass.cpp
>   cfe/trunk/test/Profile/def-assignop.cpp
>
> Index: cfe/trunk/test/Profile/def-assignop.cpp
> ===
> --- cfe/trunk/test/Profile/def-assignop.cpp
> +++ cfe/trunk/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 {
> +  B& operator=(const B );
> +  B& operator=(const B &);
> +};
> +
> +struct A {
> +  A =(const A &) = default;
> +  // PGOGEN: define {{.*}}@_ZN1AaSERKS_(
> +  // PGOGEN: %pgocount = load {{.*}} @__profc__ZN1AaSERKS_
> +  // PGOGEN: {{.*}}add{{.*}}%pgocount, 1
> +  // PGOGEN: store{{.*}}@__profc__ZN1AaSERKS_
> +  A =(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 },
> [3 x <{{.*}}>],
> +  B b;
> +};
> +
> +int main() {
> +  A a1, a2;
> +  a1 = a2;
> +  a2 = static_cast(a1);
> +  return 0;
> +}
> Index: cfe/trunk/lib/CodeGen/CGClass.cpp
> ===
> --- cfe/trunk/lib/CodeGen/CGClass.cpp
> +++ cfe/trunk/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

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

[David Blaikie via cfe-commits]

On Tue, Feb 9, 2016 at 11:26 AM, Xinliang David Li 
wrote:

> On Tue, Feb 9, 2016 at 11:14 AM, David Blaikie  wrote:
> >
> >
> > On Mon, Feb 8, 2016 at 9:23 PM, Xinliang David Li 
> > 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?
>
> that would be confusing though -- as data of many implicitly declared
> functions will be lumped together and user won't know what that is .
>

Presumably it's still going to be confusing, though - the line table will
record that line and the counter won't be there, so the header of the type
will be marked in red & a user worried about coverage may go through some
contortions to try to discover and cover that code, no? (even though it may
already be covered & is just being reported incorrectly due to their being
no counters)


>
> David
>
> >
> > - David
> >
> >>
> >>
> >> David
> >>
> >> On Mon, Feb 8, 2016 at 9:09 PM, David Blaikie 
> wrote:
> >> >
> >> >
> >> > On Mon, Feb 8, 2016 at 9:00 PM, Xinliang David Li  >
> >> > wrote:
> >> >>
> >> >> On Mon, Feb 8, 2016 at 8:46 PM, David Blaikie 
> >> >> wrote:
> >> >> >
> >> >> > On Mon, Feb 8, 2016 at 7:39 PM, Xinliang David Li
> >> >> > 
> >> >> > 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  >
> >> >> >> wrote:
> >> >> >> >
> >> >> >> >
> >> >> >> > On Mon, Feb 8, 2016 at 5:05 PM, Xinliang David Li
> >> >> >> > 
> >> >> >> > 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 

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

[David Blaikie via cfe-commits]

On Tue, Feb 9, 2016 at 11:41 AM, Xinliang David Li 
wrote:

> On Tue, Feb 9, 2016 at 11:30 AM, David Blaikie  wrote:
> >
> >
> > On Tue, Feb 9, 2016 at 11:26 AM, Xinliang David Li 
> > wrote:
> >>
> >> On Tue, Feb 9, 2016 at 11:14 AM, David Blaikie 
> wrote:
> >> >
> >> >
> >> > On Mon, Feb 8, 2016 at 9:23 PM, Xinliang David Li  >
> >> > 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?
> >>
> >> that would be confusing though -- as data of many implicitly declared
> >> functions will be lumped together and user won't know what that is .
> >
> >
> > Presumably it's still going to be confusing, though - the line table will
> > record that line and the counter won't be there, so the header of the
> type
> > will be marked in red & a user worried about coverage may go through some
> > contortions to try to discover and cover that code, no? (even though it
> may
> > already be covered & is just being reported incorrectly due to their
> being
> > no counters)
>
> coverage mapping does not use the debug line table for this purpose --
> it encodes line info in source regions of its own format. Marking
> class header as read will be super confusing if it happens (but does
> not).
>

Then why would it be a problem to omit the defaulted versions? It won't be
marked "as if they are not called" (it won't show up red) it'll just be
marked as if there's no code there, right?


> Further discussion is welcome, but I am going to commit this change soon.
>

As for the review - I'd still request that the test be in Clang, where the
code change is. Please defer adding tests like this to compiler-rt until
we've had some discussion with compiler-rt folks (I tried to rope Alexey
into one of these threads, not sure if it got lost in the noise), perhaps
on llvm-dev. Maybe I'm confused about what testing should go in
compiler-rt, but we can defer that to a discussion as there should be a
test in Clang regardless of whether there's also testing for this specific
feature in compiler-rt.

- David


>
> thanks,
>
> David
>
> >
> >>
> >>
> >> David
> >>
> >> >
> >> > - David
> >> >
> >> >>
> >> >>
> >> >> David
> >> >>
> >> >> On Mon, Feb 8, 2016 at 9:09 PM, David Blaikie 
> >> >> wrote:
> >> >> >
> >> >> >
> >> >> > On Mon, Feb 8, 2016 at 9:00 PM, Xinliang David Li
> >> >> > 
> >> >> > wrote:
> >> >> >>
> >> >> >> On Mon, Feb 8, 2016 at 8:46 PM, David Blaikie  >
> >> >> >> wrote:
> >> >> >> >
> >> >> >> > On Mon, Feb 8, 2016 at 7:39 PM, Xinliang David Li
> >> >> >> > 
> >> >> >> > 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 

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

[Xinliang David Li via cfe-commits]

On Tue, Feb 9, 2016 at 11:44 AM, David Blaikie  wrote:
>
>
> On Tue, Feb 9, 2016 at 11:41 AM, Xinliang David Li 
> wrote:
>>
>> On Tue, Feb 9, 2016 at 11:30 AM, David Blaikie  wrote:
>> >
>> >
>> > On Tue, Feb 9, 2016 at 11:26 AM, Xinliang David Li 
>> > wrote:
>> >>
>> >> On Tue, Feb 9, 2016 at 11:14 AM, David Blaikie 
>> >> wrote:
>> >> >
>> >> >
>> >> > On Mon, Feb 8, 2016 at 9:23 PM, Xinliang David Li
>> >> > 
>> >> > 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?
>> >>
>> >> that would be confusing though -- as data of many implicitly declared
>> >> functions will be lumped together and user won't know what that is .
>> >
>> >
>> > Presumably it's still going to be confusing, though - the line table
>> > will
>> > record that line and the counter won't be there, so the header of the
>> > type
>> > will be marked in red & a user worried about coverage may go through
>> > some
>> > contortions to try to discover and cover that code, no? (even though it
>> > may
>> > already be covered & is just being reported incorrectly due to their
>> > being
>> > no counters)
>>
>> coverage mapping does not use the debug line table for this purpose --
>> it encodes line info in source regions of its own format. Marking
>> class header as read will be super confusing if it happens (but does
>> not).
>
>
> Then why would it be a problem to omit the defaulted versions? It won't be
> marked "as if they are not called" (it won't show up red) it'll just be
> marked as if there's no code there, right?

This is subjective  -- but it is clear to me if a user explicitly
write a defaulted function, he/she would expect the function to be
covered in test. Class/struct tag on the hand, won't be expected to be
covered. By the way, we can do this one way or the other, but they
need to be consistent.

>
>>
>> Further discussion is welcome, but I am going to commit this change soon.
>
>
> As for the review - I'd still request that the test be in Clang, where the
> code change is.

Yes -- this patch only includes clang change.

 Please defer adding tests like this to compiler-rt until
> we've had some discussion with compiler-rt folks (I tried to rope Alexey
> into one of these threads, not sure if it got lost in the noise), perhaps on
> llvm-dev. Maybe I'm confused about what testing should go in compiler-rt,
> but we can defer that to a discussion as there should be a test in Clang
> regardless of whether there's also testing for this specific feature in
> compiler-rt.

I disagree. There are hundreds of such tests in compiler-rt, so I
don't see the point of holding off one more test case (or be a problem
for future batch porting).

David



>
> - David
>
>>
>>
>> thanks,
>>
>> David
>>
>> >
>> >>
>> >>
>> >> David
>> >>
>> >> >
>> >> > - David
>> >> >
>> >> >>
>> >> >>
>> >> >> David
>> >> >>
>> >> >> On Mon, Feb 8, 2016 at 9:09 PM, David Blaikie 
>> >> >> wrote:
>> >> >> >
>> >> >> >
>> >> >> > On Mon, Feb 8, 2016 at 9:00 PM, Xinliang David Li
>> >> >> > 
>> >> >> > wrote:
>> >> >> >>
>> >> >> >> On Mon, Feb 8, 2016 at 8:46 PM, David Blaikie
>> >> >> >> 
>> >> >> >> wrote:
>> >> >> >> >
>> >> >> >> > On Mon, Feb 8, 2016 at 7:39 PM, Xinliang David Li
>> >> >> >> > 
>> >> >> >> > 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
>> >> >> 

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

[David Blaikie via cfe-commits]

On Mon, Feb 8, 2016 at 9:00 PM, Xinliang David Li 
wrote:

> On Mon, Feb 8, 2016 at 8:46 PM, David Blaikie  wrote:
> >
> > On Mon, Feb 8, 2016 at 7:39 PM, Xinliang David Li 
> > 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 
> wrote:
> >> >
> >> >
> >> > On Mon, Feb 8, 2016 at 5:05 PM, Xinliang David Li  >
> >> > 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 =(const foo&);
> >> >   foo =(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&) = ::operator=;
> >> > bar &(bar::*x)(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 
> >> >> wrote:
> >> >> >
> >> >> >
> >> >> > On Mon, Feb 8, 2016 at 4:31 PM, Xinliang David Li
> >> >> > 
> >> >> > wrote:
> >> >> >>
> >> >> >> On Mon, Feb 8, 2016 at 4:05 PM, David Blaikie  >
> >> >> >> wrote:
> >> >> >> >
> >> >> >> >
> >> >> >> > On Mon, Feb 8, 2016 at 3:58 PM, Xinliang David Li
> >> >> >> > 
> >> >> >> > wrote:
> >> 

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

[Xinliang David Li via cfe-commits]

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.

David

On Mon, Feb 8, 2016 at 9:09 PM, David Blaikie  wrote:
>
>
> On Mon, Feb 8, 2016 at 9:00 PM, Xinliang David Li 
> wrote:
>>
>> On Mon, Feb 8, 2016 at 8:46 PM, David Blaikie  wrote:
>> >
>> > On Mon, Feb 8, 2016 at 7:39 PM, Xinliang David Li 
>> > 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 
>> >> wrote:
>> >> >
>> >> >
>> >> > On Mon, Feb 8, 2016 at 5:05 PM, Xinliang David Li
>> >> > 
>> >> > 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 =(const foo&);
>> >> >   foo =(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&) = ::operator=;
>> >> > bar &(bar::*x)(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 

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

[Xinliang David Li via cfe-commits]

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.

thanks,

David

On Mon, Feb 8, 2016 at 5:23 PM, David Blaikie  wrote:
>
>
> On Mon, Feb 8, 2016 at 5:05 PM, Xinliang David Li 
> 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 =(const foo&);
>   foo =(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&) = ::operator=;
> bar &(bar::*x)(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  wrote:
>> >
>> >
>> > On Mon, Feb 8, 2016 at 4:31 PM, Xinliang David Li 
>> > wrote:
>> >>
>> >> On Mon, Feb 8, 2016 at 4:05 PM, David Blaikie 
>> >> wrote:
>> >> >
>> >> >
>> >> > On Mon, Feb 8, 2016 at 3:58 PM, Xinliang David Li
>> >> > 
>> >> > 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 

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

[Xinliang David Li via cfe-commits]

On Mon, Feb 8, 2016 at 8:46 PM, David Blaikie  wrote:
>
> On Mon, Feb 8, 2016 at 7:39 PM, Xinliang David Li 
> 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  wrote:
>> >
>> >
>> > On Mon, Feb 8, 2016 at 5:05 PM, Xinliang David Li 
>> > 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 =(const foo&);
>> >   foo =(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&) = ::operator=;
>> > bar &(bar::*x)(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 
>> >> wrote:
>> >> >
>> >> >
>> >> > On Mon, Feb 8, 2016 at 4:31 PM, Xinliang David Li
>> >> > 
>> >> > wrote:
>> >> >>
>> >> >> On Mon, Feb 8, 2016 at 4:05 PM, David Blaikie 
>> >> >> wrote:
>> >> >> >
>> >> >> >
>> >> >> > On Mon, Feb 8, 2016 at 3:58 PM, Xinliang David Li
>> >> >> > 
>> >> >> > 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 

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

[David Li via cfe-commits]

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 ) {}
+  void operator=(const B &) {}
+};
+
+struct A {
+  A =(const A &) = default;
+  // PGOGEN: define {{.*}}@_ZN1AaSERKS_(
+  // PGOGEN: %pgocount = load {{.*}} @__profc__ZN1AaSERKS_
+  // PGOGEN: {{.*}}add{{.*}}%pgocount, 1
+  // PGOGEN: store{{.*}}@__profc__ZN1AaSERKS_
+  A =(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(a1);
+  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);


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 ) {}
+  void operator=(const B &) {}
+};
+
+struct A {
+  A =(const A &) = default;
+  // PGOGEN: define {{.*}}@_ZN1AaSERKS_(
+  // PGOGEN: %pgocount = load {{.*}} @__profc__ZN1AaSERKS_
+  // PGOGEN: {{.*}}add{{.*}}%pgocount, 1
+  // PGOGEN: store{{.*}}@__profc__ZN1AaSERKS_
+  A =(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(a1);
+  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);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Vedant Kumar via cfe-commits]

vsk added a comment.

Lgtm.


http://reviews.llvm.org/D16947



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Blaikie via cfe-commits]

This looks like a change to clang - could you test it in clang (& review it
on cfe-commits instead of llvm-commits)?

On Sat, Feb 6, 2016 at 11:57 AM, David Li via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> davidxl created this revision.
> davidxl added a reviewer: vsk.
> davidxl added subscribers: llvm-commits, cfe-commits.
>
> For compiler generated assignment operator that is not trivial (calling
> base class operator=()), Clang FE assign region counters to the function
> body but does not emit profile counter increment for the function entry.
> This leads to many problems:
>
> 1) the operator body does not have profile data generated leading to
> warning in profile-use
> 2) the size of the function body may be large and lack of profile data may
> lead to wrong inlining decisions
> 3) when FE assign region counters to the function, it also emit coverage
> mapping data for the function -- but it has no coverage data which is
> confusing (currently the llvm-cov tool will report malformed format (as the
> name of the operator is not put into the right name section).
>
> 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,34 @@
> +// RUN: %clang_cc1 -x c++ %s -triple x86_64-unknown-linux-gnu
> -main-file-name def-assignop.cpp -o - -emit-llvm -fprofile-instrument=clang
> | FileCheck --check-prefix=PGOGEN %s
> +
> +
> +struct B {
> +  int B;
> +  void *operator=(const struct B ) {
> +if (b2.B == 0) {
> +  B = b2.B + 1;
> +} else
> +  B = b2.B;
> +return this;
> +  }
> +};
> +
> +struct A : public B {
> +  A =(const A &) = default;
> +// PGOGEN: define {{.*}}@_ZN1AaSERKS_(
> +// PGOGEN: %pgocount = load {{.*}} @__profc__ZN1AaSERKS_
> +// PGOGEN: {{.*}}add{{.*}}%pgocount, 1
> +// PGOGEN: store{{.*}}@__profc__ZN1AaSERKS_
> +  int I;
> +  int J;
> +  int getI() { return I; }
> +};
> +
> +A aa;
> +int g;
> +int main() {
> +  A aa2;
> +  aa2 = aa;
> +
> +  g = aa2.getI();
> +  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);
>
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Blaikie via cfe-commits]

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 ) {}
> +  void operator=(const B &) {}
>

Probably best to make these canonical to avoid confusion:

B =(const B&);
B =(B&&);

(& they don't need definitions - just declarations)

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
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)


> +};
> +
> +struct A {
> +  A =(const A &) = default;
>

Is the fix/codepath specifically about explicitly defaulted ops? 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)


> +  // PGOGEN: define {{.*}}@_ZN1AaSERKS_(
> +  // PGOGEN: %pgocount = load {{.*}} @__profc__ZN1AaSERKS_
> +  // PGOGEN: {{.*}}add{{.*}}%pgocount, 1
> +  // PGOGEN: store{{.*}}@__profc__ZN1AaSERKS_
> +  A =(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(a1);
>

An option, though not necessarily better, would be to just take the address
of the special members:

auto (B::*x)(const B&) = ::operator=;
auto (B::*x)(B&&) = ::operator=;

In short, what I'm picturing, in total:

struct A {
  A =(const A&);
  A =(A&&);
};

struct B {
  A a;
};

auto (B::*x)(const B&) = ::operator=;
auto (B::*x)(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

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

[Xinliang David Li via cfe-commits]

On Mon, Feb 8, 2016 at 11:39 AM, David Blaikie  wrote:
>
>
> On Mon, Feb 8, 2016 at 9:25 AM, David Li via llvm-commits
>  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 ) {}
>> +  void operator=(const B &) {}
>
>
> Probably best to make these canonical to avoid confusion:
>
> B =(const B&);
> B =(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?
>
>>
>> +};
>> +
>> +struct A {
>> +  A =(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).

> 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 =(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(a1);
>
>
> An option, though not necessarily better, would be to just take the address
> of the special members:
>
> auto (B::*x)(const B&) = ::operator=;
> auto (B::*x)(B&&) = ::operator=;
>
> In short, what I'm picturing, in total:
>
> struct A {
>   A =(const A&);
>   A =(A&&);
> };
>
> struct B {
>   A a;
> };
>
> auto (B::*x)(const B&) = ::operator=;
> auto (B::*x)(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

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

[Xinliang David Li via cfe-commits]

Both cfe-commits and llvm-commits are cc'ed.

David

On Mon, Feb 8, 2016 at 11:29 AM, David Blaikie  wrote:
> This looks like a change to clang - could you test it in clang (& review it
> on cfe-commits instead of llvm-commits)?
>
> On Sat, Feb 6, 2016 at 11:57 AM, David Li via cfe-commits
>  wrote:
>>
>> davidxl created this revision.
>> davidxl added a reviewer: vsk.
>> davidxl added subscribers: llvm-commits, cfe-commits.
>>
>> For compiler generated assignment operator that is not trivial (calling
>> base class operator=()), Clang FE assign region counters to the function
>> body but does not emit profile counter increment for the function entry.
>> This leads to many problems:
>>
>> 1) the operator body does not have profile data generated leading to
>> warning in profile-use
>> 2) the size of the function body may be large and lack of profile data may
>> lead to wrong inlining decisions
>> 3) when FE assign region counters to the function, it also emit coverage
>> mapping data for the function -- but it has no coverage data which is
>> confusing (currently the llvm-cov tool will report malformed format (as the
>> name of the operator is not put into the right name section).
>>
>> 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,34 @@
>> +// RUN: %clang_cc1 -x c++ %s -triple x86_64-unknown-linux-gnu
>> -main-file-name def-assignop.cpp -o - -emit-llvm -fprofile-instrument=clang
>> | FileCheck --check-prefix=PGOGEN %s
>> +
>> +
>> +struct B {
>> +  int B;
>> +  void *operator=(const struct B ) {
>> +if (b2.B == 0) {
>> +  B = b2.B + 1;
>> +} else
>> +  B = b2.B;
>> +return this;
>> +  }
>> +};
>> +
>> +struct A : public B {
>> +  A =(const A &) = default;
>> +// PGOGEN: define {{.*}}@_ZN1AaSERKS_(
>> +// PGOGEN: %pgocount = load {{.*}} @__profc__ZN1AaSERKS_
>> +// PGOGEN: {{.*}}add{{.*}}%pgocount, 1
>> +// PGOGEN: store{{.*}}@__profc__ZN1AaSERKS_
>> +  int I;
>> +  int J;
>> +  int getI() { return I; }
>> +};
>> +
>> +A aa;
>> +int g;
>> +int main() {
>> +  A aa2;
>> +  aa2 = aa;
>> +
>> +  g = aa2.getI();
>> +  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);
>>
>>
>>
>> ___
>> cfe-commits mailing list
>> cfe-commits@lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Blaikie via cfe-commits]

ah, right, sorry about that - gmail didn't render cfe-commits on the to
line in the first email... weird.

Anyway, no need to include llvm-commits on clang-only changes.

On Mon, Feb 8, 2016 at 11:30 AM, Xinliang David Li 
wrote:

> Both cfe-commits and llvm-commits are cc'ed.
>
> David
>
> On Mon, Feb 8, 2016 at 11:29 AM, David Blaikie  wrote:
> > This looks like a change to clang - could you test it in clang (& review
> it
> > on cfe-commits instead of llvm-commits)?
> >
> > On Sat, Feb 6, 2016 at 11:57 AM, David Li via cfe-commits
> >  wrote:
> >>
> >> davidxl created this revision.
> >> davidxl added a reviewer: vsk.
> >> davidxl added subscribers: llvm-commits, cfe-commits.
> >>
> >> For compiler generated assignment operator that is not trivial (calling
> >> base class operator=()), Clang FE assign region counters to the function
> >> body but does not emit profile counter increment for the function entry.
> >> This leads to many problems:
> >>
> >> 1) the operator body does not have profile data generated leading to
> >> warning in profile-use
> >> 2) the size of the function body may be large and lack of profile data
> may
> >> lead to wrong inlining decisions
> >> 3) when FE assign region counters to the function, it also emit coverage
> >> mapping data for the function -- but it has no coverage data which is
> >> confusing (currently the llvm-cov tool will report malformed format (as
> the
> >> name of the operator is not put into the right name section).
> >>
> >> 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,34 @@
> >> +// RUN: %clang_cc1 -x c++ %s -triple x86_64-unknown-linux-gnu
> >> -main-file-name def-assignop.cpp -o - -emit-llvm
> -fprofile-instrument=clang
> >> | FileCheck --check-prefix=PGOGEN %s
> >> +
> >> +
> >> +struct B {
> >> +  int B;
> >> +  void *operator=(const struct B ) {
> >> +if (b2.B == 0) {
> >> +  B = b2.B + 1;
> >> +} else
> >> +  B = b2.B;
> >> +return this;
> >> +  }
> >> +};
> >> +
> >> +struct A : public B {
> >> +  A =(const A &) = default;
> >> +// PGOGEN: define {{.*}}@_ZN1AaSERKS_(
> >> +// PGOGEN: %pgocount = load {{.*}} @__profc__ZN1AaSERKS_
> >> +// PGOGEN: {{.*}}add{{.*}}%pgocount, 1
> >> +// PGOGEN: store{{.*}}@__profc__ZN1AaSERKS_
> >> +  int I;
> >> +  int J;
> >> +  int getI() { return I; }
> >> +};
> >> +
> >> +A aa;
> >> +int g;
> >> +int main() {
> >> +  A aa2;
> >> +  aa2 = aa;
> >> +
> >> +  g = aa2.getI();
> >> +  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);
> >>
> >>
> >>
> >> ___
> >> cfe-commits mailing list
> >> cfe-commits@lists.llvm.org
> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
> >>
> >
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Li via cfe-commits]

davidxl updated this revision to Diff 47239.
davidxl added a comment.

Further simplify tests according to David B's comment.


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 {
+  B& operator=(const B );
+  B& operator=(const B &);
+};
+
+struct A {
+  A =(const A &) = default;
+  // PGOGEN: define {{.*}}@_ZN1AaSERKS_(
+  // PGOGEN: %pgocount = load {{.*}} @__profc__ZN1AaSERKS_
+  // PGOGEN: {{.*}}add{{.*}}%pgocount, 1
+  // PGOGEN: store{{.*}}@__profc__ZN1AaSERKS_
+  A =(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 }, [3 x 
<{{.*}}>],
+  B b;
+};
+
+int main() {
+  A a1, a2;
+  a1 = a2;
+  a2 = static_cast(a1);
+  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);


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 {
+  B& operator=(const B );
+  B& operator=(const B &);
+};
+
+struct A {
+  A =(const A &) = default;
+  // PGOGEN: define {{.*}}@_ZN1AaSERKS_(
+  // PGOGEN: %pgocount = load {{.*}} @__profc__ZN1AaSERKS_
+  // PGOGEN: {{.*}}add{{.*}}%pgocount, 1
+  // PGOGEN: store{{.*}}@__profc__ZN1AaSERKS_
+  A =(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 }, [3 x <{{.*}}>],
+  B b;
+};
+
+int main() {
+  A a1, a2;
+  a1 = a2;
+  a2 = static_cast(a1);
+  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);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Xinliang David Li via cfe-commits]

On Mon, Feb 8, 2016 at 4:05 PM, David Blaikie  wrote:
>
>
> On Mon, Feb 8, 2016 at 3:58 PM, Xinliang David Li 
> 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.

>
> & 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 :)

thanks,

David



>
> - Dave
>
>>
>>
>> thanks,
>>
>> David
>>
>> On Mon, Feb 8, 2016 at 3:52 PM, David Blaikie  wrote:
>> >
>> >
>> > On Mon, Feb 8, 2016 at 3:46 PM, Xinliang David Li 
>> > wrote:
>> >>
>> >> On Mon, Feb 8, 2016 at 3:35 PM, David Blaikie 
>> >> wrote:
>> >> >
>> >> >
>> >> > On Mon, Feb 8, 2016 at 3:21 PM, Xinliang David Li
>> >> > 
>> >> > wrote:
>> >> >>
>> >> >> On Mon, Feb 8, 2016 at 3:17 PM, David Blaikie 
>> >> >> wrote:
>> >> >> >
>> >> >> >
>> >> >> > On Mon, Feb 8, 2016 at 12:07 PM, Xinliang David Li
>> >> >> > 
>> >> >> > wrote:
>> >> >> >>
>> >> >> >> On Mon, Feb 8, 2016 at 11:39 AM, David Blaikie
>> >> >> >> 
>> >> >> >> wrote:
>> >> >> >> >
>> >> >> >> >
>> >> >> >> > On Mon, Feb 8, 2016 at 9:25 AM, David Li via llvm-commits
>> >> >> >> >  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 ) {}
>> >> >> >> >> +  void operator=(const B &) {}
>> >> >> >> >
>> >> >> >> >
>> >> >> >> > Probably best to make these canonical to avoid confusion:
>> >> >> >> >
>> >> >> >> > B =(const B&);
>> >> >> >> > B =(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
>> >> 

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

[David Blaikie via cfe-commits]

On Mon, Feb 8, 2016 at 12:07 PM, Xinliang David Li 
wrote:

> On Mon, Feb 8, 2016 at 11:39 AM, David Blaikie  wrote:
> >
> >
> > On Mon, Feb 8, 2016 at 9:25 AM, David Li via llvm-commits
> >  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 ) {}
> >> +  void operator=(const B &) {}
> >
> >
> > Probably best to make these canonical to avoid confusion:
> >
> > B =(const B&);
> > B =(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.


> >
> >>
> >> +};
> >> +
> >> +struct A {
> >> +  A =(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?


>
> > 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 =(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(a1);
> >
> >
> > An option, though not necessarily better, would be to just take the
> address
> > of the special members:
> >
> > auto (B::*x)(const B&) = ::operator=;
> > auto (B::*x)(B&&) = ::operator=;
> >
> > In short, what I'm picturing, in total:
> >
> > struct A {
> >   A =(const A&);
> >   A =(A&&);
> > };
> >
> > struct B {
> >   A a;
> > };
> >
> > auto (B::*x)(const B&) = ::operator=;
> > auto (B::*x)(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());
> >>
> >> +  

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

[Xinliang David Li via cfe-commits]

On Mon, Feb 8, 2016 at 3:17 PM, David Blaikie  wrote:
>
>
> On Mon, Feb 8, 2016 at 12:07 PM, Xinliang David Li 
> wrote:
>>
>> On Mon, Feb 8, 2016 at 11:39 AM, David Blaikie  wrote:
>> >
>> >
>> > On Mon, Feb 8, 2016 at 9:25 AM, David Li via llvm-commits
>> >  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 ) {}
>> >> +  void operator=(const B &) {}
>> >
>> >
>> > Probably best to make these canonical to avoid confusion:
>> >
>> > B =(const B&);
>> > B =(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.

>
>>
>> >
>> >>
>> >> +};
>> >> +
>> >> +struct A {
>> >> +  A =(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.

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 =(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(a1);
>> >
>> >
>> > An option, though not necessarily better, would be to just take the
>> > address
>> > of the special members:
>> >
>> > auto (B::*x)(const B&) = ::operator=;
>> > auto (B::*x)(B&&) = ::operator=;
>> >
>> > In short, what I'm picturing, in total:
>> >
>> > struct A {
>> >   A =(const 

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

[David Blaikie via cfe-commits]

On Mon, Feb 8, 2016 at 3:21 PM, Xinliang David Li 
wrote:

> On Mon, Feb 8, 2016 at 3:17 PM, David Blaikie  wrote:
> >
> >
> > On Mon, Feb 8, 2016 at 12:07 PM, Xinliang David Li 
> > wrote:
> >>
> >> On Mon, Feb 8, 2016 at 11:39 AM, David Blaikie 
> wrote:
> >> >
> >> >
> >> > On Mon, Feb 8, 2016 at 9:25 AM, David Li via llvm-commits
> >> >  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 ) {}
> >> >> +  void operator=(const B &) {}
> >> >
> >> >
> >> > Probably best to make these canonical to avoid confusion:
> >> >
> >> > B =(const B&);
> >> > B =(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 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.


>
> >
> >>
> >> >
> >> >>
> >> >> +};
> >> >> +
> >> >> +struct A {
> >> >> +  A =(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?)

- 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 

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

[Xinliang David Li via cfe-commits]

On Mon, Feb 8, 2016 at 3:35 PM, David Blaikie  wrote:
>
>
> On Mon, Feb 8, 2016 at 3:21 PM, Xinliang David Li 
> wrote:
>>
>> On Mon, Feb 8, 2016 at 3:17 PM, David Blaikie  wrote:
>> >
>> >
>> > On Mon, Feb 8, 2016 at 12:07 PM, Xinliang David Li 
>> > wrote:
>> >>
>> >> On Mon, Feb 8, 2016 at 11:39 AM, David Blaikie 
>> >> wrote:
>> >> >
>> >> >
>> >> > On Mon, Feb 8, 2016 at 9:25 AM, David Li via llvm-commits
>> >> >  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 ) {}
>> >> >> +  void operator=(const B &) {}
>> >> >
>> >> >
>> >> > Probably best to make these canonical to avoid confusion:
>> >> >
>> >> > B =(const B&);
>> >> > B =(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.

>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.

>
>>
>>
>> >
>> >>
>> >> >
>> >> >>
>> >> >> +};
>> >> >> +
>> >> >> +struct A {
>> >> >> +  A =(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

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

[David Blaikie via cfe-commits]

On Mon, Feb 8, 2016 at 3:46 PM, Xinliang David Li 
wrote:

> On Mon, Feb 8, 2016 at 3:35 PM, David Blaikie  wrote:
> >
> >
> > On Mon, Feb 8, 2016 at 3:21 PM, Xinliang David Li 
> > wrote:
> >>
> >> On Mon, Feb 8, 2016 at 3:17 PM, David Blaikie 
> 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 
> >> >> wrote:
> >> >> >
> >> >> >
> >> >> > On Mon, Feb 8, 2016 at 9:25 AM, David Li via llvm-commits
> >> >> >  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 ) {}
> >> >> >> +  void operator=(const B &) {}
> >> >> >
> >> >> >
> >> >> > Probably best to make these canonical to avoid confusion:
> >> >> >
> >> >> > B =(const B&);
> >> >> > B =(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 

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

[Xinliang David Li via cfe-commits]

To be clear, you are suggesting breaking the test into two (one for
copy, and one for move) ? I am totally fine with that.  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.

thanks,

David

On Mon, Feb 8, 2016 at 3:52 PM, David Blaikie  wrote:
>
>
> On Mon, Feb 8, 2016 at 3:46 PM, Xinliang David Li 
> wrote:
>>
>> On Mon, Feb 8, 2016 at 3:35 PM, David Blaikie  wrote:
>> >
>> >
>> > On Mon, Feb 8, 2016 at 3:21 PM, Xinliang David Li 
>> > wrote:
>> >>
>> >> On Mon, Feb 8, 2016 at 3:17 PM, David Blaikie 
>> >> wrote:
>> >> >
>> >> >
>> >> > On Mon, Feb 8, 2016 at 12:07 PM, Xinliang David Li
>> >> > 
>> >> > wrote:
>> >> >>
>> >> >> On Mon, Feb 8, 2016 at 11:39 AM, David Blaikie 
>> >> >> wrote:
>> >> >> >
>> >> >> >
>> >> >> > On Mon, Feb 8, 2016 at 9:25 AM, David Li via llvm-commits
>> >> >> >  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 ) {}
>> >> >> >> +  void operator=(const B &) {}
>> >> >> >
>> >> >> >
>> >> >> > Probably best to make these canonical to avoid confusion:
>> >> >> >
>> >> >> > B =(const B&);
>> >> >> > B =(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, 

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

[David Blaikie via cfe-commits]

On Mon, Feb 8, 2016 at 3:58 PM, Xinliang David Li 
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)

& 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.

- Dave


>
> thanks,
>
> David
>
> On Mon, Feb 8, 2016 at 3:52 PM, David Blaikie  wrote:
> >
> >
> > On Mon, Feb 8, 2016 at 3:46 PM, Xinliang David Li 
> > wrote:
> >>
> >> On Mon, Feb 8, 2016 at 3:35 PM, David Blaikie 
> wrote:
> >> >
> >> >
> >> > On Mon, Feb 8, 2016 at 3:21 PM, Xinliang David Li  >
> >> > wrote:
> >> >>
> >> >> On Mon, Feb 8, 2016 at 3:17 PM, David Blaikie 
> >> >> wrote:
> >> >> >
> >> >> >
> >> >> > On Mon, Feb 8, 2016 at 12:07 PM, Xinliang David Li
> >> >> > 
> >> >> > 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
> >> >> >> >  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 ) {}
> >> >> >> >> +  void operator=(const B &) {}
> >> >> >> >
> >> >> >> >
> >> >> >> > Probably best to make these canonical to avoid confusion:
> >> >> >> >
> >> >> >> > B =(const B&);
> >> >> >> > B =(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
> 

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

[David Blaikie via cfe-commits]

On Mon, Feb 8, 2016 at 4:31 PM, Xinliang David Li 
wrote:

> On Mon, Feb 8, 2016 at 4:05 PM, David Blaikie  wrote:
> >
> >
> > On Mon, Feb 8, 2016 at 3:58 PM, Xinliang David Li 
> > 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 =(const foo &);
};

struct bar {
  foo f;
};

auto (bar::*x)(const 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 
> wrote:
> >> >
> >> >
> >> > On Mon, Feb 8, 2016 at 3:46 PM, Xinliang David Li  >
> >> > wrote:
> >> >>
> >> >> On Mon, Feb 8, 2016 at 3:35 PM, David Blaikie 
> >> >> wrote:
> >> >> >
> >> >> >
> >> >> > On Mon, Feb 8, 2016 at 3:21 PM, Xinliang David Li
> >> >> > 
> >> >> > wrote:
> >> >> >>
> >> >> >> On Mon, Feb 8, 2016 at 3:17 PM, David Blaikie  >
> >> >> >> wrote:
> >> >> >> >
> >> >> >> >
> >> >> >> > On Mon, Feb 8, 2016 at 12:07 PM, Xinliang David Li
> >> >> >> > 
> >> >> >> > wrote:
> >> >> >> >>
> >> >> >> >> On Mon, Feb 8, 2016 at 11:39 AM, David Blaikie
> >> >> >> >> 
> >> >> >> >> wrote:
> >> >> >> >> >
> >> >> >> >> >
> >> >> >> >> > On Mon, Feb 8, 2016 at 9:25 AM, David Li via llvm-commits
> >> >> >> >> >  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
> >> >> >> >> >>
> >> >> >> >> >>
> >> >> >> >> >>
> 

Re: [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  wrote:
>
>
> On Mon, Feb 8, 2016 at 4:31 PM, Xinliang David Li 
> wrote:
>>
>> On Mon, Feb 8, 2016 at 4:05 PM, David Blaikie  wrote:
>> >
>> >
>> > On Mon, Feb 8, 2016 at 3:58 PM, Xinliang David Li 
>> > 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 =(const foo &);
> };
>
> struct bar {
>   foo f;
> };
>
> auto (bar::*x)(const 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 
>> >> wrote:
>> >> >
>> >> >
>> >> > On Mon, Feb 8, 2016 at 3:46 PM, Xinliang David Li
>> >> > 
>> >> > wrote:
>> >> >>
>> >> >> On Mon, Feb 8, 2016 at 3:35 PM, David Blaikie 
>> >> >> wrote:
>> >> >> >
>> >> >> >
>> >> >> > On Mon, Feb 8, 2016 at 3:21 PM, Xinliang David Li
>> >> >> > 
>> >> >> > wrote:
>> >> >> >>
>> >> >> >> On Mon, Feb 8, 2016 at 3:17 PM, David Blaikie
>> >> >> >> 
>> >> >> >> wrote:
>> >> >> >> >
>> >> >> >> >
>> >> >> >> > On Mon, Feb 8, 2016 at 12:07 PM, Xinliang David Li
>> >> >> >> > 
>> >> >> >> > wrote:
>> >> >> >> >>
>> >> >> >> >> On Mon, Feb 8, 2016 at 11:39 AM, David Blaikie
>> >> >> >> >> 
>> >> >> >> >> wrote:
>> >> >> >> >> >
>> >> >> >> >> >
>> >> >> >> >> > On Mon, Feb 8, 2016 at 9:25 AM, David Li via llvm-commits
>> >> >> >> >> >  

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

[David Blaikie via cfe-commits]

On Mon, Feb 8, 2016 at 5:05 PM, Xinliang David Li 
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 =(const foo&);
  foo =(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&) = ::operator=;
bar &(bar::*x)(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  wrote:
> >
> >
> > On Mon, Feb 8, 2016 at 4:31 PM, Xinliang David Li 
> > wrote:
> >>
> >> On Mon, Feb 8, 2016 at 4:05 PM, David Blaikie 
> wrote:
> >> >
> >> >
> >> > On Mon, Feb 8, 2016 at 3:58 PM, Xinliang David Li  >
> >> > 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 =(const foo &);
> > };
> >
> > struct bar {
> >   foo f;
> > };
> >
> > auto (bar::*x)(const 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 

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

[David Li via cfe-commits]

davidxl updated this revision to Diff 47150.
davidxl added a comment.

Updated test case.

Another test case in profile-rt (pending) is : http://reviews.llvm.org/D16974


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,33 @@
+// 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 Base {
+  int B;
+  Base(int BB) : B(BB) {}
+  void operator=(const struct Base ) { B = b.B + 10; }
+};
+
+struct A : public Base {
+  A =(const A &) = default;
+  // PGOGEN: define {{.*}}@_ZN1AaSERKS_(
+  // PGOGEN: %pgocount = load {{.*}} @__profc__ZN1AaSERKS_
+  // PGOGEN: {{.*}}add{{.*}}%pgocount, 1
+  // PGOGEN: store{{.*}}@__profc__ZN1AaSERKS_
+
+  // Check that coverage mapping includes 6 function records including the
+  // defaulted A::operator=
+  // COVMAP: @__llvm_coverage_mapping = {{.*}} { { i32, i32, i32, i32 }, [6 x 
<{{.*}}>],
+  A(int II) : Base(II + 1), I(II) {}
+  A() : Base(0), I(0) {}
+  int I;
+};
+
+A ga(10);
+int main() {
+  A la;
+  la = ga;
+
+  if (la.I != 10 || la.B != 21)
+return 1;
+  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);


Index: test/Profile/def-assignop.cpp
===
--- test/Profile/def-assignop.cpp
+++ test/Profile/def-assignop.cpp
@@ -0,0 +1,33 @@
+// 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 Base {
+  int B;
+  Base(int BB) : B(BB) {}
+  void operator=(const struct Base ) { B = b.B + 10; }
+};
+
+struct A : public Base {
+  A =(const A &) = default;
+  // PGOGEN: define {{.*}}@_ZN1AaSERKS_(
+  // PGOGEN: %pgocount = load {{.*}} @__profc__ZN1AaSERKS_
+  // PGOGEN: {{.*}}add{{.*}}%pgocount, 1
+  // PGOGEN: store{{.*}}@__profc__ZN1AaSERKS_
+
+  // Check that coverage mapping includes 6 function records including the
+  // defaulted A::operator=
+  // COVMAP: @__llvm_coverage_mapping = {{.*}} { { i32, i32, i32, i32 }, [6 x <{{.*}}>],
+  A(int II) : Base(II + 1), I(II) {}
+  A() : Base(0), I(0) {}
+  int I;
+};
+
+A ga(10);
+int main() {
+  A la;
+  la = ga;
+
+  if (la.I != 10 || la.B != 21)
+return 1;
+  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);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits