Re: [PATCH] D21823: [Driver] Add flags for enabling both types of PGO Instrumentation

[Xinliang David Li via cfe-commits]

On Sun, Jul 3, 2016 at 1:50 PM, Sean Silva  wrote:

>
>
> On Sat, Jul 2, 2016 at 7:38 PM, Xinliang David Li 
> wrote:
>
>> Sanitizers are different IMO. Different santizers are rather independent,
>> and there is no such thing as -fsantize to mean -fsantize=all
>>
>> For PGO, in most of the cases, users do not need to care about the
>> sub-options implied -- by default they should just get the best profiling
>> (whatever that is). Fine-grained control is more useful for savvy users.
>> Besides, any flavor of value profiling usually does not make sense to be
>> used standalone and need to be used with edge profiling for max benefit
>> (also not supported in our implementation) - so I don't see why pgo control
>> needs to be done in a way similar to sanitizers.
>>
>
> Thanks for the explanation. So I think we can start by reducing this patch
> to just `-fpgo-train` (enables IRPGO), `-fpgo-train-file=`, and
> `-fpgo-apply=`.
>
> While -fpgo-apply= is technically redundant with -fprofile-instr-use, I
> think it is useful for consistency.
>
> I'm also indifferent to adding
> `-fpgo-train=source`/`-fpgo-train=optimizer` in this patch.
>


I like this suggestion (the reduced version).



>
> btw, I don't understand the intuition for calling IRPGO "cfg"; maybe you
> could explain?
>


> I like "optimizer" because it is easy to document to users, as users
> generally understand the difference between the "optimizer" and
> source-level analysis of their program. For example, we could document:
> "-fpgo-train=source instruments at source-level similar to code coverage
> instrumentation. -fpgo-train=optimizer applies the instrumentation inside
> the optimizer and has freedom to do sophisticated analyses and
> transformations as part of the instrumentation process; these analyses and
> transformations allow it to reduce instrumentation overhead and increase
> profile accuracy."
>
>
I am fine with using source/optimizer. I was mainly against overloading
-fpgo-train to do fine grain control.

David


>
> -- Sean Silva
>
>
>>
>>  In other words, I don't think the primary option -fpgo-train should be
>> used for fine-grain suboption control.   If we have a different option to
>> do fine-grain control for PGO, using sanitize like syntax is fine with me:
>>
>> -fpgo-xxx=y:z  turn on y and z for pgo
>> -fno-pgo-xxx=y:z  turn off y and z for pgo
>> or
>> -fpgo-xxx=no-w:no-y:z   turn on z but turn off w, and y
>>
>>
>> David
>>
>>
>> On Sat, Jul 2, 2016 at 6:45 PM, Sean Silva  wrote:
>>
>>>
>>>
>>> On Sat, Jul 2, 2016 at 1:57 PM, Xinliang David Li 
>>> wrote:
>>>


 On Fri, Jul 1, 2016 at 4:32 PM, Sean Silva 
 wrote:

> silvas added inline comments.
>
> 
> Comment at: lib/Driver/Tools.cpp:3560
> @@ +3559,3 @@
> +if (PGOTrainArg->getOption().matches(options::OPT_fpgo_train_EQ))
> {
> +  if (StringRef(PGOTrainArg->getValue()) == "source-cfg")
> +CmdArgs.push_back("-fprofile-instrument=clang");
> 
> davidxl wrote:
> > I think it is better to make the selector  'source' vs 'cfg'.
> >
> > -fpgo-train=source
> > -fpgo-train=cfg
> So would `-fpgo-train=cfg` enable icall instrumentation or not?
>
> My thinking is that down the road we will have one flag for each
> independent instrumentation capability (and of course some are mutually
> incompatible). This mirrors what the sanitizers do. For example, we would
> have:
> `-fpgo-train=optimizer-cfg` --> IR edge profiling
> `-fpgo-train=optimizer-icall` --> IR icall value profiling
> `-fpgo-train=optimizer-...` --> other independent instrumentation we
> can do in IR instrumentation.
> `-fpgo-train=source-cfg` --> FE edge profiling
> `-fpgo-train=source-icall` --> FE icall profiling (if that even
> exists; I see some code but there is no user-visible flag)
> `-fpgo-train=source-...` --> other FE instrumentation.
>
> We can then have `-fpgo-train=optimizer` enable e.g.
> `-fpgo-train=optimizer-cfg,optimizer-icall`.
> We can also have `-fpgo-train=source` enable e.g.
> `-fpgo-train=source-cfg`.
>
> Since instrumentation can have different overheads or different
> runtime requirements, users may want to disable some instrumentation.
>
>

 -fpgo-train is the new umbrella option that turn on at least edge
 profiling and some other flavor of value profiling (icall, or stringop, etc
 in the the future). There is a default setting for source or cfg based PGO.
 The fine grain control of each sub-option should be done via a different
 flag -- otherwise we will have to introduce 2 x N sub-options if used with
 -fpgo-train.  In other words, -fpgo-train=cfg turns on edge and icall
 profiling as of today.

 To summarize, I think the following behavior will be nice to users:

 1) -fpgo-train when used without any option, it defaults to IR

r270728 - Use new triple API to check comdat /NFC

[Xinliang David Li via cfe-commits]

Author: davidxl
Date: Wed May 25 12:25:57 2016
New Revision: 270728

URL: http://llvm.org/viewvc/llvm-project?rev=270728&view=rev
Log:
Use new triple API to check comdat /NFC

Modified:
cfe/trunk/lib/CodeGen/TargetInfo.cpp

Modified: cfe/trunk/lib/CodeGen/TargetInfo.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/TargetInfo.cpp?rev=270728&r1=270727&r2=270728&view=diff
==
--- cfe/trunk/lib/CodeGen/TargetInfo.cpp (original)
+++ cfe/trunk/lib/CodeGen/TargetInfo.cpp Wed May 25 12:25:57 2016
@@ -7818,7 +7818,7 @@ const llvm::Triple &CodeGenModule::getTr
 }
 
 bool CodeGenModule::supportsCOMDAT() const {
-  return !getTriple().isOSBinFormatMachO();
+  return getTriple().supportsCOMDAT();
 }
 
 const TargetCodeGenInfo &CodeGenModule::getTargetCodeGenInfo() {


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

r260161 - [PGO] Cover more cases of implicitly generated C++ methods

[Xinliang David Li via cfe-commits]

Author: davidxl
Date: Mon Feb  8 16:41:37 2016
New Revision: 260161

URL: http://llvm.org/viewvc/llvm-project?rev=260161&view=rev
Log:
[PGO] Cover more cases of implicitly generated C++ methods

Modified:
cfe/trunk/test/Profile/cxx-implicit.cpp

Modified: cfe/trunk/test/Profile/cxx-implicit.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Profile/cxx-implicit.cpp?rev=260161&r1=260160&r2=260161&view=diff
==
--- cfe/trunk/test/Profile/cxx-implicit.cpp (original)
+++ cfe/trunk/test/Profile/cxx-implicit.cpp Mon Feb  8 16:41:37 2016
@@ -1,17 +1,51 @@
 // Ensure that implicit methods aren't instrumented.
 
-// RUN: %clang_cc1 -x c++ %s -triple %itanium_abi_triple -main-file-name 
cxx-implicit.cpp -o - -emit-llvm -fprofile-instrument=clang | FileCheck %s
+// RUN: %clang_cc1 -x c++ -std=c++11 %s -triple %itanium_abi_triple 
-main-file-name cxx-implicit.cpp -o - -emit-llvm -fprofile-instrument=clang | 
FileCheck %s
 
-// An implicit constructor is generated for Base. We should not emit counters
-// for it.
+// Implicit constructors are generated for Base. We should not emit counters
+// for them.
+// CHECK-DAG: define {{.*}}_ZN4BaseC2Ev
+// CHECK-DAG: define {{.*}}_ZN4BaseC2ERKS_
+// CHECK-DAG: define {{.*}}_ZN4BaseC2EOS_
+// CHECK-DAG: __profc__ZN7DerivedC2Ev,
+// CHECK-DAG: __profc__ZN7DerivedC2ERKS_
+// CHECK-DAG: __profc__ZN7DerivedC2EOS_
 // CHECK-NOT: @__profc__ZN4BaseC2Ev =
+// CHECK-NOT: @__profc__ZN4BaseC2ERKS_
+// CHECK-NOT: @__profc__ZN4BaseC2EOS_
+//
+// Implicit assignment operators are generated for Base. We should not emit 
counters
+// for them.
+// CHECK-NOT: @__profc__ZN4BaseaSEOS_
+// CHECK-NOT: @__profc__ZN4BaseaSERKS_
 
-struct Base {
+struct BaseBase {
+ BaseBase();
+ BaseBase(const BaseBase &);
+ BaseBase &operator=(const BaseBase &);
+ BaseBase &operator=(BaseBase &&);
+};
+
+struct Base : public BaseBase {
   virtual void foo();
 };
 
 struct Derived : public Base {
   Derived();
+  Derived(const Derived &);
+  Derived(Derived &&);
+  Derived &operator=(const Derived &);
+  Derived &operator=(Derived &&);
 };
 
 Derived::Derived() {}
+Derived::Derived(const Derived &d) : Base(d) {}
+Derived::Derived(Derived &&d) : Base(static_cast(d)) {}
+Derived& Derived::operator=(const Derived &d) {
+  Base::operator=(d);
+  return *this;
+}
+Derived& Derived::operator=(Derived &&d) {
+  Base::operator=(static_cast(d));
+  return *this;
+}


___
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 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 &b) {}
>> >> +  void operator=(const B &&b) {}
>> >
>> >
>> > Probably best to make these canonical to avoid confusion:
>> >
>> > B &operator=(const B&);
>> > B &operator=(B&&);
>> >
>> > (& they don't need definitions - just declarations)
>>
>> Will change.
>>
>> >
>> > Also, neither of these are the move /constructor/, just the move
>> > operator.
>> > Not sure if Vedant just used the wrong terminology, or whether it's
>> > worth
>> > testing the move/copy ctors too, to check that they do the right thing
>> > as
>>
>> I added tests for copy ctors, and plan to add move ctor test soon.
>>
>> > well. (if all of these things use the same codepath, I don't see a great
>> > benefit in having separate tests for them (but you can add them here if
>> > you
>> > like) - I'm just suggesting a manual verification in case those need a
>> > separate fix
>>
>> the ctor and assignment op do not share the same path -- the ctor path
>> is working as expected without the fix -- or do you mean there is no
>> need to cover both copy and move variants?
>
>
> I wouldn't necessarily bother testing multiple instances of the same
> codepath (so the copy and move ctor for example) - but 2 instances is no big
> deal (if there were several more, I might be inclined to just test one as a
> representative sample). I don't mind either way, though. The number is small
> & the test cases are arguably distinct.

Sorry I disagree with your general statement here. I treat such test
cases as 'black box testing' that do not know about the internal
implementation (code path). It may or may not share the same code path
today -- same is true in the future.

>
>>
>> >
>> >>
>> >> +};
>> >> +
>> >> +struct A {
>> >> +  A &operator=(const A &) = default;
>> >
>> >
>> > Is the fix/codepath specifically about explicitly defaulted ops?
>>
>> yes -- explicitly defaulted. There are some test coverage already for
>> implicitly declared ctors (but not assignment op -- probably worth
>> adding some testing too).
>
>
> Hmm - are you sure there's no common codepath that would cover the
> explicitly defaulted or implicitly defaulted ops together in one go?

Sorry I am not sure what you mean here.

David

>
>>
>>
>> > Or just any
>> > compiler-generated ones? (you could drop these lines if it's about any
>> > compiler-generated ones, might be simpler/more obvious that it's not
>> > about
>> > the "= default" feature)
>>
>> Other compiler generated ones are handled differently.
>>
>> thanks,
>>
>> David
>>
>> >
>> >>
>> >> +  // PGOGEN: define {{.*}}@_ZN1AaSERKS_(
>> >> +  // PGOGEN: %pgocount = load {{.*}} @__profc__ZN1AaSERKS_
>> >> +  // PGOGEN: {{.*}}add{{.*}}%pgocount, 1
>> >> +  // PGOGEN: store{{.*}}@__profc__ZN1AaSERKS_
>> >> +  A &operator=(A &&) = default;
>> >>
>> >> +  // PGOGEN: define {{.*}}@_ZN1AaSEOS_
>> >> +  // PGOGEN: %pgocount = load {{.*}} @__profc__ZN1AaSEOS_
>> >> +  // PGOGEN: {{.*}}add{{.*}}%pgocount, 1
>> >> +  // PGOGEN: store{{.*}}@__profc__ZN1AaSEOS_
>> >> +
>> >> +  // Check that coverage mapping includes 6 function records including
>> >> the
>> >> +  // defaulted copy and move operators: A::operator=
>> >> +  // COVMAP: @__llvm_coverage_mapping = {{.*}} { { i32, i32, i32, i32
>> >> },
>> >> [5 x <{{.*}}>],
>> >> +  B b;
>> >> +};
>> >> +
>> >> +int main() {
>> >> +  A a1, a2;
>> >> +  a1 = a2;
>> >> +  a2 = static_cast(a1);
>> >
>> >
>> > An option, though not necessarily better, would be to just take the
>> > address
>> > of the special members:
>> >
>> > auto (B::*x)(const B&) = &bar::operator=;
>> > auto (B::*x)(B&&) = &bar::operator=;
>> >
>> > In short, what I'm picturing, in total:
>> >
>> > struct A {
>> >   A &operator=(const A&);
>> >   A &operator=(A&&);
>> > 

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 &b) {}
>> >> >> +  void operator=(const B &&b) {}
>> >> >
>> >> >
>> >> > Probably best to make these canonical to avoid confusion:
>> >> >
>> >> > B &operator=(const B&);
>> >> > B &operator=(B&&);
>> >> >
>> >> > (& they don't need definitions - just declarations)
>> >>
>> >> Will change.
>> >>
>> >> >
>> >> > Also, neither of these are the move /constructor/, just the move
>> >> > operator.
>> >> > Not sure if Vedant just used the wrong terminology, or whether it's
>> >> > worth
>> >> > testing the move/copy ctors too, to check that they do the right
>> >> > thing
>> >> > as
>> >>
>> >> I added tests for copy ctors, and plan to add move ctor test soon.
>> >>
>> >> > well. (if all of these things use the same codepath, I don't see a
>> >> > great
>> >> > benefit in having separate tests for them (but you can add them here
>> >> > if
>> >> > you
>> >> > like) - I'm just suggesting a manual verification in case those need
>> >> > a
>> >> > separate fix
>> >>
>> >> the ctor and assignment op do not share the same path -- the ctor path
>> >> is working as expected without the fix -- or do you mean there is no
>> >> need to cover both copy and move variants?
>> >
>> >
>> > I wouldn't necessarily bother testing multiple instances of the same
>> > codepath (so the copy and move ctor for example) - but 2 instances is no
>> > big
>> > deal (if there were several more, I might be inclined to just test one
>> > as a
>> > representative sample). I don't mind either way, though. The number is
>> > small
>> > & the test cases are arguably distinct.
>>
>> Sorry I disagree with your general statement here. I treat such test
>> cases as 'black box testing' that do not know about the internal
>> implementation (code path). It may or may not share the same code path
>> today -- same is true in the future.
>
>
> While there's merit in both approaches, practically speaking it seems
> difficult to test in that way in general - any feature could interact with
> any other.

The language features are well specified -- so writing small test
cases to cover them is a general accepted way of testing.

>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 &operator=(const A &) = default;
>> >> >
>> >> >
>> >> > Is the fix/codepath specifically about explicitly defaulted ops?
>> >>
>> >> yes -- explicitly defaulted. There are some test coverage already for
>> >> implicitly declared ctors (but not assignment op -- probably worth
>> >> adding some testing too).
>> >
>> >
>> > Hmm - are you sure there's no common codepath that would cover the
>> > explicitly defaulted or implicitly defaulted ops together in one go?
>>
>> Sorry I am not sure what you mean here.
> Is there some part of Clang that is responsible for generating both
> implicitly defaulted and explicitly defaulted move/copy ops that could
> handle this case, rather than apparently handling the implicit and explicit
> cases separately (it seems they're being handled separately if the implicit
> case worked before and you added code (rather than moving code) to fix the
> explicit case - it s

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 &b) {}
>> >> >> >> +  void operator=(const B &&b) {}
>> >> >> >
>> >> >> >
>> >> >> > Probably best to make these canonical to avoid confusion:
>> >> >> >
>> >> >> > B &operator=(const B&);
>> >> >> > B &operator=(B&&);
>> >> >> >
>> >> >> > (& they don't need definitions - just declarations)
>> >> >>
>> >> >> Will change.
>> >> >>
>> >> >> >
>> >> >> > Also, neither of these are the move /constructor/, just the move
>> >> >> > operator.
>> >> >> > Not sure if Vedant just used the wrong terminology, or whether
>> >> >> > it's
>> >> >> > worth
>> >> >> > testing the move/copy ctors too, to check that they do the right
>> >> >> > thing
>> >> >> > as
>> >> >>
>> >> >> I added tests for copy ctors, and plan to add move ctor test soon.
>> >> >>
>> >> >> > well. (if all of these things use the same codepath, I don't see a
>> >> >> > great
>> >> >> > benefit in having separate tests for them (but you can add them
>> >> >> > here
>> >> >> > if
>> >> >> > you
>> >> >> > like) - I'm just suggesting a manual verification in case those
>> >> >> > need
>> >> >> > a
>> >> >> > separate fix
>> >> >>
>> >> >> the ctor and assignment op do not share the same path -- the ctor
>> >> >> path
>> >> >> is working as expected without the fix -- or do you mean there is no
>> >> >> need to cover both copy and move variants?
>> >> >
>> >> >
>> >> > I wouldn't necessarily bother testing multiple instances of the same
>> >> > codepath (so the copy and move ctor for example) - but 2 instances is
>> >> > no
>> >> > big
>> >> > deal (if there were several more, I might be inclined to just test
>> >> > one
>> >> > as a
>> >> > representative sample). I don't mind either way, though. The number
>> >> > is
>> >> > small
>> >> > & the test cases are arguably distinct.
>> >>
>> >> Sorry I disagree with your general statement here. I treat such test
>> >> cases as 'black box testing' that do not know about the internal
>> >> implementation (code path). It may or may not share the same code path
>> >> today -- same is true in the future.
>> >
>> >
>> > While there's merit in both approaches, practically speaking it seems
>> > difficult to test in that way in general - any feature could interact
>> > with
>> > any other.
>>
>> The language features are well specified -- so writing small test
>> cases to cover them is a general accepted way of testing.
>
>
> I'm not sure I follow the distinction you're drawing between the middle end
> optimization tests and the features you're testing here. If the features are
> relatively independent, even within the same API/feature area, they're
> generally tested independently (even two features within a single middle end
> optimization - a test case is written to ensure that, say, ArgumentPromotion
> correctly handles debug info, and another that it correctly handles inalloca
> (or fp80, etc - just looking at test/Transforms/

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 &b) {}
>> >> >> >> >> +  void operator=(const B &&b) {}
>> >> >> >> >
>> >> >> >> >
>> >> >> >> > Probably best to make these canonical to avoid confusion:
>> >> >> >> >
>> >> >> >> > B &operator=(const B&);
>> >> >> >> > B &operator=(B&&);
>> >> >> >> >
>> >> >> >> > (& they don't need definitions - just declarations)
>> >> >> >>
>> >> >> >> Will change.
>> >> >> >>
>> >> >> >> >
>> >> >> >> > Also, neither of these are the move /constructor/, just the
>> >> >> >> > move
>> >> >> >> > operator.
>> >> >> >> > Not sure if Vedant just used the wrong terminology, or whether
>> >> >> >> > it's
>> >> >> >> > worth
>> >> >> >> > testing the move/copy ctors too, to check that they do the
>> >> >> >> > right
>> >> >> >> > thing
>> >> >> >> > as
>> >> >> >>
>> >> >> >> I added tests for copy ctors, and plan to add move ctor test
>> >> >> >> soon.
>> >> >> >>
>> >> >> >> > well. (if all of these things use the same codepath, I don't
>> >> >> >> > see a
>> >> >> >> > great
>> >> >> >> > benefit in having separate tests for them (but you can add them
>> >> >> >> > here
>> >> >> >> > if
>> >> >> >> > you
>> >> >> >> > like) -

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 &operator=(const foo &);
> };
>
> struct bar {
>   foo f;
> };
>
> auto (bar::*x)(const bar&) = &bar::operator=;
>
> Fires the assertion - this seems to me to indicate that the codepath you
> changed is used for both the explicitly (based on the change fixing your
> test case) and implicitly defaulted (based on my test case) cases.
>
> Is it possible that you end up with duplicate counters by accident in this
> path, then? Or at least that whatever codepath was handling the implicitly
> defaulted ones is now redundant with this one?
>
> Actually, so far as I can tell this doesn't work for implicitly defaulted
> move ops (the above test case doesn't have an add pgocount in it) - perhaps
> I'm missing something/doing it wrong? or was just not communicating clearly
> regarding explicit versus implicitly defaulted special members.
>
> - Dave
>
>>
>>
>> thanks,
>>
>> David
>>
>>
>>
>> >
>> > - Dave
>> >
>> >>
>> >>
>> >> thanks,
>> >>
>> >> David
>> >>
>> >> On Mon, Feb 8, 2016 at 3:52 PM, David Blaikie 
>> >> 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.
>> >> >> >> >> >>
>> >> >> >> >>

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 &operator=(const foo&);
>   foo &operator=(foo&&);
> };
>
> struct bar {
>   foo f; // or derive bar from foo, but I think the member version is
> simpler
> };
>
> // force emission of bar's implicit special members, one way or another:
> bar &(bar::*x)(const bar&) = &bar::operator=;
> bar &(bar::*x)(bar&&) = &bar::operator=;
>
> (or just call them as you had in your test case - but that produces more
> code, etc in the module, extra functions/profile counters/etc)
>
> - Dave
>
>>
>>
>> thanks,
>>
>> David
>>
>>
>> On Mon, Feb 8, 2016 at 4:58 PM, David Blaikie  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 p

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 &operator=(const foo&);
>> >   foo &operator=(foo&&);
>> > };
>> >
>> > struct bar {
>> >   foo f; // or derive bar from foo, but I think the member version is
>> > simpler
>> > };
>> >
>> > // force emission of bar's implicit special members, one way or another:
>> > bar &(bar::*x)(const bar&) = &bar::operator=;
>> > bar &(bar::*x)(bar&&) = &bar::operator=;
>> >
>> > (or just call them as you had in your test case - but that produces more
>> > code, etc in the module, extra functions/profile counters/etc)
>> >
>> > - Dave
>> >
>> >>
>> >>
>> >> thanks,
>> >>
>> >> David
>> >>
>> >>
>> >> On Mon, Feb 8, 2016 at 4:58 PM, David Blaikie 
>> >> 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 te

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 &operator=(const foo&);
>> >> >   foo &operator=(foo&&);
>> >> > };
>> >> >
>> >> > struct bar {
>> >> >   foo f; // or derive bar from foo, but I think the member version is
>> >> > simpler
>> >> > };
>> >> >
>> >> > // force emission of bar's implicit special members, one way or
>> >> > another:
>> >> > bar &(bar::*x)(const bar&) = &bar::operator=;
>> >> > bar &(bar::*x)(bar&&) = &bar::operator=;
>> >> >
>> >> > (or just call them as you had in your test case - but that produces
>> >> > more
>> >> > code, etc in the module, extra functions/profile counters/etc)
>> >> >
>> >> > - Dave
>> >> >
>> >> >>
>> >> >>
>> >> >> thanks,
>> >> >>
>> >> >> David
>> >> >>
>> >> >>
>> >> >> On Mon, Feb 8, 2016 at 4:58 PM, David Blaikie 
>> >> >> wrote:
>> >> >> >
>> >> >> >
>> >> >> > On Mon, Feb 8, 2016

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
>> >> >> > end up
>> >> >> > testing all sorts of ways of writing/interacting with dtors which
>> >> >> > wouldn't
>> >> >> > be relevant to the code/fix/etc.
>> >

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.
>> >> >> >> >
>> >> >> >> >
>> >> >> >> > Ah, glad we figured out the disconnect - thanks for bearing
>> >> >> >> > with
>> >> >> >> > me!
>> >> >> >> >
>> >> >> >> >>
>> >> >> >> >>
>> >> >> >> >> From your 

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
>> >> >> >> > be
>> >> >> >> > any different from a profile (& profile-guided-optimization)
>> >> >> >> > perspective
>> >> >> >> > from an explicitly defaulted one?
>> >> >> >>
>> >> >> >> There are two facto

r260270 - [PGO] Fix issue: explicitly defaulted assignop is not profiled

[Xinliang David Li via cfe-commits]

Author: davidxl
Date: Tue Feb  9 14:02:59 2016
New Revision: 260270

URL: http://llvm.org/viewvc/llvm-project?rev=260270&view=rev
Log:
[PGO] Fix issue: explicitly defaulted assignop is not profiled

Differential Revision: http://reviews.llvm.org/D16947
 




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

Modified: cfe/trunk/lib/CodeGen/CGClass.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGClass.cpp?rev=260270&r1=260269&r2=260270&view=diff
==
--- cfe/trunk/lib/CodeGen/CGClass.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGClass.cpp Tue Feb  9 14:02:59 2016
@@ -1608,6 +1608,7 @@ void CodeGenFunction::emitImplicitAssign
 
   LexicalScope Scope(*this, RootCS->getSourceRange());
 
+  incrementProfileCounter(RootCS);
   AssignmentMemcpyizer AM(*this, AssignOp, Args);
   for (auto *I : RootCS->body())
 AM.emitAssignment(I);

Added: cfe/trunk/test/Profile/def-assignop.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Profile/def-assignop.cpp?rev=260270&view=auto
==
--- cfe/trunk/test/Profile/def-assignop.cpp (added)
+++ cfe/trunk/test/Profile/def-assignop.cpp Tue Feb  9 14:02:59 2016
@@ -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);
+  B& operator=(const B &&b);
+};
+
+struct A {
+  A &operator=(const A &) = default;
+  // PGOGEN: define {{.*}}@_ZN1AaSERKS_(
+  // PGOGEN: %pgocount = load {{.*}} @__profc__ZN1AaSERKS_
+  // PGOGEN: {{.*}}add{{.*}}%pgocount, 1
+  // PGOGEN: store{{.*}}@__profc__ZN1AaSERKS_
+  A &operator=(A &&) = default;
+  // PGOGEN: define {{.*}}@_ZN1AaSEOS_
+  // PGOGEN: %pgocount = load {{.*}} @__profc__ZN1AaSEOS_
+  // PGOGEN: {{.*}}add{{.*}}%pgocount, 1
+  // PGOGEN: store{{.*}}@__profc__ZN1AaSEOS_
+
+  // Check that coverage mapping includes 6 function records including the
+  // defaulted copy and move operators: A::operator=
+  // COVMAP: @__llvm_coverage_mapping = {{.*}} { { i32, i32, i32, i32 }, [3 x 
<{{.*}}>],
+  B b;
+};
+
+int main() {
+  A a1, a2;
+  a1 = a2;
+  a2 = static_cast(a1);
+  return 0;
+}


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

r261046 - Restrengthen tests relaxed in r259955

[Xinliang David Li via cfe-commits]

Author: davidxl
Date: Tue Feb 16 18:58:13 2016
New Revision: 261046

URL: http://llvm.org/viewvc/llvm-project?rev=261046&view=rev
Log:
Restrengthen tests relaxed in r259955

Modified:
cfe/trunk/test/CoverageMapping/ir.c
cfe/trunk/test/CoverageMapping/unused_names.c

Modified: cfe/trunk/test/CoverageMapping/ir.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CoverageMapping/ir.c?rev=261046&r1=261045&r2=261046&view=diff
==
--- cfe/trunk/test/CoverageMapping/ir.c (original)
+++ cfe/trunk/test/CoverageMapping/ir.c Tue Feb 16 18:58:13 2016
@@ -9,4 +9,4 @@ int main(void) {
   return 0;
 }
 
-// CHECK: @__llvm_coverage_mapping = internal constant { { i32, i32, i32, i32 
}, [2 x <{{.*}}>], [{{[0-9]+}} x i8] } { { i32, i32, i32, i32 } { i32 2, i32 
{{[0-9]+}}, i32 {{[0-9]+}}, i32 {{[0-9]+}} }, [2 x <{{.*}}>] [<{{.*}}> 
<{{.*}}>, <{{.*}}> <{{.*}}>]
+// CHECK: @__llvm_coverage_mapping = internal constant { { i32, i32, i32, i32 
}, [2 x <{ i64, i32, i64 }>], [{{[0-9]+}} x i8] } { { i32, i32, i32, i32 } { 
i32 2, i32 {{[0-9]+}}, i32 {{[0-9]+}}, i32 {{[0-9]+}} }, [2 x <{ i64, i32, i64 
}>] [<{{.*}}> <{{.*}}>, <{{.*}}> <{{.*}}>]

Modified: cfe/trunk/test/CoverageMapping/unused_names.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CoverageMapping/unused_names.c?rev=261046&r1=261045&r2=261046&view=diff
==
--- cfe/trunk/test/CoverageMapping/unused_names.c (original)
+++ cfe/trunk/test/CoverageMapping/unused_names.c Tue Feb 16 18:58:13 2016
@@ -7,6 +7,7 @@
 // CHECK-DAG: @__profn_bar = {{.*}} [3 x i8] c"bar"
 // CHECK-DAG: @__profn_baz = {{.*}} [3 x i8] c"baz"
 // CHECK-DAG: @__profn_unused_names.c_qux = {{.*}} [18 x i8] 
c"unused_names.c:qux"
+// CHECK-DAG: @__llvm_prf_nm = private constant {{.*}}, section 
"{{.*}}__llvm_prf_names"
 
 // SYSHEADER-NOT: @__profn_foo =
 


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

r261047 - Test simplification

[Xinliang David Li via cfe-commits]

Author: davidxl
Date: Tue Feb 16 18:59:01 2016
New Revision: 261047

URL: http://llvm.org/viewvc/llvm-project?rev=261047&view=rev
Log:
Test simplification

Modified:
cfe/trunk/test/Profile/def-assignop.cpp

Modified: cfe/trunk/test/Profile/def-assignop.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Profile/def-assignop.cpp?rev=261047&r1=261046&r2=261047&view=diff
==
--- cfe/trunk/test/Profile/def-assignop.cpp (original)
+++ cfe/trunk/test/Profile/def-assignop.cpp Tue Feb 16 18:59:01 2016
@@ -24,9 +24,8 @@ struct A {
   B b;
 };
 
-int main() {
-  A a1, a2;
+A a1, a2;
+void foo() {
   a1 = a2;
   a2 = static_cast(a1);
-  return 0;
 }


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

r258106 - Fix local variable name /NFC

[Xinliang David Li via cfe-commits]

Author: davidxl
Date: Mon Jan 18 18:49:06 2016
New Revision: 258106

URL: http://llvm.org/viewvc/llvm-project?rev=258106&view=rev
Log:
Fix local variable name /NFC

Modified:
cfe/trunk/lib/CodeGen/CoverageMappingGen.cpp
cfe/trunk/lib/CodeGen/CoverageMappingGen.h

Modified: cfe/trunk/lib/CodeGen/CoverageMappingGen.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CoverageMappingGen.cpp?rev=258106&r1=258105&r2=258106&view=diff
==
--- cfe/trunk/lib/CodeGen/CoverageMappingGen.cpp (original)
+++ cfe/trunk/lib/CodeGen/CoverageMappingGen.cpp Mon Jan 18 18:49:06 2016
@@ -911,7 +911,7 @@ static void dump(llvm::raw_ostream &OS,
 
 void CoverageMappingModuleGen::addFunctionMappingRecord(
 llvm::GlobalVariable *NamePtr, StringRef NameValue, uint64_t FuncHash,
-const std::string &CoverageMapping, bool isUsed) {
+const std::string &CoverageMapping, bool IsUsed) {
   llvm::LLVMContext &Ctx = CGM.getLLVMContext();
   if (!FunctionRecordTy) {
 #define COVMAP_FUNC_RECORD(Type, LLVMType, Name, Init) LLVMType,
@@ -929,7 +929,7 @@ void CoverageMappingModuleGen::addFuncti
   };
   FunctionRecords.push_back(llvm::ConstantStruct::get(
   FunctionRecordTy, makeArrayRef(FunctionRecordVals)));
-  if (!isUsed)
+  if (!IsUsed)
 FunctionNames.push_back(
 llvm::ConstantExpr::getBitCast(NamePtr, 
llvm::Type::getInt8PtrTy(Ctx)));
   CoverageMappings += CoverageMapping;

Modified: cfe/trunk/lib/CodeGen/CoverageMappingGen.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CoverageMappingGen.h?rev=258106&r1=258105&r2=258106&view=diff
==
--- cfe/trunk/lib/CodeGen/CoverageMappingGen.h (original)
+++ cfe/trunk/lib/CodeGen/CoverageMappingGen.h Mon Jan 18 18:49:06 2016
@@ -72,7 +72,7 @@ public:
 StringRef FunctionNameValue,
 uint64_t FunctionHash,
 const std::string &CoverageMapping,
-bool isUsed = true);
+bool IsUsed = true);
 
   /// \brief Emit the coverage mapping data for a translation unit.
   void emit();


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

r258261 - Reference the updated function name /NFC

[Xinliang David Li via cfe-commits]

Author: davidxl
Date: Tue Jan 19 18:24:52 2016
New Revision: 258261

URL: http://llvm.org/viewvc/llvm-project?rev=258261&view=rev
Log:
Reference the updated function name /NFC

Modified:
cfe/trunk/lib/CodeGen/CoverageMappingGen.cpp

Modified: cfe/trunk/lib/CodeGen/CoverageMappingGen.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CoverageMappingGen.cpp?rev=258261&r1=258260&r2=258261&view=diff
==
--- cfe/trunk/lib/CodeGen/CoverageMappingGen.cpp (original)
+++ cfe/trunk/lib/CodeGen/CoverageMappingGen.cpp Tue Jan 19 18:24:52 2016
@@ -1035,7 +1035,7 @@ void CoverageMappingModuleGen::emit() {
 // to pass the list of names referenced to codegen.
 new llvm::GlobalVariable(CGM.getModule(), NamesArrTy, true,
  llvm::GlobalValue::InternalLinkage, NamesArrVal,
- llvm::getCoverageNamesVarName());
+ llvm::getCoverageUnusedNamesVarName());
   }
 }
 


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

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Xinliang David Li via cfe-commits]

For the longer term, one possible solution is to make FE based
instrumentation only used for coverage testing which can be turned on
with -fcoverage-mapping option (currently, -fcoverage-mapping can not
be used alone and must be used together with
-fprofile-instr-generate). To summarize:

A. Current behavior:
---
1) -fprofile-instr-generate turns on FE based instrumentation
2) -fprofile-instr-generate -fcoverage-mapping turns on FE based
instrumentation and coverage mapping data generation.
3) -fprofile-instr-use=<..> assumes profile data from FE instrumentation.


B. Proposed new behavior:

1) -fprofile-instr-generate turns on IR late instrumentation
2) -fcoverage-mapping turns on FE instrumentation and coverage-mapping
3) -fprofile-instr-generate -fcoverage-mapping result in compiler warning
4) -fprofile-instr-use=<> will automatically determine how to use the
profile data.

B.2) above can be done today for improved usability. B.1) needs a
transition period before  the IR based instrumentation becomes
stablized (and can be flipped to the default).  During the transition
period, the behavior of 1) does not change, but a cc1 option can be
used to turn on IR instrumentation (as proposed by Sean).


In the real longer term, I think IR based instrumentation can also be
used for coverage testing too (by disabling some pre-optimizations and
pre-inlining needed for PGO purpose) -- but this is a different topic
to be discussed.

thanks,

David









On Thu, Jan 21, 2016 at 7:40 PM, Sean Silva  wrote:
> silvas added a comment.
>
> @bogner btw, did you say that at Apple you guys have a requirement of 
> supporting existing profdata? I.e. users can pass older profdata to a newer 
> compiler?
>
> Realistically, it would be nice if our PGO offering defaulted to the IR stuff 
> (since it seems like it is going to be the focus of optimization work and so 
> is likely to be our best-performing offering). But if we need to support 
> profdata across versions of clang, that puts us in a thorny situation because 
> suddenly `clang -c foo.c 
> -fprofile-instr-use=old-profdata-from-frontend-instrumentation.profdata` has 
> a default meaning equivalent to `clang -c foo.c -fir-level-instrumentation 
> -fprofile-instr-use=old-profdata-from-frontend-instrumentation.profdata` 
> (setting aside the flag name issue), which is a case we want to error on due 
> to mismatch between frontend and IR-level instrumentation.
>
>
> http://reviews.llvm.org/D15829
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Xinliang David Li via cfe-commits]

On Fri, Jan 22, 2016 at 1:43 PM, Sean Silva  wrote:
> silvas added a comment.
>
> In http://reviews.llvm.org/D15829#333902, @davidxl wrote:
>
>> For the longer term, one possible solution is to make FE based
>>  instrumentation only used for coverage testing which can be turned on
>>  with -fcoverage-mapping option (currently, -fcoverage-mapping can not
>>  be used alone and must be used together with
>>  -fprofile-instr-generate). To summarize:
>>
>> A. Current behavior:
>>
>>  ---
>>
>> 1. -fprofile-instr-generate turns on FE based instrumentation
>> 2. -fprofile-instr-generate -fcoverage-mapping turns on FE based 
>> instrumentation and coverage mapping data generation.
>> 3. -fprofile-instr-use=<..> assumes profile data from FE instrumentation.
>>
>> B. Proposed new behavior:
>>
>>  
>>
>> 1. -fprofile-instr-generate turns on IR late instrumentation
>> 2. -fcoverage-mapping turns on FE instrumentation and coverage-mapping
>> 3. -fprofile-instr-generate -fcoverage-mapping result in compiler warning
>> 4. -fprofile-instr-use=<> will automatically determine how to use the 
>> profile data.
>
>
> Very good observation that we can determine FE or IR automatically based on 
> the input profdata. That simplifies things.
>
>> B.2) above can be done today for improved usability.
>
>
> I don't see how this improves usability. In fact, it is confusing because it 
> hijacks an existing option.
>
> Also B.3 causes existing user builds to emit a warning, which is annoying.

Since it is pointless to specify -fcoverage-mapping option alone, why
not do not automatically? Yes it means some behavior change (in a good
way) and  some annoying warnings initially, but those are trivially
fixable by the user.


>
> I would propose the following modification of B:
>
> C.:
>
> 1. -fprofile-instr-generate defaults to IR instrumentation (i.e. behaves 
> exactly as before, except that it uses IR instrumentation)
> 2. -fprofile-instr-generate -fcoverage-mapping turns on frontend 
> instrumentation and coverage. (i.e. behaves exactly as before)

I am fine with this -- as long as the user does not need to bear the
burden of understanding where and how the instrumentation is done.

> 3. -fprofile-instr-use=<> automatically determines which method to use
>
> All existing user workflows continue to work, except for workflows that 
> attempt to `llvm-profdata merge` some old frontend profile data (e.g. they 
> have checked-in to version control and represents some special workload) with 
> the profile data from new binaries.
>
> Concretely, imagine the following workflow:
>
>   clang -fprofile-instr-generate foo.c -o foo
>   ./foo
>   llvm-profdata merge default.profraw -o new.profdata
>   llvm-profdata merge new.profdata 
> /versioncontrol/some-important-but-expensive-to-reproduce-workload.profdata 
> -o foo.profdata
>   clang -fprofile-instr-use=foo.profdata foo.c -o foo_pgo
>
> I think this is a reasonable breakage. We would need to add a note in the 
> release notes. Unfortunately this is not expected breakage if we claim to 
> have forward compatibility for profdata (which IIRC Apple requires; 
> @bogner?). But I think this case will be rare and exceptional enough that we 
> can tolerate it:
>

The old profile data has to out live the program source versions which
usually change fast. In reality, I don't expect profile data to live
too long.

> - a simple immediate workaround is to specify `-fcoverage-mapping` (which 
> also adds some extra stuff, but works around the issue)

This is a reasonable workaround

> - Presumably 
> /versioncontrol/some-important-but-expensive-to-reproduce-workload.profdata 
> is regenerated with some frequency which is more frequent than upgrading the 
> compiler, and so it is likely reasonable to regenerate it alongside a 
> compiler upgrade, using the workaround until then.
>
>
>
>> B.1) needs a
>
>>  transition period before  the IR based instrumentation becomes
>
>>  stablized (and can be flipped to the default).  During the transition
>
>>  period, the behavior of 1) does not change, but a cc1 option can be
>
>>  used to turn on IR instrumentation (as proposed by Sean).
>
>
> Just to clarify, users are not allowed to use cc1 options. The cc1 option is 
> purely for us as compiler developers to do integration and testing, put off 
> some discussions for later, etc.
>

yes.

thanks,

David

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

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Xinliang David Li via cfe-commits]

We first need to nail the use model of the option, and then talk about
implementation. To clarify, I think the following should be the way:

(assuming the current clang instrumetnation is the default):

1) To turn on clang based instrumentation:
  -fprofile-instr-generate[=]
2) To turn on clang based instrumentation explicitly:
  -fprofile-instr-generate[=] -Xclang -fprofile-instrumentor=clang

3) To turn on IR based instrumentation:
  -fprofile-instr-generate[=] -Xclang -fprofile-instrumentor=llvm


There is no need to change driver handling -- just let it forward. In
CompilerInvocation.cpp, diagnostics can be emitted if user does

-Xclang -fprofile-instrumentor=xxx without specfiying -fprofile-instr-generate


Handling of -fprofile-instr-use=<> is different which does not depend
on -fprofile-instrumentor, so it should be done in a different patch.

David




On Wed, Jan 27, 2016 at 11:38 AM, Rong Xu  wrote:
> On Wed, Jan 27, 2016 at 11:31 AM, David Li  wrote:
>> davidxl added inline comments.
>>
>> 
>> Comment at: lib/Driver/Tools.cpp:5520
>> @@ +5519,3 @@
>> +A->claim();
>> +if ((StringRef(A->getValue(0)) == "-fprofile-ir-instr") &&
>> +(Args.hasArg(options::OPT_fprofile_instr_generate) ||
>> 
>> silvas wrote:
>>> This is definitely not the right thing to do. Don't hijack -Xclang (which 
>>> is a completely generic thing).
>> Why do we need special handling here ? will the existing behavior (simple 
>> forwarding) work?
>
> The original patch does the simple forwarding. In that case, we handle
> the cc1 options (choosing b/w IR or clang instrumentation) in
> CompilerInvocation.cpp. Sean and Chad think this logic should be
> driver.
>
>>
>> Note that intention of the cc1 option is to hide it from the user but make 
>> it used by the developers -- so we can make assumption that this option is 
>> used only when -fprofile-instr-generate[=...] is specified. You can add 
>> diagnostics later during cc1 option processing if it is not the case.
>>
>> Also think about making it easier to flip the default behavior in the 
>> future, it might be better to make the cc1 option look like this:
>>
>> -fprofile-instrumentor=[clang|LLVM]
>>
>> if the option is missing, the current default will be 'clang'. In the future 
>> just switch it to 'llvm'.
>>
>> 
>> Comment at: lib/Frontend/CompilerInvocation.cpp:483
>> @@ +482,3 @@
>> +Opts.ProfileIRInstr = true;
>> +  else
>> +// Opts.ClangProfInstrGen will be used for Clang instrumentation only.
>> 
>> silvas wrote:
>>> This still isn't factored right. I think at this point the meaning of the 
>>> driver-level options is not really useful at CC1 level (too convoluted) for 
>>> it to be useful to pass them through.
>>>
>>> The right thing to do here is probably more like:
>>> - refactor so that we have 4 individual CC1 options for InstrProfileOutput, 
>>> InstrProfileInput, ProfileIRInstr, ClangProfInstrGen (although probably 
>>> rename ClangProfInstrGen and ProfileIRInstr to be consistent with each 
>>> other, e.g. "ProfileIRInstr" and "ProfileClangInstr").
>>> - add logic in Driver to convert from the driver-level options to the CC1 
>>> options.
>> It is already pretty close to your proposal -- the only missing thing is cc1 
>> option for ClangProfInstrGen. However given that IR and Clang InstrGen are 
>> mutually exclusive, is an additional CC1 option needed?
>>
>>
>> http://reviews.llvm.org/D15829
>>
>>
>>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r259067 - [PGO] test case cleanups

[Xinliang David Li via cfe-commits]

Author: davidxl
Date: Thu Jan 28 12:25:53 2016
New Revision: 259067

URL: http://llvm.org/viewvc/llvm-project?rev=259067&view=rev
Log:
[PGO] test case cleanups

1. Make test case more focused and robust by focusing on what to be tested 
(linkage, icall) -- make it easier to validate
2. Testing linkages of data and counter variables instead of names. Counters 
and data are more relavant to be tested.


Modified:
cfe/trunk/test/Profile/c-indirect-call.c
cfe/trunk/test/Profile/c-linkage-available_externally.c
cfe/trunk/test/Profile/c-linkage.c
cfe/trunk/test/Profile/cxx-linkage.cpp

Modified: cfe/trunk/test/Profile/c-indirect-call.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Profile/c-indirect-call.c?rev=259067&r1=259066&r2=259067&view=diff
==
--- cfe/trunk/test/Profile/c-indirect-call.c (original)
+++ cfe/trunk/test/Profile/c-indirect-call.c Thu Jan 28 12:25:53 2016
@@ -7,7 +7,7 @@ int main(void) {
 // CHECK:  [[REG1:%[0-9]+]] = load void ()*, void ()** @foo, align 8
 // CHECK-NEXT:  call void [[REG1]]()
 // CHECK-NEXT:  [[REG2:%[0-9]+]] = ptrtoint void ()* [[REG1]] to i64
-// CHECK-NEXT:  call void @__llvm_profile_instrument_target(i64 [[REG2]], i8* 
bitcast ({ i32, i32, i64, i8*, i64*, i8*, i8*, [1 x i16] }* @__profd_main to 
i8*), i32 0)
+// CHECK-NEXT:  call void @__llvm_profile_instrument_target(i64 [[REG2]], i8* 
bitcast ({{.*}}* @__profd_main to i8*), i32 0)
   foo();
   return 0;
 }

Modified: cfe/trunk/test/Profile/c-linkage-available_externally.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Profile/c-linkage-available_externally.c?rev=259067&r1=259066&r2=259067&view=diff
==
--- cfe/trunk/test/Profile/c-linkage-available_externally.c (original)
+++ cfe/trunk/test/Profile/c-linkage-available_externally.c Thu Jan 28 12:25:53 
2016
@@ -1,11 +1,9 @@
-// Make sure instrementation data from available_externally functions doesn't
-// get thrown out.
+// Make sure instrumentation data from available_externally functions doesn't
+// get thrown out and are emitted with the expected linkage.
 // RUN: %clang_cc1 -O2 -triple x86_64-apple-macosx10.9 -main-file-name 
c-linkage-available_externally.c %s -o - -emit-llvm -fprofile-instr-generate | 
FileCheck %s
 
-// CHECK: @__profn_foo = linkonce_odr hidden constant [3 x i8] c"foo", section 
"__DATA,__llvm_prf_names", align 1
-
 // CHECK: @__profc_foo = linkonce_odr hidden global [1 x i64] zeroinitializer, 
section "__DATA,__llvm_prf_cnts", align 8
-// CHECK: @__profd_foo = linkonce_odr hidden global { i32, i32, i64, i8*, 
i64*, i8*, i8*, [1 x i16] } { i32 3, i32 1, i64 0, i8* getelementptr inbounds 
([3 x i8], [3 x i8]* @__profn_foo, i32 0, i32 0), i64* getelementptr inbounds 
([1 x i64], [1 x i64]* @__profc_foo, i32 0, i32 0), i8* null, i8* null, [1 x 
i16] zeroinitializer }, section "__DATA,__llvm_prf_data", align 8
+// CHECK: @__profd_foo = linkonce_odr hidden global {{.*}} i64* getelementptr 
inbounds ([1 x i64], [1 x i64]* @__profc_foo, i32 0, i32 0){{.*}}, section 
"__DATA,__llvm_prf_data", align 8
 inline int foo(void) { return 1; }
 
 int main(void) {

Modified: cfe/trunk/test/Profile/c-linkage.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Profile/c-linkage.c?rev=259067&r1=259066&r2=259067&view=diff
==
--- cfe/trunk/test/Profile/c-linkage.c (original)
+++ cfe/trunk/test/Profile/c-linkage.c Thu Jan 28 12:25:53 2016
@@ -1,10 +1,14 @@
-// Check that the profiling names we create have the linkage we expect
+// Check that the profiling counters and data we create have the linkage we 
expect
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.9 -main-file-name c-linkage.c 
%s -o - -emit-llvm -fprofile-instr-generate | FileCheck %s
 
-// CHECK: @__profn_foo = private constant [3 x i8] c"foo"
-// CHECK: @__profn_foo_weak = weak hidden constant [8 x i8] c"foo_weak"
-// CHECK: @__profn_main = private constant [4 x i8] c"main"
-// CHECK: @__profn_c_linkage.c_foo_internal = private constant [24 x i8] 
c"c-linkage.c:foo_internal"
+// CHECK: @__profc_foo = private global
+// CHECK: @__profd_foo = private global
+// CHECK: @__profc_foo_weak = weak hidden global
+// CHECK: @__profd_foo_weak = weak hidden global
+// CHECK: @__profc_main = private global
+// CHECK: @__profd_main = private global
+// CHECK: @__profc_c_linkage.c_foo_internal = private global
+// CHECK: @__profd_c_linkage.c_foo_internal = private global
 
 void foo(void) { }
 

Modified: cfe/trunk/test/Profile/cxx-linkage.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Profile/cxx-linkage.cpp?rev=259067&r1=259066&r2=259067&view=diff
==
--- cfe/trunk/test/Profile/cxx-linkage.cpp (original)
+++ cfe/trunk/test/Profile/cxx-linkage.cpp Thu Jan 28 12:25:

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Xinliang David Li via cfe-commits]

On Thu, Jan 28, 2016 at 2:34 PM, Rong Xu  wrote:
> The previous patch was not good as it failed 58 tests that use
> -fprofile-instr-generate as a cc1 option.
>
> I can see two methods to solve this:
> (1) we change all the 58 tests to use -fprofile-instrument=Clang option.

My vote is on (1) -- get rid of -fprofile-instr-generate as cc1 option
as it is now a subset of what -fprofile-instrument=<...> does.

Changing test cases are mechanical.

David

> (2) Fall back to my previous patch: keep -fprofile-instr-generate as
> the CC1 options and check it in Frontend/CompilerInvocation.cpp. (i.e.
> The redundant else branch Sean was referring to).  We have to do it
> here because we cannot assume driver will append
> -fprofile-instrument=Clang in the case the user uses
> -fprofile-instr-generate as the cc1 option.


>
>
>
> On Thu, Jan 28, 2016 at 12:04 PM, Rong Xu  wrote:
>> xur updated this revision to Diff 46303.
>> xur added a comment.
>>
>> Integrated most recently comments/suggestions from David and Sean.
>>
>> Thanks,
>>
>> -Rong
>>
>>
>> http://reviews.llvm.org/D15829
>>
>> Files:
>>   include/clang/Basic/DiagnosticDriverKinds.td
>>   include/clang/Driver/CC1Options.td
>>   include/clang/Driver/Options.td
>>   include/clang/Frontend/CodeGenOptions.def
>>   include/clang/Frontend/CodeGenOptions.h
>>   lib/CodeGen/BackendUtil.cpp
>>   lib/CodeGen/CGStmt.cpp
>>   lib/CodeGen/CodeGenFunction.cpp
>>   lib/CodeGen/CodeGenFunction.h
>>   lib/CodeGen/CodeGenModule.cpp
>>   lib/CodeGen/CodeGenPGO.cpp
>>   lib/Driver/Tools.cpp
>>   lib/Frontend/CompilerInvocation.cpp
>>   test/CodeGen/Inputs/pgotest.profraw
>>   test/CodeGen/pgo-instrumentation.c
>>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Xinliang David Li via cfe-commits]

On Tue, Feb 2, 2016 at 1:31 PM, Bob Wilson  wrote:
>
>> On Jan 22, 2016, at 1:43 PM, Sean Silva via cfe-commits 
>>  wrote:
>>
>> silvas added a comment.
>>
>> In http://reviews.llvm.org/D15829#333902, @davidxl wrote:
>>
>>> For the longer term, one possible solution is to make FE based
>>> instrumentation only used for coverage testing which can be turned on
>>> with -fcoverage-mapping option (currently, -fcoverage-mapping can not
>>> be used alone and must be used together with
>>> -fprofile-instr-generate). To summarize:
>>>
>>> A. Current behavior:
>>>
>>> ---
>>>
>>> 1. -fprofile-instr-generate turns on FE based instrumentation
>>> 2. -fprofile-instr-generate -fcoverage-mapping turns on FE based 
>>> instrumentation and coverage mapping data generation.
>>> 3. -fprofile-instr-use=<..> assumes profile data from FE instrumentation.
>>>
>>> B. Proposed new behavior:
>>>
>>> 
>>>
>>> 1. -fprofile-instr-generate turns on IR late instrumentation
>>> 2. -fcoverage-mapping turns on FE instrumentation and coverage-mapping
>>> 3. -fprofile-instr-generate -fcoverage-mapping result in compiler warning
>>> 4. -fprofile-instr-use=<> will automatically determine how to use the 
>>> profile data.
>>
>>
>> Very good observation that we can determine FE or IR automatically based on 
>> the input profdata. That simplifies things.
>>
>>> B.2) above can be done today for improved usability.
>>
>>
>> I don't see how this improves usability. In fact, it is confusing because it 
>> hijacks an existing option.
>
> Hijacking an existing option to do something different would definitely be a 
> problem. Please find a way to specify IR-level instrumentation without 
> breaking compatibility. If you want to replace the existing options with 
> something different, we’ll need a transition period of at least 1-2 LLVM 
> releases to migrate.
>

If we remove B.3 above,  then the proposed change (B.2) is essentially
making '-fcoverage-mapping' an alias to '-fprofile-instr-generate
-fcoverage-mapping'.   No existing workflow will be broken and new
users can take advantage of the shortened option.  The reason is that
there will be no users that only use -fcoverage-mapping option alone
and rely on its behavior (which is clang error).


>>
>> Also B.3 causes existing user builds to emit a warning, which is annoying.
>>
>> I would propose the following modification of B:
>>
>> C.:
>>
>> 1. -fprofile-instr-generate defaults to IR instrumentation (i.e. behaves 
>> exactly as before, except that it uses IR instrumentation)
>> 2. -fprofile-instr-generate -fcoverage-mapping turns on frontend 
>> instrumentation and coverage. (i.e. behaves exactly as before)
>> 3. -fprofile-instr-use=<> automatically determines which method to use
>>
>> All existing user workflows continue to work, except for workflows that 
>> attempt to `llvm-profdata merge` some old frontend profile data (e.g. they 
>> have checked-in to version control and represents some special workload) 
>> with the profile data from new binaries.
>
> The coverage mapping adds considerable cost. IR-level instrumentation has 
> some problems that make it undesirable for our workflow, so we need a way to 
> select front-end instrumentation without adding a bunch of unnecessary 
> overhead (generating the coverage mapping when you’re not actually doing 
> coverage testing). I disagree with your assessment that existing workflows 
> would continue to “work” because ours would not.
>
>>
>> Concretely, imagine the following workflow:
>>
>>  clang -fprofile-instr-generate foo.c -o foo
>>  ./foo
>>  llvm-profdata merge default.profraw -o new.profdata
>>  llvm-profdata merge new.profdata 
>> /versioncontrol/some-important-but-expensive-to-reproduce-workload.profdata 
>> -o foo.profdata
>>  clang -fprofile-instr-use=foo.profdata foo.c -o foo_pgo
>>
>> I think this is a reasonable breakage. We would need to add a note in the 
>> release notes. Unfortunately this is not expected breakage if we claim to 
>> have forward compatibility for profdata (which IIRC Apple requires; 
>> @bogner?).
>
> Yes, that is a requirement for us. We need existing profdata to work with 
> newer versions of clang (which is why IR-level instrumentation doesn’t work 
> for us).
>

profile-use can automatically detect FE based profile data and use it
properly. The question is whether we have a need to support merging
profiles from IR and FE instrumentation.



>> But I think this case will be rare and exceptional enough that we can 
>> tolerate it:
>>
>> - a simple immediate workaround is to specify `-fcoverage-mapping` (which 
>> also adds some extra stuff, but works around the issue)
>> - Presumably 
>> /versioncontrol/some-important-but-expensive-to-reproduce-workload.profdata 
>> is regenerated with some frequency which is more frequent than upgrading the 
>> compiler, and so it is likely reasonable to regenerate it alongside a 
>> compiler upgrade, using the workaround 

Re: [PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Xinliang David Li via cfe-commits]

On Wed, Feb 3, 2016 at 12:40 PM, Bob Wilson  wrote:
>
> On Feb 3, 2016, at 12:23 PM, Xinliang David Li  wrote:
>
> On Tue, Feb 2, 2016 at 1:31 PM, Bob Wilson  wrote:
>
>
> On Jan 22, 2016, at 1:43 PM, Sean Silva via cfe-commits
>  wrote:
>
> silvas added a comment.
>
> In http://reviews.llvm.org/D15829#333902, @davidxl wrote:
>
> For the longer term, one possible solution is to make FE based
> instrumentation only used for coverage testing which can be turned on
> with -fcoverage-mapping option (currently, -fcoverage-mapping can not
> be used alone and must be used together with
> -fprofile-instr-generate). To summarize:
>
> A. Current behavior:
>
> ---
>
> 1. -fprofile-instr-generate turns on FE based instrumentation
> 2. -fprofile-instr-generate -fcoverage-mapping turns on FE based
> instrumentation and coverage mapping data generation.
> 3. -fprofile-instr-use=<..> assumes profile data from FE instrumentation.
>
> B. Proposed new behavior:
>
> 
>
> 1. -fprofile-instr-generate turns on IR late instrumentation
> 2. -fcoverage-mapping turns on FE instrumentation and coverage-mapping
> 3. -fprofile-instr-generate -fcoverage-mapping result in compiler warning
> 4. -fprofile-instr-use=<> will automatically determine how to use the
> profile data.
>
>
>
> Very good observation that we can determine FE or IR automatically based on
> the input profdata. That simplifies things.
>
> B.2) above can be done today for improved usability.
>
>
>
> I don't see how this improves usability. In fact, it is confusing because it
> hijacks an existing option.
>
>
> Hijacking an existing option to do something different would definitely be a
> problem. Please find a way to specify IR-level instrumentation without
> breaking compatibility. If you want to replace the existing options with
> something different, we’ll need a transition period of at least 1-2 LLVM
> releases to migrate.
>
>
> If we remove B.3 above,  then the proposed change (B.2) is essentially
> making '-fcoverage-mapping' an alias to '-fprofile-instr-generate
> -fcoverage-mapping'.   No existing workflow will be broken and new
> users can take advantage of the shortened option.  The reason is that
> there will be no users that only use -fcoverage-mapping option alone
> and rely on its behavior (which is clang error).
>
>
> The part I’m concerned about is B.1. The current behavior is that
> -fprofile-instr-generate enabled FE instrumentation. We can’t hijack that to
> do something different, at least without a sufficiently long transition
> period for clients to adapt. We use that to generate PGO profiles even when
> not using -fcoverage-mapping.

yes. I don't see this happening overnight. IR based instrumentation
also needs to get stablized and widely used before the switch can
happen.

>>
> Yes, that is a requirement for us. We need existing profdata to work with
> newer versions of clang (which is why IR-level instrumentation doesn’t work
> for us).
>
>
> profile-use can automatically detect FE based profile data and use it
> properly. The question is whether we have a need to support merging
> profiles from IR and FE instrumentation.
>
>
> I don’t think it makes sense to merge those. They seem like fundamentally
> different kinds of data. The “forward compatibility” requirement is about
> different versions of the FE-based profile data.


great -- one fewer thing to worry about.

>

>
> I want to understand how we can guarantee to support old (FE based)
> profiles with new compilers.  The region to counter id
> mapping/assignment depends on how the AST is generated by the frontend
> and how the AST is traversed. Do we have any guarantee that the new
> compiler can generate them in the same order? How is that enforced?
> The function structural hash generated may also be different (given
> the same source).
>
>
> The FE-based instrumentation uses a custom traversal of the ASTs so that we
> can control the order and make sure it doesn’t change. It still depends on
> the way the ASTs are generated but the AST nodes that are relevant for this
> are unlikely to change in ways that would affect the instrumentation. I
> would love to have a better way to enforce that.

My guess is that IR based instrumentation (which is CFG based) can
produce as stable profile as what FE based instrumentation (when it is
used in a conservative mode that does not require pre-inlining).  CFG
based checksum is also used to detect incompatible changes such that
old profile data can be kept live for a long time (while
gradually/slowly degrades in quality due to the increased number of
mismatches).To get really stable profile, we need to used a source
location based representation (which sample based PGO uses).In
other words, I don't see a big obstacle that prevent IR based
instrumentation from being usable  in such a workflow.

>
> The hashing scheme is specified in a way that does not tie it to the details
> of the compiler ve

r259819 - [PGO] code simplification: use existing VP annotation API /NFC

[Xinliang David Li via cfe-commits]

Author: davidxl
Date: Thu Feb  4 13:54:17 2016
New Revision: 259819

URL: http://llvm.org/viewvc/llvm-project?rev=259819&view=rev
Log:
[PGO] code simplification: use existing VP annotation API /NFC

Modified:
cfe/trunk/lib/CodeGen/CodeGenPGO.cpp

Modified: cfe/trunk/lib/CodeGen/CodeGenPGO.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenPGO.cpp?rev=259819&r1=259818&r2=259819&view=diff
==
--- cfe/trunk/lib/CodeGen/CodeGenPGO.cpp (original)
+++ cfe/trunk/lib/CodeGen/CodeGenPGO.cpp Thu Feb  4 13:54:17 2016
@@ -780,35 +780,11 @@ void CodeGenPGO::valueProfile(CGBuilderT
 // pairs for each function.
 if (NumValueSites[ValueKind] >= ProfRecord->getNumValueSites(ValueKind))
   return;
-uint32_t NV = ProfRecord->getNumValueDataForSite(ValueKind,
- NumValueSites[ValueKind]);
-std::unique_ptr VD =
-ProfRecord->getValueForSite(ValueKind, NumValueSites[ValueKind]);
 
-uint64_t Sum = 0;
-for (uint32_t I = 0; I < NV; ++I)
-  Sum += VD[I].Count;
+llvm::annotateValueSite(CGM.getModule(), *ValueSite, *ProfRecord,
+(llvm::InstrProfValueKind)ValueKind,
+NumValueSites[ValueKind]);
 
-llvm::LLVMContext &Ctx = CGM.getLLVMContext();
-llvm::MDBuilder MDHelper(Ctx);
-SmallVector Vals;
-Vals.push_back(MDHelper.createString("VP"));
-Vals.push_back(MDHelper.createConstant(
-llvm::ConstantInt::get(llvm::Type::getInt32Ty(Ctx), ValueKind)));
-Vals.push_back(MDHelper.createConstant(
-llvm::ConstantInt::get(llvm::Type::getInt64Ty(Ctx), Sum)));
-
-uint32_t MDCount = 3;
-for (uint32_t I = 0; I < NV; ++I) {
-  Vals.push_back(MDHelper.createConstant(
-  llvm::ConstantInt::get(llvm::Type::getInt64Ty(Ctx), VD[I].Value)));
-  Vals.push_back(MDHelper.createConstant(
-  llvm::ConstantInt::get(llvm::Type::getInt64Ty(Ctx), VD[I].Count)));
-  if (--MDCount == 0)
-break;
-}
-ValueSite->setMetadata(
-llvm::LLVMContext::MD_prof, llvm::MDNode::get(Ctx, Vals));
 NumValueSites[ValueKind]++;
   }
 }


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

r259955 - [PGO] Test case update

[Xinliang David Li via cfe-commits]

Author: davidxl
Date: Fri Feb  5 17:36:08 2016
New Revision: 259955

URL: http://llvm.org/viewvc/llvm-project?rev=259955&view=rev
Log:
[PGO] Test case update
 
  Temporarily relax check in test to avoid 
  breakage for format change in LLVM side. Once that is
  done, the test case will be retightened.




Modified:
cfe/trunk/test/CoverageMapping/ir.c
cfe/trunk/test/CoverageMapping/unused_names.c

Modified: cfe/trunk/test/CoverageMapping/ir.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CoverageMapping/ir.c?rev=259955&r1=259954&r2=259955&view=diff
==
--- cfe/trunk/test/CoverageMapping/ir.c (original)
+++ cfe/trunk/test/CoverageMapping/ir.c Fri Feb  5 17:36:08 2016
@@ -9,4 +9,4 @@ int main(void) {
   return 0;
 }
 
-// CHECK: @__llvm_coverage_mapping = internal constant { { i32, i32, i32, i32 
}, [2 x <{ i8*, i32, i32, i64 }>], [{{[0-9]+}} x i8] } { { i32, i32, i32, i32 } 
{ i32 2, i32 {{[0-9]+}}, i32 {{[0-9]+}}, i32 0 }, [2 x <{ i8*, i32, i32, i64 
}>] [<{ i8*, i32, i32, i64 }> <{ i8* getelementptr inbounds ([3 x i8], [3 x 
i8]* @__profn_foo, i32 0, i32 0), i32 3, i32 9, i64 {{[0-9]+}} }>, <{ i8*, i32, 
i32, i64 }> <{ i8* getelementptr inbounds ([4 x i8], [4 x i8]* @__profn_main, 
i32 0, i32 0), i32 4, i32 9, i64 {{[0-9]+}} }>]
+// CHECK: @__llvm_coverage_mapping = internal constant { { i32, i32, i32, i32 
}, [2 x <{{.*}}>], [{{[0-9]+}} x i8] } { { i32, i32, i32, i32 } { i32 2, i32 
{{[0-9]+}}, i32 {{[0-9]+}}, i32 {{[0-9]+}} }, [2 x <{{.*}}>] [<{{.*}}> 
<{{.*}}>, <{{.*}}> <{{.*}}>]

Modified: cfe/trunk/test/CoverageMapping/unused_names.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CoverageMapping/unused_names.c?rev=259955&r1=259954&r2=259955&view=diff
==
--- cfe/trunk/test/CoverageMapping/unused_names.c (original)
+++ cfe/trunk/test/CoverageMapping/unused_names.c Fri Feb  5 17:36:08 2016
@@ -4,9 +4,9 @@
 
 // Since foo is never emitted, there should not be a profile name for it.
 
-// CHECK-DAG: @__profn_bar = {{.*}} [3 x i8] c"bar", section 
"{{.*}}__llvm_prf_names"
-// CHECK-DAG: @__profn_baz = {{.*}} [3 x i8] c"baz", section 
"{{.*}}__llvm_prf_names"
-// CHECK-DAG: @__profn_unused_names.c_qux = {{.*}} [18 x i8] 
c"unused_names.c:qux", section "{{.*}}__llvm_prf_names"
+// CHECK-DAG: @__profn_bar = {{.*}} [3 x i8] c"bar"
+// CHECK-DAG: @__profn_baz = {{.*}} [3 x i8] c"baz"
+// CHECK-DAG: @__profn_unused_names.c_qux = {{.*}} [18 x i8] 
c"unused_names.c:qux"
 
 // SYSHEADER-NOT: @__profn_foo =
 


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

r260021 - [PGO] add profile/coverage test cases for defaulted ctor/ctors

[Xinliang David Li via cfe-commits]

Author: davidxl
Date: Sun Feb  7 00:57:29 2016
New Revision: 260021

URL: http://llvm.org/viewvc/llvm-project?rev=260021&view=rev
Log:
[PGO] add profile/coverage test cases for defaulted ctor/ctors

Added:
cfe/trunk/test/Profile/def-ctors.cpp
cfe/trunk/test/Profile/def-dtors.cpp

Added: cfe/trunk/test/Profile/def-ctors.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Profile/def-ctors.cpp?rev=260021&view=auto
==
--- cfe/trunk/test/Profile/def-ctors.cpp (added)
+++ cfe/trunk/test/Profile/def-ctors.cpp Sun Feb  7 00:57:29 2016
@@ -0,0 +1,49 @@
+// RUN: %clang_cc1 -x c++ -std=c++11 %s -triple x86_64-unknown-linux-gnu
+// -main-file-name def-ctors.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-ctors.cpp -o - -emit-llvm -fprofile-instrument=clang
+// -fcoverage-mapping | FileCheck --check-prefix=COVMAP %s
+
+struct Base {
+  int B;
+  Base() : B(2) {}
+  Base(const struct Base &b2) {
+if (b2.B == 0) {
+  B = b2.B + 1;
+} else
+  B = b2.B;
+  }
+};
+
+struct Derived : public Base {
+  Derived(const Derived &) = default;
+  // PGOGEN-DAG: define {{.*}}@_ZN7DerivedC2ERKS_
+  // PGOGEN-DAG: %pgocount = load {{.*}} @__profc__ZN7DerivedC2ERKS_
+  // PGOGEN-DAG: {{.*}}add{{.*}}%pgocount, 1
+  // PGOGEN-DAG: store{{.*}}@__profc__ZN7DerivedC2ERKS_
+  Derived() = default;
+  // PGOGEN-DAG: define {{.*}}@_ZN7DerivedC2Ev
+  // PGOGEN-DAG: %pgocount = load {{.*}} @__profc__ZN7DerivedC2Ev
+  // PGOGEN-DAG: {{.*}}add{{.*}}%pgocount, 1
+  // PGOGEN-DAG: store{{.*}}@__profc__ZN7DerivedC2Ev
+
+  // Check that coverage mapping has 6 function records including
+  // the defaulted Derived::Derived(const Derived), and Derived::Derived()
+  // methds.
+  // COVMAP: @__llvm_coverage_mapping = {{.*}} { { i32, i32, i32, i32 }, [6 x
+  // <{{.*}}>],
+  int I;
+  int J;
+  int getI() { return I; }
+};
+
+Derived dd;
+int g;
+int main() {
+  Derived dd2(dd);
+
+  g = dd2.getI();
+  return 0;
+}

Added: cfe/trunk/test/Profile/def-dtors.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Profile/def-dtors.cpp?rev=260021&view=auto
==
--- cfe/trunk/test/Profile/def-dtors.cpp (added)
+++ cfe/trunk/test/Profile/def-dtors.cpp Sun Feb  7 00:57:29 2016
@@ -0,0 +1,39 @@
+// RUN: %clang_cc1 -x c++ -std=c++11 %s -triple x86_64-unknown-linux-gnu
+// -main-file-name def-dtors.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-dtors.cpp -o - -emit-llvm -fprofile-instrument=clang
+// -fcoverage-mapping | FileCheck --check-prefix=COVMAP %s
+
+struct Base {
+  int B;
+  Base(int B_) : B(B_) {}
+  ~Base() {}
+};
+
+struct Derived : public Base {
+  Derived(int K) : Base(K), I(K), J(K) {}
+  ~Derived() = default;
+  // PGOGEN-LABEL: define {{.*}}@_ZN7DerivedD2Ev
+  // PGOGEN: %pgocount = load {{.*}} @__profc__ZN7DerivedD2Ev
+  // PGOGEN: {{.*}}add{{.*}}%pgocount, 1
+  // PGOGEN: store{{.*}}@__profc__ZN7DerivedD2Ev
+
+  // Check that coverage mapping has 6 function records including
+  // the default destructor in the derived class.
+  // COVMAP: @__llvm_coverage_mapping = {{.*}} { { i32, i32, i32, i32 }, [6 x
+  // <{{.*}}>],
+
+  int I;
+  int J;
+  int getI() { return I; }
+};
+
+Derived dd(100);
+int g;
+int main() {
+  Derived dd2(dd.getI());
+  g = dd2.getI();
+  return 0;
+}


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

r260022 - Fix test case problem(caused by clang-format

[Xinliang David Li via cfe-commits]

Author: davidxl
Date: Sun Feb  7 01:13:18 2016
New Revision: 260022

URL: http://llvm.org/viewvc/llvm-project?rev=260022&view=rev
Log:
Fix test case problem(caused by clang-format

Modified:
cfe/trunk/test/Profile/def-ctors.cpp
cfe/trunk/test/Profile/def-dtors.cpp

Modified: cfe/trunk/test/Profile/def-ctors.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Profile/def-ctors.cpp?rev=260022&r1=260021&r2=260022&view=diff
==
--- cfe/trunk/test/Profile/def-ctors.cpp (original)
+++ cfe/trunk/test/Profile/def-ctors.cpp Sun Feb  7 01:13:18 2016
@@ -1,10 +1,6 @@
-// RUN: %clang_cc1 -x c++ -std=c++11 %s -triple x86_64-unknown-linux-gnu
-// -main-file-name def-ctors.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-ctors.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-ctors.cpp -o - -emit-llvm -fprofile-instrument=clang
-// -fcoverage-mapping | FileCheck --check-prefix=COVMAP %s
+// RUN: %clang_cc1 -x c++ -std=c++11 %s -triple x86_64-unknown-linux-gnu 
-main-file-name def-ctors.cpp -o - -emit-llvm -fprofile-instrument=clang 
-fcoverage-mapping | FileCheck --check-prefix=COVMAP %s
 
 struct Base {
   int B;

Modified: cfe/trunk/test/Profile/def-dtors.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Profile/def-dtors.cpp?rev=260022&r1=260021&r2=260022&view=diff
==
--- cfe/trunk/test/Profile/def-dtors.cpp (original)
+++ cfe/trunk/test/Profile/def-dtors.cpp Sun Feb  7 01:13:18 2016
@@ -1,10 +1,6 @@
-// RUN: %clang_cc1 -x c++ -std=c++11 %s -triple x86_64-unknown-linux-gnu
-// -main-file-name def-dtors.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-dtors.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-dtors.cpp -o - -emit-llvm -fprofile-instrument=clang
-// -fcoverage-mapping | FileCheck --check-prefix=COVMAP %s
+// RUN: %clang_cc1 -x c++ -std=c++11 %s -triple x86_64-unknown-linux-gnu 
-main-file-name def-dtors.cpp -o - -emit-llvm -fprofile-instrument=clang 
-fcoverage-mapping | FileCheck --check-prefix=COVMAP %s
 
 struct Base {
   int B;


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

r260126 - Simplify test cases

[Xinliang David Li via cfe-commits]

Author: davidxl
Date: Mon Feb  8 13:14:14 2016
New Revision: 260126

URL: http://llvm.org/viewvc/llvm-project?rev=260126&view=rev
Log:
Simplify test cases

Modified:
cfe/trunk/test/Profile/def-ctors.cpp
cfe/trunk/test/Profile/def-dtors.cpp

Modified: cfe/trunk/test/Profile/def-ctors.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Profile/def-ctors.cpp?rev=260126&r1=260125&r2=260126&view=diff
==
--- cfe/trunk/test/Profile/def-ctors.cpp (original)
+++ cfe/trunk/test/Profile/def-ctors.cpp Mon Feb  8 13:14:14 2016
@@ -5,12 +5,7 @@
 struct Base {
   int B;
   Base() : B(2) {}
-  Base(const struct Base &b2) {
-if (b2.B == 0) {
-  B = b2.B + 1;
-} else
-  B = b2.B;
-  }
+  Base(const struct Base &b2) {}
 };
 
 struct Derived : public Base {
@@ -28,18 +23,14 @@ struct Derived : public Base {
   // Check that coverage mapping has 6 function records including
   // the defaulted Derived::Derived(const Derived), and Derived::Derived()
   // methds.
-  // COVMAP: @__llvm_coverage_mapping = {{.*}} { { i32, i32, i32, i32 }, [6 x
+  // COVMAP: @__llvm_coverage_mapping = {{.*}} { { i32, i32, i32, i32 }, [5 x
   // <{{.*}}>],
-  int I;
-  int J;
-  int getI() { return I; }
 };
 
 Derived dd;
 int g;
 int main() {
   Derived dd2(dd);
-
-  g = dd2.getI();
+  g = dd2.B;
   return 0;
 }

Modified: cfe/trunk/test/Profile/def-dtors.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Profile/def-dtors.cpp?rev=260126&r1=260125&r2=260126&view=diff
==
--- cfe/trunk/test/Profile/def-dtors.cpp (original)
+++ cfe/trunk/test/Profile/def-dtors.cpp Mon Feb  8 13:14:14 2016
@@ -9,7 +9,7 @@ struct Base {
 };
 
 struct Derived : public Base {
-  Derived(int K) : Base(K), I(K), J(K) {}
+  Derived(int K) : Base(K) {}
   ~Derived() = default;
   // PGOGEN-LABEL: define {{.*}}@_ZN7DerivedD2Ev
   // PGOGEN: %pgocount = load {{.*}} @__profc__ZN7DerivedD2Ev
@@ -18,18 +18,13 @@ struct Derived : public Base {
 
   // Check that coverage mapping has 6 function records including
   // the default destructor in the derived class.
-  // COVMAP: @__llvm_coverage_mapping = {{.*}} { { i32, i32, i32, i32 }, [6 x
+  // COVMAP: @__llvm_coverage_mapping = {{.*}} { { i32, i32, i32, i32 }, [5 x
   // <{{.*}}>],
-
-  int I;
-  int J;
-  int getI() { return I; }
 };
 
-Derived dd(100);
-int g;
 int main() {
-  Derived dd2(dd.getI());
-  g = dd2.getI();
+  Derived dd2(10);
+  if (dd2.B != 10)
+return 1;
   return 0;
 }


___
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 &b2) {
>> +if (b2.B == 0) {
>> +  B = b2.B + 1;
>> +} else
>> +  B = b2.B;
>> +return this;
>> +  }
>> +};
>> +
>> +struct A : public B {
>> +  A &operator=(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

[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 &b) {}
>> +  void operator=(const B &&b) {}
>
>
> Probably best to make these canonical to avoid confusion:
>
> B &operator=(const B&);
> B &operator=(B&&);
>
> (& they don't need definitions - just declarations)

Will change.

>
> Also, neither of these are the move /constructor/, just the move operator.
> Not sure if Vedant just used the wrong terminology, or whether it's worth
> testing the move/copy ctors too, to check that they do the right thing as

I added tests for copy ctors, and plan to add move ctor test soon.

> well. (if all of these things use the same codepath, I don't see a great
> benefit in having separate tests for them (but you can add them here if you
> like) - I'm just suggesting a manual verification in case those need a
> separate fix

the ctor and assignment op do not share the same path -- the ctor path
is working as expected without the fix -- or do you mean there is no
need to cover both copy and move variants?
>
>>
>> +};
>> +
>> +struct A {
>> +  A &operator=(const A &) = default;
>
>
> Is the fix/codepath specifically about explicitly defaulted ops?

yes -- explicitly defaulted. There are some test coverage already for
implicitly declared ctors (but not assignment op -- probably worth
adding some testing too).

> Or just any
> compiler-generated ones? (you could drop these lines if it's about any
> compiler-generated ones, might be simpler/more obvious that it's not about
> the "= default" feature)

Other compiler generated ones are handled differently.

thanks,

David

>
>>
>> +  // PGOGEN: define {{.*}}@_ZN1AaSERKS_(
>> +  // PGOGEN: %pgocount = load {{.*}} @__profc__ZN1AaSERKS_
>> +  // PGOGEN: {{.*}}add{{.*}}%pgocount, 1
>> +  // PGOGEN: store{{.*}}@__profc__ZN1AaSERKS_
>> +  A &operator=(A &&) = default;
>>
>> +  // PGOGEN: define {{.*}}@_ZN1AaSEOS_
>> +  // PGOGEN: %pgocount = load {{.*}} @__profc__ZN1AaSEOS_
>> +  // PGOGEN: {{.*}}add{{.*}}%pgocount, 1
>> +  // PGOGEN: store{{.*}}@__profc__ZN1AaSEOS_
>> +
>> +  // Check that coverage mapping includes 6 function records including
>> the
>> +  // defaulted copy and move operators: A::operator=
>> +  // COVMAP: @__llvm_coverage_mapping = {{.*}} { { i32, i32, i32, i32 },
>> [5 x <{{.*}}>],
>> +  B b;
>> +};
>> +
>> +int main() {
>> +  A a1, a2;
>> +  a1 = a2;
>> +  a2 = static_cast(a1);
>
>
> An option, though not necessarily better, would be to just take the address
> of the special members:
>
> auto (B::*x)(const B&) = &bar::operator=;
> auto (B::*x)(B&&) = &bar::operator=;
>
> In short, what I'm picturing, in total:
>
> struct A {
>   A &operator=(const A&);
>   A &operator=(A&&);
> };
>
> struct B {
>   A a;
> };
>
> auto (B::*x)(const B&) = &B::operator=;
> auto (B::*x)(B&&) = &B::operator=;
>
> Also, this test should probably be in clang, since it's a clang code
> change/fix.
>
>
>>
>> +  return 0;
>> +}
>> Index: lib/CodeGen/CGClass.cpp
>> ===
>> --- lib/CodeGen/CGClass.cpp
>> +++ lib/CodeGen/CGClass.cpp
>> @@ -1608,6 +1608,7 @@
>>
>>LexicalScope Scope(*this, RootCS->getSourceRange());
>>
>> +  incrementProfileCounter(RootCS);
>>AssignmentMemcpyizer AM(*this, AssignOp, Args);
>>for (auto *I : RootCS->body())
>>  AM.emitAssignment(I);
>>
>>
>>
>> ___
>> llvm-commits mailing list
>> llvm-comm...@lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18489: [PGO, clang] Comment how function pointers for indirect calls are mapped to function names

[Xinliang David Li via cfe-commits]

On Mon, Mar 28, 2016 at 10:40 AM, Adam Nemet  wrote:

> anemet added inline comments.
>
> 
> Comment at: lib/CodeGen/CodeGenPGO.cpp:758
> @@ -757,1 +757,3 @@
>
> +  // During instrumentation, function pointers are collected for the
> different
> +  // indirect call targets.  Then as part of the conversion from raw to
> merged
> 
> davidxl wrote:
> > anemet wrote:
> > > davidxl wrote:
> > > > CodeGenPGO::valueProfile is a common API which can also be used for
> other kinds of value profile, so the comments belong to the callsite of
> this method in CGCall.cpp.
> > > >
> > > > Suggested wording:
> > > >
> > > > For indirect function call value profiling, the addresses of the
> target functions are profiled by the instrumented code. The target
> addresses are written in the raw profile data and converted to target
> function name's MD5 hash by the profile reader during deserialization.
> > > Hi David,
> > >
> > > Thanks for taking a look.
> > >
> > > I don't mind rewording the comment if you have some specific issues
> but your version drops many of the details that was painful for me to
> discover.  I carefully worded my comment to describe some of the design
> details for indirect profiling that are not covered elsewhere:
> > >
> > > 1) the remapping from function pointer to hashes of function names
> happens during profdata merging
> > >
> > >   It was surprisingly hard to figure out what the PGO file contained
> for indirect call targets: function pointers or func name hashes?  And of
> course as stated, the answer is -- it depends.
> > >
> > > 2) the remapping happens pretty deep in the infrastructure code during
> deserializing of the rawdata
> > >
> > >   I was looking at several more logical places to find where this
> happens and this was a bit unexpected location for this functionality.
> > >
> > > 3) how do we know the function addresses necessary for the mapping
> from function address -> func name -> hash
> > >
> > >   The entire code to populate the FunctionAddr using macro magic in
> InstrProfiling::getOrCreateRegionCounters is pretty hard to follow.  I much
> rather have an overview of the logic somewhere centrally.
> > >
> > > From the above list I feel that your version dropped 1 and 3 and only
> kept 2.  Of course, feel free to add more context to my comments and fix
> inaccuracies/oversimplifications but I would want want to drop any of the
> points mentioned above.
> > >
> > > Thanks.
> > Actually bullet 1 is not dropped from my suggested version: each time
> when a raw profile data is read, the reader interface will covert the
> address into MD5 -- profile merging is simply a user of the reader.
> >
> > About bullet 3, it is ok to add it if you think it is useful. ( I did
> not suggest it because ProfData is needed here is not for the purpose of
> conversion, but to pass the context for the runtime to find/set the counter
> arrays.)
> > Actually bullet 1 is not dropped from my suggested version: each time
> when a raw profile data is read, the reader interface will covert the
> address into MD5 -- profile merging is simply a user of the reader.
>
> Sure but that is still pretty implicit.  I'd like to describe this in
> terms of the well established phases of PGO: instrumentation, profile
> merge,  recompile with profile data.
>
> How about:
>
> ```
> For indirect function call value profiling, the addresses of the target
> functions are profiled by the instrumented code. The target addresses are
> written in the raw profile data and converted to target function name's MD5
> hash by the profile reader during deserialization.  Typically, this happens
> when the the raw profile data is read during profile merging.
>


This sounds great!



> ```
>
> > About bullet 3, it is ok to add it if you think it is useful. ( I did
> not suggest it because ProfData is needed here is not for the purpose of
> conversion, but to pass the context for the runtime to find/set the counter
> arrays.)
>
> I am not completely sure I understand what you're saying here but if you
> mean that the comment does not really apply to the surrounding code then
> that I guess is expected.


Right.


> I wanted to give a high-level overview of *what* we collect at run-time,
> and *how* we map that into function names hashes that are then used in the
> IR.
>

I am fine putting them here.

So LGTM with your revised version.

David


>

I can also put this in the function comment if you think that's better.
>
>
>
> http://reviews.llvm.org/D18489
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18489: [PGO, clang] Comment how function pointers for indirect calls are mapped to function names

[Xinliang David Li via cfe-commits]

On Mon, Mar 28, 2016 at 12:31 PM, Adam Nemet  wrote:

> anemet added inline comments.
>
> 
> Comment at: lib/CodeGen/CodeGenPGO.cpp:758
> @@ -757,1 +757,3 @@
>
> +  // During instrumentation, function pointers are collected for the
> different
> +  // indirect call targets.  Then as part of the conversion from raw to
> merged
> 
> anemet wrote:
> > davidxl wrote:
> > > anemet wrote:
> > > > davidxl wrote:
> > > > > CodeGenPGO::valueProfile is a common API which can also be used
> for other kinds of value profile, so the comments belong to the callsite of
> this method in CGCall.cpp.
> > > > >
> > > > > Suggested wording:
> > > > >
> > > > > For indirect function call value profiling, the addresses of the
> target functions are profiled by the instrumented code. The target
> addresses are written in the raw profile data and converted to target
> function name's MD5 hash by the profile reader during deserialization.
> > > > Hi David,
> > > >
> > > > Thanks for taking a look.
> > > >
> > > > I don't mind rewording the comment if you have some specific issues
> but your version drops many of the details that was painful for me to
> discover.  I carefully worded my comment to describe some of the design
> details for indirect profiling that are not covered elsewhere:
> > > >
> > > > 1) the remapping from function pointer to hashes of function names
> happens during profdata merging
> > > >
> > > >   It was surprisingly hard to figure out what the PGO file contained
> for indirect call targets: function pointers or func name hashes?  And of
> course as stated, the answer is -- it depends.
> > > >
> > > > 2) the remapping happens pretty deep in the infrastructure code
> during deserializing of the rawdata
> > > >
> > > >   I was looking at several more logical places to find where this
> happens and this was a bit unexpected location for this functionality.
> > > >
> > > > 3) how do we know the function addresses necessary for the mapping
> from function address -> func name -> hash
> > > >
> > > >   The entire code to populate the FunctionAddr using macro magic in
> InstrProfiling::getOrCreateRegionCounters is pretty hard to follow.  I much
> rather have an overview of the logic somewhere centrally.
> > > >
> > > > From the above list I feel that your version dropped 1 and 3 and
> only kept 2.  Of course, feel free to add more context to my comments and
> fix inaccuracies/oversimplifications but I would want want to drop any of
> the points mentioned above.
> > > >
> > > > Thanks.
> > > Actually bullet 1 is not dropped from my suggested version: each time
> when a raw profile data is read, the reader interface will covert the
> address into MD5 -- profile merging is simply a user of the reader.
> > >
> > > About bullet 3, it is ok to add it if you think it is useful. ( I did
> not suggest it because ProfData is needed here is not for the purpose of
> conversion, but to pass the context for the runtime to find/set the counter
> arrays.)
> > > Actually bullet 1 is not dropped from my suggested version: each time
> when a raw profile data is read, the reader interface will covert the
> address into MD5 -- profile merging is simply a user of the reader.
> >
> > Sure but that is still pretty implicit.  I'd like to describe this in
> terms of the well established phases of PGO: instrumentation, profile
> merge,  recompile with profile data.
> >
> > How about:
> >
> > ```
> > For indirect function call value profiling, the addresses of the target
> functions are profiled by the instrumented code. The target addresses are
> written in the raw profile data and converted to target function name's MD5
> hash by the profile reader during deserialization.  Typically, this happens
> when the the raw profile data is read during profile merging.
> > ```
> >
> > > About bullet 3, it is ok to add it if you think it is useful. ( I did
> not suggest it because ProfData is needed here is not for the purpose of
> conversion, but to pass the context for the runtime to find/set the counter
> arrays.)
> >
> > I am not completely sure I understand what you're saying here but if you
> mean that the comment does not really apply to the surrounding code then
> that I guess is expected.  I wanted to give a high-level overview of *what*
> we collect at run-time, and *how* we map that into function names hashes
> that are then used in the IR.
> >
> > I can also put this in the function comment if you think that's better.
> >
> David,
>
> > CodeGenPGO::valueProfile is a common API which can also be used for
> other kinds of value profile, so the comments belong to the callsite of
> this method in CGCall.cpp.
>
> How would you actually feel about moving this comment to InstrProfData.inc
> as well, just before the definition of IPVK_IndirectCallTarget?
>
> I think that's a better place in terms of its centrality since this
> applies to both early and late instrumentation.  What do you think?
>
>

Sounds ok -- but p

Re: [PATCH] D18489: [PGO, clang] Comment how function pointers for indirect calls are mapped to function names

[Xinliang David Li via cfe-commits]

What I meant is that putting the comments in InstrProfData.inc file, and
update the one in CodeGenPGO.cpp to reference that.

David

On Mon, Mar 28, 2016 at 12:35 PM, Xinliang David Li 
wrote:

>
>
> On Mon, Mar 28, 2016 at 12:31 PM, Adam Nemet  wrote:
>
>> anemet added inline comments.
>>
>> 
>> Comment at: lib/CodeGen/CodeGenPGO.cpp:758
>> @@ -757,1 +757,3 @@
>>
>> +  // During instrumentation, function pointers are collected for the
>> different
>> +  // indirect call targets.  Then as part of the conversion from raw to
>> merged
>> 
>> anemet wrote:
>> > davidxl wrote:
>> > > anemet wrote:
>> > > > davidxl wrote:
>> > > > > CodeGenPGO::valueProfile is a common API which can also be used
>> for other kinds of value profile, so the comments belong to the callsite of
>> this method in CGCall.cpp.
>> > > > >
>> > > > > Suggested wording:
>> > > > >
>> > > > > For indirect function call value profiling, the addresses of the
>> target functions are profiled by the instrumented code. The target
>> addresses are written in the raw profile data and converted to target
>> function name's MD5 hash by the profile reader during deserialization.
>> > > > Hi David,
>> > > >
>> > > > Thanks for taking a look.
>> > > >
>> > > > I don't mind rewording the comment if you have some specific issues
>> but your version drops many of the details that was painful for me to
>> discover.  I carefully worded my comment to describe some of the design
>> details for indirect profiling that are not covered elsewhere:
>> > > >
>> > > > 1) the remapping from function pointer to hashes of function names
>> happens during profdata merging
>> > > >
>> > > >   It was surprisingly hard to figure out what the PGO file
>> contained for indirect call targets: function pointers or func name
>> hashes?  And of course as stated, the answer is -- it depends.
>> > > >
>> > > > 2) the remapping happens pretty deep in the infrastructure code
>> during deserializing of the rawdata
>> > > >
>> > > >   I was looking at several more logical places to find where this
>> happens and this was a bit unexpected location for this functionality.
>> > > >
>> > > > 3) how do we know the function addresses necessary for the mapping
>> from function address -> func name -> hash
>> > > >
>> > > >   The entire code to populate the FunctionAddr using macro magic in
>> InstrProfiling::getOrCreateRegionCounters is pretty hard to follow.  I much
>> rather have an overview of the logic somewhere centrally.
>> > > >
>> > > > From the above list I feel that your version dropped 1 and 3 and
>> only kept 2.  Of course, feel free to add more context to my comments and
>> fix inaccuracies/oversimplifications but I would want want to drop any of
>> the points mentioned above.
>> > > >
>> > > > Thanks.
>> > > Actually bullet 1 is not dropped from my suggested version: each time
>> when a raw profile data is read, the reader interface will covert the
>> address into MD5 -- profile merging is simply a user of the reader.
>> > >
>> > > About bullet 3, it is ok to add it if you think it is useful. ( I did
>> not suggest it because ProfData is needed here is not for the purpose of
>> conversion, but to pass the context for the runtime to find/set the counter
>> arrays.)
>> > > Actually bullet 1 is not dropped from my suggested version: each time
>> when a raw profile data is read, the reader interface will covert the
>> address into MD5 -- profile merging is simply a user of the reader.
>> >
>> > Sure but that is still pretty implicit.  I'd like to describe this in
>> terms of the well established phases of PGO: instrumentation, profile
>> merge,  recompile with profile data.
>> >
>> > How about:
>> >
>> > ```
>> > For indirect function call value profiling, the addresses of the target
>> functions are profiled by the instrumented code. The target addresses are
>> written in the raw profile data and converted to target function name's MD5
>> hash by the profile reader during deserialization.  Typically, this happens
>> when the the raw profile data is read during profile merging.
>> > ```
>> >
>> > > About bullet 3, it is ok to add it if you think it is useful. ( I did
>> not suggest it because ProfData is needed here is not for the purpose of
>> conversion, but to pass the context for the runtime to find/set the counter
>> arrays.)
>> >
>> > I am not completely sure I understand what you're saying here but if
>> you mean that the comment does not really apply to the surrounding code
>> then that I guess is expected.  I wanted to give a high-level overview of
>> *what* we collect at run-time, and *how* we map that into function names
>> hashes that are then used in the IR.
>> >
>> > I can also put this in the function comment if you think that's better.
>> >
>> David,
>>
>> > CodeGenPGO::valueProfile is a common API which can also be used for
>> other kinds of value profile, so the comments belong to the callsite of
>> this method in CGCall.cpp.
>>
>>

Re: [PATCH] D18489: [PGO, clang] Comment how function pointers for indirect calls are mapped to function names

[Xinliang David Li via cfe-commits]

Sure.

thanks,

David

On Mon, Mar 28, 2016 at 12:41 PM, Adam Nemet  wrote:

> anemet added a comment.
>
> In http://reviews.llvm.org/D18489#384851, @davidxl wrote:
>
> > What I meant is that putting the comments in InstrProfData.inc file, and
> >  update the one in CodeGenPGO.cpp to reference that.
>
>
> Sounds good.  You mean CGCall.cpp instead of CodeGenPGO.cpp though,
> correct?
>
> So to summarize, I'll move this comment to InstrProfData.inc and just have
> a reference to it in CGCall.cpp before the call to valueProfile.
>
> OK?
>
>
> http://reviews.llvm.org/D18489
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r266638 - Update InstrProf pass creator API reference

[Xinliang David Li via cfe-commits]

Author: davidxl
Date: Mon Apr 18 12:48:12 2016
New Revision: 266638

URL: http://llvm.org/viewvc/llvm-project?rev=266638&view=rev
Log:
Update InstrProf pass creator API reference

Modified:
cfe/trunk/lib/CodeGen/BackendUtil.cpp

Modified: cfe/trunk/lib/CodeGen/BackendUtil.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/BackendUtil.cpp?rev=266638&r1=266637&r2=266638&view=diff
==
--- cfe/trunk/lib/CodeGen/BackendUtil.cpp (original)
+++ cfe/trunk/lib/CodeGen/BackendUtil.cpp Mon Apr 18 12:48:12 2016
@@ -439,7 +439,7 @@ void EmitAssemblyHelper::CreatePasses(Mo
 InstrProfOptions Options;
 Options.NoRedZone = CodeGenOpts.DisableRedZone;
 Options.InstrProfileOutput = CodeGenOpts.InstrProfileOutput;
-MPM->add(createInstrProfilingPass(Options));
+MPM->add(createInstrProfilingLegacyPass(Options));
   }
   if (CodeGenOpts.hasProfileIRInstr()) {
 if (!CodeGenOpts.InstrProfileOutput.empty())


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

Re: [PATCH] D18624: [PGO] PGOFuncName meta data if PGOFuncName is different from function's raw name.

[Xinliang David Li via cfe-commits]

Can you also try IR based instrumentation?  -fprofile-instr-generate
-Xclang=-fprofile-instrument=llvm

David

On Tue, Apr 19, 2016 at 9:40 AM, Adam Nemet  wrote:

> anemet added a comment.
>
> As discussed under http://reviews.llvm.org/D17864, I did a run with this
> and I don't get the indirect call promoted that calls static functions in
> povray.   I will dig more but do I need to pass some extra flag?
>
>
> http://reviews.llvm.org/D18624
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r250994 - clang driver toolchain refactoring

[Xinliang David Li via cfe-commits]

Author: davidxl
Date: Thu Oct 22 01:15:31 2015
New Revision: 250994

URL: http://llvm.org/viewvc/llvm-project?rev=250994&view=rev
Log:
clang driver toolchain refactoring

In this patch, the file static method addProfileRT is
moved to be a virtual member function of base ToolChain class.
This allows derived toolchain to override the default behavior
easily and make it consistent with Darwin toolchain (a TODO was
added for this refactoring - now removed). A new helper method
is also introduced to test if instrumentation profile option
is turned on or not.

Differential Revision: http://reviews.llvm.org/D13326

Modified:
cfe/trunk/include/clang/Driver/ToolChain.h
cfe/trunk/lib/Driver/ToolChain.cpp
cfe/trunk/lib/Driver/ToolChains.cpp
cfe/trunk/lib/Driver/ToolChains.h
cfe/trunk/lib/Driver/Tools.cpp

Modified: cfe/trunk/include/clang/Driver/ToolChain.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/ToolChain.h?rev=250994&r1=250993&r2=250994&view=diff
==
--- cfe/trunk/include/clang/Driver/ToolChain.h (original)
+++ cfe/trunk/include/clang/Driver/ToolChain.h Thu Oct 22 01:15:31 2015
@@ -258,6 +258,12 @@ public:
 StringRef Component,
 bool Shared = false) const;
 
+  const char *getCompilerRTArgString(const llvm::opt::ArgList &Args,
+ StringRef Component,
+ bool Shared = false) const;
+  /// needsProfileRT - returns true if instrumentation profile is on.
+  static bool needsProfileRT(const llvm::opt::ArgList &Args);
+
   /// IsUnwindTablesDefault - Does this tool chain use -funwind-tables
   /// by default.
   virtual bool IsUnwindTablesDefault() const;
@@ -378,8 +384,11 @@ public:
   /// global flags for unsafe floating point math, add it and return true.
   ///
   /// This checks for presence of the -Ofast, -ffast-math or -funsafe-math 
flags.
-  virtual bool
-  AddFastMathRuntimeIfAvailable(const llvm::opt::ArgList &Args,
+  virtual bool AddFastMathRuntimeIfAvailable(
+  const llvm::opt::ArgList &Args, llvm::opt::ArgStringList &CmdArgs) const;
+  /// addProfileRTLibs - When -fprofile-instr-profile is specified, add profile
+  /// runtime library, otherwise return false.
+  virtual void addProfileRTLibs(const llvm::opt::ArgList &Args,
 llvm::opt::ArgStringList &CmdArgs) const;
 
   /// \brief Return sanitizers which are available in this toolchain.

Modified: cfe/trunk/lib/Driver/ToolChain.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChain.cpp?rev=250994&r1=250993&r2=250994&view=diff
==
--- cfe/trunk/lib/Driver/ToolChain.cpp (original)
+++ cfe/trunk/lib/Driver/ToolChain.cpp Thu Oct 22 01:15:31 2015
@@ -301,9 +301,28 @@ std::string ToolChain::getCompilerRT(con
   return Path.str();
 }
 
+const char *ToolChain::getCompilerRTArgString(const llvm::opt::ArgList &Args,
+  StringRef Component,
+  bool Shared) const {
+  return Args.MakeArgString(getCompilerRT(Args, Component, Shared));
+}
+
+bool ToolChain::needsProfileRT(const ArgList &Args) {
+  if (Args.hasFlag(options::OPT_fprofile_arcs, options::OPT_fno_profile_arcs,
+   false) ||
+  Args.hasArg(options::OPT_fprofile_generate) ||
+  Args.hasArg(options::OPT_fprofile_generate_EQ) ||
+  Args.hasArg(options::OPT_fprofile_instr_generate) ||
+  Args.hasArg(options::OPT_fprofile_instr_generate_EQ) ||
+  Args.hasArg(options::OPT_fcreate_profile) ||
+  Args.hasArg(options::OPT_coverage))
+return true;
+
+  return false;
+}
+
 Tool *ToolChain::SelectTool(const JobAction &JA) const {
-  if (getDriver().ShouldUseClangCompiler(JA))
-return getClang();
+  if (getDriver().ShouldUseClangCompiler(JA)) return getClang();
   Action::ActionClass AC = JA.getKind();
   if (AC == Action::AssembleJobClass && useIntegratedAs())
 return getClangAs();
@@ -491,9 +510,16 @@ void ToolChain::addClangTargetOptions(co
 
 void ToolChain::addClangWarningOptions(ArgStringList &CC1Args) const {}
 
+void ToolChain::addProfileRTLibs(const llvm::opt::ArgList &Args,
+ llvm::opt::ArgStringList &CmdArgs) const {
+  if (!needsProfileRT(Args)) return;
+
+  CmdArgs.push_back(getCompilerRTArgString(Args, "profile"));
+  return;
+}
+
 ToolChain::RuntimeLibType ToolChain::GetRuntimeLibType(
-  const ArgList &Args) const
-{
+const ArgList &Args) const {
   if (Arg *A = Args.getLastArg(options::OPT_rtlib_EQ)) {
 StringRef Value = A->getValue();
 if (Value == "compiler-rt")

Modified: cfe/trunk/lib/Driver/ToolChains.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains.cpp?rev=250994&r1=250993&r2=250994&view=diff
=

r251072 - Use newly introduced interfaces in LLVM (NFC)

[Xinliang David Li via cfe-commits]

Author: davidxl
Date: Thu Oct 22 17:25:11 2015
New Revision: 251072

URL: http://llvm.org/viewvc/llvm-project?rev=251072&view=rev
Log:
Use newly introduced interfaces in LLVM (NFC)

Replaced references to raw strings in instrumentation
and coverage code.

Modified:
cfe/trunk/lib/CodeGen/CodeGenPGO.cpp
cfe/trunk/lib/CodeGen/CoverageMappingGen.cpp

Modified: cfe/trunk/lib/CodeGen/CodeGenPGO.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenPGO.cpp?rev=251072&r1=251071&r2=251072&view=diff
==
--- cfe/trunk/lib/CodeGen/CodeGenPGO.cpp (original)
+++ cfe/trunk/lib/CodeGen/CodeGenPGO.cpp Thu Oct 22 17:25:11 2015
@@ -73,7 +73,7 @@ void CodeGenPGO::createFuncNameVar(llvm:
   llvm::ConstantDataArray::getString(CGM.getLLVMContext(), FuncName, 
false);
   FuncNameVar =
   new llvm::GlobalVariable(CGM.getModule(), Value->getType(), true, 
Linkage,
-   Value, "__llvm_profile_name_" + FuncName);
+   Value, llvm::getInstrProfNameVarPrefix() + 
FuncName);
 
   // Hide the symbol so that we correctly get a copy for each executable.
   if (!llvm::GlobalValue::isLocalLinkage(FuncNameVar->getLinkage()))

Modified: cfe/trunk/lib/CodeGen/CoverageMappingGen.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CoverageMappingGen.cpp?rev=251072&r1=251071&r2=251072&view=diff
==
--- cfe/trunk/lib/CodeGen/CoverageMappingGen.cpp (original)
+++ cfe/trunk/lib/CodeGen/CoverageMappingGen.cpp Thu Oct 22 17:25:11 2015
@@ -879,7 +879,7 @@ static bool isMachO(const CodeGenModule
 }
 
 static StringRef getCoverageSection(const CodeGenModule &CGM) {
-  return isMachO(CGM) ? "__DATA,__llvm_covmap" : "__llvm_covmap";
+  return llvm::getInstrProfCoverageSectionName(isMachO(CGM));
 }
 
 static void dump(llvm::raw_ostream &OS, StringRef FunctionName,
@@ -1011,7 +1011,7 @@ void CoverageMappingModuleGen::emit() {
   auto CovData = new llvm::GlobalVariable(CGM.getModule(), CovDataTy, true,
   llvm::GlobalValue::InternalLinkage,
   CovDataVal,
-  "__llvm_coverage_mapping");
+  llvm::getCoverageMappingVarName());
 
   CovData->setSection(getCoverageSection(CGM));
   CovData->setAlignment(8);


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

Re: [PATCH] D14030: Use linker option to reference profile runtime hook (Linux)

[Xinliang David Li via cfe-commits]

On Mon, Oct 26, 2015 at 8:44 AM, Vedant Kumar  wrote:

> vsk added a comment.
>
> This looks good to me. Does this mean we will get rid of
> `getInstrProfRuntimeHookVarUseFuncName`?
>

not yet -- note that this is only enabled for Linux toolchain only for
now.  If it works for all platforms, we can get rid of the hook use
function all together.

thanks,

David

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

r251385 - Create undef reference to profile hook symbol

[Xinliang David Li via cfe-commits]

Author: davidxl
Date: Tue Oct 27 00:15:35 2015
New Revision: 251385

URL: http://llvm.org/viewvc/llvm-project?rev=251385&view=rev
Log:
Create undef reference to profile hook symbol 

Create undef reference to profile hook symbol when
PGO instrumentation is turned on. This allows 
LLVM to omit emission of hook variable use method
for every single module instrumented.

Modified:
cfe/trunk/lib/Driver/ToolChains.cpp
cfe/trunk/lib/Driver/ToolChains.h

Modified: cfe/trunk/lib/Driver/ToolChains.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains.cpp?rev=251385&r1=251384&r2=251385&view=diff
==
--- cfe/trunk/lib/Driver/ToolChains.cpp (original)
+++ cfe/trunk/lib/Driver/ToolChains.cpp Tue Oct 27 00:15:35 2015
@@ -25,6 +25,7 @@
 #include "llvm/Option/ArgList.h"
 #include "llvm/Option/OptTable.h"
 #include "llvm/Option/Option.h"
+#include "llvm/ProfileData/InstrProf.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
@@ -3811,6 +3812,18 @@ SanitizerMask Linux::getSupportedSanitiz
   return Res;
 }
 
+void Linux::addProfileRTLibs(const llvm::opt::ArgList &Args,
+ llvm::opt::ArgStringList &CmdArgs) const {
+  if (!needsProfileRT(Args)) return;
+
+  // Add linker option -u__llvm_runtime_variable to cause runtime
+  // initialization module to be linked in.
+  if (!Args.hasArg(options::OPT_coverage))
+CmdArgs.push_back(Args.MakeArgString(
+Twine("-u", llvm::getInstrProfRuntimeHookVarName(;
+  ToolChain::addProfileRTLibs(Args, CmdArgs);
+}
+
 /// DragonFly - DragonFly tool chain which can call as(1) and ld(1) directly.
 
 DragonFly::DragonFly(const Driver &D, const llvm::Triple &Triple,

Modified: cfe/trunk/lib/Driver/ToolChains.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains.h?rev=251385&r1=251384&r2=251385&view=diff
==
--- cfe/trunk/lib/Driver/ToolChains.h (original)
+++ cfe/trunk/lib/Driver/ToolChains.h Tue Oct 27 00:15:35 2015
@@ -744,6 +744,8 @@ public:
   llvm::opt::ArgStringList &CC1Args) const override;
   bool isPIEDefault() const override;
   SanitizerMask getSupportedSanitizers() const override;
+  void addProfileRTLibs(const llvm::opt::ArgList &Args,
+llvm::opt::ArgStringList &CmdArgs) const override;
 
   std::string Linker;
   std::vector ExtraOpts;


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

r251611 - Fix a soon to be invalid test

[Xinliang David Li via cfe-commits]

Author: davidxl
Date: Wed Oct 28 23:04:07 2015
New Revision: 251611

URL: http://llvm.org/viewvc/llvm-project?rev=251611&view=rev
Log:
Fix a soon to be invalid test

Remove a check that won't be valid when LLVM stops
emitting runtime hook user function.

Modified:
cfe/trunk/test/Profile/gcc-flag-compatibility.c

Modified: cfe/trunk/test/Profile/gcc-flag-compatibility.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Profile/gcc-flag-compatibility.c?rev=251611&r1=251610&r2=251611&view=diff
==
--- cfe/trunk/test/Profile/gcc-flag-compatibility.c (original)
+++ cfe/trunk/test/Profile/gcc-flag-compatibility.c Wed Oct 28 23:04:07 2015
@@ -9,7 +9,6 @@
 
 // Check that -fprofile-generate uses the runtime default profile file.
 // RUN: %clang %s -c -S -o - -emit-llvm -fprofile-generate | FileCheck 
-check-prefix=PROFILE-GEN %s
-// PROFILE-GEN: @__llvm_profile_runtime = external global i32
 // PROFILE-GEN-NOT: call void @__llvm_profile_override_default_filename
 // PROFILE-GEN-NOT: declare void @__llvm_profile_override_default_filename(i8*)
 


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

r252147 - Use profile data template file for covmap func record (NFC)

[Xinliang David Li via cfe-commits]

Author: davidxl
Date: Wed Nov  4 23:46:39 2015
New Revision: 252147

URL: http://llvm.org/viewvc/llvm-project?rev=252147&view=rev
Log:
Use profile data template file for covmap func record (NFC)

Modified:
cfe/trunk/lib/CodeGen/CoverageMappingGen.cpp

Modified: cfe/trunk/lib/CodeGen/CoverageMappingGen.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CoverageMappingGen.cpp?rev=252147&r1=252146&r2=252147&view=diff
==
--- cfe/trunk/lib/CodeGen/CoverageMappingGen.cpp (original)
+++ cfe/trunk/lib/CodeGen/CoverageMappingGen.cpp Wed Nov  4 23:46:39 2015
@@ -910,24 +910,23 @@ static void dump(llvm::raw_ostream &OS,
 }
 
 void CoverageMappingModuleGen::addFunctionMappingRecord(
-llvm::GlobalVariable *FunctionName, StringRef FunctionNameValue,
-uint64_t FunctionHash, const std::string &CoverageMapping) {
+llvm::GlobalVariable *NamePtr, StringRef NameValue,
+uint64_t FuncHash, const std::string &CoverageMapping) {
   llvm::LLVMContext &Ctx = CGM.getLLVMContext();
-  auto *Int32Ty = llvm::Type::getInt32Ty(Ctx);
-  auto *Int64Ty = llvm::Type::getInt64Ty(Ctx);
-  auto *Int8PtrTy = llvm::Type::getInt8PtrTy(Ctx);
   if (!FunctionRecordTy) {
-llvm::Type *FunctionRecordTypes[] = {Int8PtrTy, Int32Ty, Int32Ty, Int64Ty};
+#define COVMAP_FUNC_RECORD(Type, LLVMType, Name, Init) LLVMType,
+llvm::Type *FunctionRecordTypes[] = {
+  #include "llvm/ProfileData/InstrProfData.inc"
+};
 FunctionRecordTy =
 llvm::StructType::get(Ctx, makeArrayRef(FunctionRecordTypes),
   /*isPacked=*/true);
   }
 
+  #define COVMAP_FUNC_RECORD(Type, LLVMType, Name, Init) Init,
   llvm::Constant *FunctionRecordVals[] = {
-  llvm::ConstantExpr::getBitCast(FunctionName, Int8PtrTy),
-  llvm::ConstantInt::get(Int32Ty, FunctionNameValue.size()),
-  llvm::ConstantInt::get(Int32Ty, CoverageMapping.size()),
-  llvm::ConstantInt::get(Int64Ty, FunctionHash)};
+  #include "llvm/ProfileData/InstrProfData.inc"
+  };
   FunctionRecords.push_back(llvm::ConstantStruct::get(
   FunctionRecordTy, makeArrayRef(FunctionRecordVals)));
   CoverageMappings += CoverageMapping;
@@ -949,7 +948,7 @@ void CoverageMappingModuleGen::addFuncti
 Expressions, Regions);
 if (Reader.read())
   return;
-dump(llvm::outs(), FunctionNameValue, Expressions, Regions);
+dump(llvm::outs(), NameValue, Expressions, Regions);
   }
 }
 


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

r252434 - [PGO] Code cleanup [NFC]

[Xinliang David Li via cfe-commits]

Author: davidxl
Date: Sun Nov  8 18:04:16 2015
New Revision: 252434

URL: http://llvm.org/viewvc/llvm-project?rev=252434&view=rev
Log:
[PGO] Code cleanup [NFC]

Use interfaces defined in LLVM to create FuncName
and FuncNameVar.

Modified:
cfe/trunk/lib/CodeGen/CodeGenPGO.cpp

Modified: cfe/trunk/lib/CodeGen/CodeGenPGO.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenPGO.cpp?rev=252434&r1=252433&r2=252434&view=diff
==
--- cfe/trunk/lib/CodeGen/CodeGenPGO.cpp (original)
+++ cfe/trunk/lib/CodeGen/CodeGenPGO.cpp Sun Nov  8 18:04:16 2015
@@ -28,58 +28,17 @@ using namespace CodeGen;
 
 void CodeGenPGO::setFuncName(StringRef Name,
  llvm::GlobalValue::LinkageTypes Linkage) {
-  StringRef RawFuncName = Name;
-
-  // Function names may be prefixed with a binary '1' to indicate
-  // that the backend should not modify the symbols due to any platform
-  // naming convention. Do not include that '1' in the PGO profile name.
-  if (RawFuncName[0] == '\1')
-RawFuncName = RawFuncName.substr(1);
-
-  FuncName = RawFuncName;
-  if (llvm::GlobalValue::isLocalLinkage(Linkage)) {
-// For local symbols, prepend the main file name to distinguish them.
-// Do not include the full path in the file name since there's no guarantee
-// that it will stay the same, e.g., if the files are checked out from
-// version control in different locations.
-if (CGM.getCodeGenOpts().MainFileName.empty())
-  FuncName = FuncName.insert(0, ":");
-else
-  FuncName = FuncName.insert(0, CGM.getCodeGenOpts().MainFileName + ":");
-  }
+  FuncName = llvm::getPGOFuncName(Name, Linkage, 
CGM.getCodeGenOpts().MainFileName);
 
   // If we're generating a profile, create a variable for the name.
   if (CGM.getCodeGenOpts().ProfileInstrGenerate)
-createFuncNameVar(Linkage);
+FuncNameVar = llvm::createPGOFuncNameVar(CGM.getModule(), Linkage, 
FuncName);
 }
 
 void CodeGenPGO::setFuncName(llvm::Function *Fn) {
   setFuncName(Fn->getName(), Fn->getLinkage());
 }
 
-void CodeGenPGO::createFuncNameVar(llvm::GlobalValue::LinkageTypes Linkage) {
-  // We generally want to match the function's linkage, but 
available_externally
-  // and extern_weak both have the wrong semantics, and anything that doesn't
-  // need to link across compilation units doesn't need to be visible at all.
-  if (Linkage == llvm::GlobalValue::ExternalWeakLinkage)
-Linkage = llvm::GlobalValue::LinkOnceAnyLinkage;
-  else if (Linkage == llvm::GlobalValue::AvailableExternallyLinkage)
-Linkage = llvm::GlobalValue::LinkOnceODRLinkage;
-  else if (Linkage == llvm::GlobalValue::InternalLinkage ||
-   Linkage == llvm::GlobalValue::ExternalLinkage)
-Linkage = llvm::GlobalValue::PrivateLinkage;
-
-  auto *Value =
-  llvm::ConstantDataArray::getString(CGM.getLLVMContext(), FuncName, 
false);
-  FuncNameVar =
-  new llvm::GlobalVariable(CGM.getModule(), Value->getType(), true, 
Linkage,
-   Value, llvm::getInstrProfNameVarPrefix() + 
FuncName);
-
-  // Hide the symbol so that we correctly get a copy for each executable.
-  if (!llvm::GlobalValue::isLocalLinkage(FuncNameVar->getLinkage()))
-FuncNameVar->setVisibility(llvm::GlobalValue::HiddenVisibility);
-}
-
 namespace {
 /// \brief Stable hasher for PGO region counters.
 ///


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

Re: [PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass from pipeline

[Xinliang David Li via cfe-commits]

On Mon, Jun 14, 2021 at 3:59 PM Roman Lebedev via Phabricator <
revi...@reviews.llvm.org> wrote:

> lebedev.ri added a subscriber: MaskRay.
> lebedev.ri added a comment.
>
> In D104099#2815531 , @wenlei
> wrote:
>
> > In D104099#2814167 , @davidxl
> wrote:
> >
> >> Adding Wei to help measure performance impact on our internal
> workloads.  Also add Wenlei to help measure impact with FB's workloads.
> >
> > Measured perf using FB internal workload w/ and w/o this pass, result is
> neutral.
>
> Thank you for checking!
>
> So far, it seems the reaction to this proposal has been overwhelmingly
> positive.
> Does anyone else wish to chime in? Should i land this? @asbirlea @MaskRay ?
>

Wei is doing more measurement @google. Please wait for the response.

David


>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D104099/new/
>
> https://reviews.llvm.org/D104099
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12247: [libc++] remove possible trailing padding from aligned_storage

[Xinliang David Li via cfe-commits]

On Wed, Aug 26, 2015 at 11:11 AM, Eric Fiselier  wrote:
> EricWF added a comment.
>
> In http://reviews.llvm.org/D12247#233323, @davidxl wrote:
>
>> We certainly need a fix without breaking ABI. Is there a ABI conformance 
>> test for libcxx?
>
>
> Well this patch definitely breaks the ABI. This was my attempt at fixing the 
> problem in `std::function` that might not be ABI breaking..
> https://gist.github.com/EricWF/3a35b140a66d4826a9d0.

Great. My question is more general about testing any potentially ABI
breaking changes.

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

Re: [PATCH] D12247: [libc++] remove possible trailing padding from aligned_storage

[Xinliang David Li via cfe-commits]

On Wed, Aug 26, 2015 at 3:38 PM, Eric Fiselier  wrote:
> EricWF added a comment.
>
> In http://reviews.llvm.org/D12247#233717, @yiranwang wrote:
>
>> Hi Eric,
>>
>> Could you please explain a bit more what is broken specifically? As we can 
>> see, sizeof(), _Len, and _Align, and alignof() of "aligned_storage" are all 
>> not changed.
>
>
> That's correct. At the risk of sounding like a broken record those fields 
> have not changed because doing so would break the ABI. Instead my patch fixes 
> the issue your seeing by simply not using __buf_ unless its the correct size 
> and correctly aligned.

This seems safe. The downside is that some cases which uses internal
buffer before now will use dynamic allocator which might have some
small performance and memory regression.

David

>
> The alignment is the important part. Previously we didn't check if `Fn` was 
> alignment compatible with `__buf_`.
>
>
> http://reviews.llvm.org/D12247
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12247: [libc++] remove possible trailing padding from aligned_storage

[Xinliang David Li via cfe-commits]

On Thu, Aug 27, 2015 at 2:57 PM, Yiran Wang  wrote:
> yiranwang added a comment.
>
> In http://reviews.llvm.org/D12247#234533, @EricWF wrote:
>
>> So this patch LGTM. Sorry for being slow to understand it. However I want to 
>> see two things before it lands.
>>
>> 1. I would like to see a test of some sort that the resulting type has the 
>> same size and alignment requirements it did before the change. I'm not sure 
>> what the best way to test this is.
>> 2. I would like @mclow.lists to approve this as well. I just want to be 
>> cautious.

Re. testing, I think what Eric means is to add some compile time
assertion to make sure the size does not change before and after.

>
>
> I think now we are on the same page about these bugs.
> Also, thanks for your review and findings (hopefully fix it soon too).
>
> For the test, we compile it with -O3 -fstrict-aliasing -finline-limit=300, 
> and as mentioned before the target is AARCH64+ANDROID.
>
>
> http://reviews.llvm.org/D12247
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D21823: [Driver] Add flags for enabling both types of PGO Instrumentation

[Xinliang David Li via cfe-commits]

The new behavior works really well. There is one follow up change I would
like to discuss.

The EQ form of the option -fprofile-generate= takes a directory name as the
argument instead of the filename. Currently the compiler will create a
default 'default.profraw' name for it, but this means, online merging won't
be available unless environment variable is also used, which is not
convenient.  I propose we change the default behavior to enable profile
online merging (i.e., use default_%m.profraw as the default name) for the
following reasons:

1) There is almost no downside enabling profiling merging by default -- new
capability is enabled with no lost functionality
2) WIth profile merging enabled for instrumented programs with instrumented
shared libs, each lib will dump its own profile data in the same dir, but
users of -fprofile-generate= option usually do not care about what/how many
raw profile names are in the directory (note that GCC's behavior is one
profile per object module), they just pack up the while directory.
llvm-profdata also support merging (for indexed profile) with
input-list-file option.
3) GCC's has online merging support, so having online merging by default
with this option matches up better the claim that it is a GCC compatible
option.

What do you think?

thanks,

David



On Sat, Jul 9, 2016 at 4:39 PM, Sean Silva  wrote:

> silvas added a comment.
>
> In http://reviews.llvm.org/D21823#479418, @davidxl wrote:
>
> > I should have brought it up earlier, but I forgot.I think a better
> (and simpler) proposal is to make -fprofile-generate and -fprofile-use turn
> on IR based PGO.
> >
> > -fprofile-generate and -fprofile-use options were introduced by Diego
> (cc'ed) recently for GCC option compatibility. At that time, IR based PGO
> was not yet ready, so they were made aliases to FE instrumentation options
> -fprofile-instr-generate and -fprofile-instr-use.Now I think it is time
> to make it right.   The documentation only says that these two options are
> gcc compatible options to control profile generation and use, but does not
> specify the underlying implementation. It is also likely that Google is the
> only user of this option. If a user using this option really want FE
> instrumentation, there is always -fprofile-instr-generate to use.  It also
> more likely that IR instrumentation is what user wants when using GCC
> compatible options (as they behave more similarly).
>
>
> This SGTM.
>
> It may cause some user confusion since there is no obvious distinction
> between "profile generate" and "profile instr generate" from a user
> perspective. But we can avoid that with improved documentation.
>
> My main concern is that the existing documentation already says that
> -fprofile-generate behaves identically to -fprofile-instr-generate
> http://clang.llvm.org/docs/UsersManual.html#cmdoption-fprofile-generate
> However, I think it is reasonable to change this behavior (and of course
> the documentation), as users of this option are likely using it for
> compatibility with GCC and so they likely don't care about the specifics of
> clang FEPGO.
> We probably want to at least leave a small note in the documentation
> regarding this change of behavior.
>
>
> Repository:
>   rL LLVM
>
> http://reviews.llvm.org/D21823
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D21823: [Driver] Add flags for enabling both types of PGO Instrumentation

[Xinliang David Li via cfe-commits]

On Tue, Jul 19, 2016 at 5:06 PM, Vedant Kumar  wrote:

>
> > On Jul 19, 2016, at 5:01 PM, Xinliang David Li 
> wrote:
> >
> > The new behavior works really well. There is one follow up change I
> would like to discuss.
> >
> > The EQ form of the option -fprofile-generate= takes a directory name as
> the argument instead of the filename. Currently the compiler will create a
> default 'default.profraw' name for it, but this means, online merging won't
> be available unless environment variable is also used, which is not
> convenient.  I propose we change the default behavior to enable profile
> online merging (i.e., use default_%m.profraw as the default name) for the
> following reasons:
> >
> > 1) There is almost no downside enabling profiling merging by default --
> new capability is enabled with no lost functionality
>
> To be clear, this is a behavior change, but IMO the new behavior would be
> better.
>
Instead of creating programs that clobber over existing profiles by default
> they would merge into a set of profiles.
>
>
I consider this a bug fix -- as it makes a broken behavior into a working
one ;)

The only real behavior change is the default file name and number of
produced profile files may change -- but it is more likely the users of
-fprofile-generate=<> option expect to see the new behavior vs the old.

David



>
> > 2) WIth profile merging enabled for instrumented programs with
> instrumented shared libs, each lib will dump its own profile data in the
> same dir, but users of -fprofile-generate= option usually do not care about
> what/how many raw profile names are in the directory (note that GCC's
> behavior is one profile per object module), they just pack up the while
> directory. llvm-profdata also support merging (for indexed profile) with
> input-list-file option.
> > 3) GCC's has online merging support, so having online merging by default
> with this option matches up better the claim that it is a GCC compatible
> option.
> >
> > What do you think?
>
> + 1
>
> thanks,
> vedant
>
> >
> > thanks,
> >
> > David
> >
> >
> >
> > On Sat, Jul 9, 2016 at 4:39 PM, Sean Silva 
> wrote:
> > silvas added a comment.
> >
> > In http://reviews.llvm.org/D21823#479418, @davidxl wrote:
> >
> > > I should have brought it up earlier, but I forgot.I think a better
> (and simpler) proposal is to make -fprofile-generate and -fprofile-use turn
> on IR based PGO.
> > >
> > > -fprofile-generate and -fprofile-use options were introduced by Diego
> (cc'ed) recently for GCC option compatibility. At that time, IR based PGO
> was not yet ready, so they were made aliases to FE instrumentation options
> -fprofile-instr-generate and -fprofile-instr-use.Now I think it is time
> to make it right.   The documentation only says that these two options are
> gcc compatible options to control profile generation and use, but does not
> specify the underlying implementation. It is also likely that Google is the
> only user of this option. If a user using this option really want FE
> instrumentation, there is always -fprofile-instr-generate to use.  It also
> more likely that IR instrumentation is what user wants when using GCC
> compatible options (as they behave more similarly).
> >
> >
> > This SGTM.
> >
> > It may cause some user confusion since there is no obvious distinction
> between "profile generate" and "profile instr generate" from a user
> perspective. But we can avoid that with improved documentation.
> >
> > My main concern is that the existing documentation already says that
> -fprofile-generate behaves identically to -fprofile-instr-generate
> http://clang.llvm.org/docs/UsersManual.html#cmdoption-fprofile-generate
> > However, I think it is reasonable to change this behavior (and of course
> the documentation), as users of this option are likely using it for
> compatibility with GCC and so they likely don't care about the specifics of
> clang FEPGO.
> > We probably want to at least leave a small note in the documentation
> regarding this change of behavior.
> >
> >
> > Repository:
> >   rL LLVM
> >
> > http://reviews.llvm.org/D21823
> >
> >
> >
> >
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r276207 - [Profile] Document new profile file name modifiers

[Xinliang David Li via cfe-commits]

Author: davidxl
Date: Wed Jul 20 18:32:50 2016
New Revision: 276207

URL: http://llvm.org/viewvc/llvm-project?rev=276207&view=rev
Log:
[Profile] Document new profile file name modifiers

Differential Revision: http://reviews.llvm.org/D22593



Modified:
cfe/trunk/docs/UsersManual.rst

Modified: cfe/trunk/docs/UsersManual.rst
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/UsersManual.rst?rev=276207&r1=276206&r2=276207&view=diff
==
--- cfe/trunk/docs/UsersManual.rst (original)
+++ cfe/trunk/docs/UsersManual.rst Wed Jul 20 18:32:50 2016
@@ -1470,8 +1470,13 @@ instrumentation:
 
 2. Run the instrumented executable with inputs that reflect the typical usage.
By default, the profile data will be written to a ``default.profraw`` file
-   in the current directory. You can override that default by setting the
-   ``LLVM_PROFILE_FILE`` environment variable to specify an alternate file.
+   in the current directory. You can override that default by using option
+   ``-fprofile-instr-generate=`` or by setting the ``LLVM_PROFILE_FILE`` 
+   environment variable to specify an alternate file. If non-default file name
+   is specified by both the environment variable and the command line option,
+   the environment variable takes precedence. The file name pattern specified
+   can include different modifiers: ``%p``, ``%h``, and ``%m``.
+
Any instance of ``%p`` in that file name will be replaced by the process
ID, so that you can easily distinguish the profile output from multiple
runs.
@@ -1480,6 +1485,33 @@ instrumentation:
 
  $ LLVM_PROFILE_FILE="code-%p.profraw" ./code
 
+   The modifier ``%h`` can be used in scenarios where the same instrumented
+   binary is run in multiple different host machines dumping profile data
+   to a shared network based storage. The ``%h`` specifier will be substituted
+   with the hostname so that profiles collected from different hosts do not
+   clobber each other.
+
+   While the use of ``%p`` specifier can reduce the likelihood for the profiles
+   dumped from different processes to clobber each other, such clobbering can 
still
+   happen because of the ``pid`` re-use by the OS. Another side-effect of using
+   ``%p`` is that the storage requirement for raw profile data files is greatly
+   increased.  To avoid issues like this, the ``%m`` specifier can used in the 
profile
+   name.  When this specifier is used, the profiler runtime will substitute 
``%m``
+   with a unique integer identifier associated with the instrumented binary. 
Additionally,
+   multiple raw profiles dumped from different processes that share a file 
system (can be
+   on different hosts) will be automatically merged by the profiler runtime 
during the
+   dumping. If the program links in multiple instrumented shared libraries, 
each library
+   will dump the profile data into its own profile data file (with its unique 
integer
+   id embedded in the profile name). Note that the merging enabled by ``%m`` 
is for raw
+   profile data generated by profiler runtime. The resulting merged "raw" 
profile data
+   file still needs to be converted to a different format expected by the 
compiler (
+   see step 3 below).
+
+   .. code-block:: console
+
+ $ LLVM_PROFILE_FILE="code-%m.profraw" ./code
+
+
 3. Combine profiles from multiple runs and convert the "raw" profile format to
the input expected by clang. Use the ``merge`` command of the
``llvm-profdata`` tool to do this.


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

Re: [PATCH] D22593: [Profile] document %h and %m

[Xinliang David Li via cfe-commits]

ok

David

On Wed, Jul 20, 2016 at 4:32 PM, Sean Silva  wrote:

> silvas accepted this revision.
> silvas added a comment.
> This revision is now accepted and ready to land.
>
> LGTM (also, another small suggestion).
>
>
> 
> Comment at: docs/UsersManual.rst:1502
> @@ +1501,3 @@
> +   multiple raw profiles dumped from different processes (running on the
> same or
> +   different hosts) will be automatically merged by the profiler runtime
> during the
> +   dumping. If the program links in multiple instrumented shared
> libraries, each library
> 
> "running on the same or different hosts" should probably just be "that
> share a file system" to clarify that we depend on the filesystem guarantees
> and aren't doing anything else special.
>
>
> https://reviews.llvm.org/D22593
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r276356 - [profile] update test case with interface change.

[Xinliang David Li via cfe-commits]

Author: davidxl
Date: Thu Jul 21 18:19:39 2016
New Revision: 276356

URL: http://llvm.org/viewvc/llvm-project?rev=276356&view=rev
Log:
[profile] update test case with interface change.

See http://reviews.llvm.org/D22613, http://reviews.llvm.org/D22614

Modified:
cfe/trunk/test/Profile/c-generate.c
cfe/trunk/test/Profile/gcc-flag-compatibility.c

Modified: cfe/trunk/test/Profile/c-generate.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Profile/c-generate.c?rev=276356&r1=276355&r2=276356&view=diff
==
--- cfe/trunk/test/Profile/c-generate.c (original)
+++ cfe/trunk/test/Profile/c-generate.c Thu Jul 21 18:19:39 2016
@@ -3,9 +3,7 @@
 // RUN: %clang_cc1 %s -o - -emit-llvm -fprofile-instrument=none | FileCheck %s 
--check-prefix=PROF-INSTR-NONE
 // RUN: not %clang_cc1 %s -o - -emit-llvm -fprofile-instrument=garbage 2>&1 | 
FileCheck %s --check-prefix=PROF-INSTR-GARBAGE
 //
-// PROF-INSTR-PATH: private constant [24 x i8] c"c-generate-test.profraw\00"
-// PROF-INSTR-PATH: call void @__llvm_profile_override_default_filename(i8* 
getelementptr inbounds ([24 x i8], [24 x i8]* @0, i32 0, i32 0))
-// PROF-INSTR-PATH: declare void @__llvm_profile_override_default_filename(i8*)
+// PROF-INSTR-PATH: constant [24 x i8] c"c-generate-test.profraw\00"
 //
 // PROF-INSTR-NONE-NOT: @__profn_main
 // PROF-INSTR-GARBAGE: invalid PGO instrumentor in argument 
'-fprofile-instrument=garbage'

Modified: cfe/trunk/test/Profile/gcc-flag-compatibility.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Profile/gcc-flag-compatibility.c?rev=276356&r1=276355&r2=276356&view=diff
==
--- cfe/trunk/test/Profile/gcc-flag-compatibility.c (original)
+++ cfe/trunk/test/Profile/gcc-flag-compatibility.c Thu Jul 21 18:19:39 2016
@@ -7,17 +7,12 @@
 // -fprofile-use=Uses the profile file /default.profdata
 // -fprofile-use=/file   Uses the profile file /file
 
-// FIXME: IRPGO shouldn't use the override API when no profraw name is given.
-// Check that -fprofile-generate overrides the default profraw.
 // RUN: %clang %s -c -S -o - -emit-llvm -fprofile-generate | FileCheck 
-check-prefix=PROFILE-GEN %s
-// PROFILE-GEN: call void @__llvm_profile_override_default_filename
-// PROFILE-GEN: declare void @__llvm_profile_override_default_filename(i8*)
+// PROFILE-GEN: __llvm_profile_filename
 
 // Check that -fprofile-generate=/path/to generates /path/to/default.profraw
 // RUN: %clang %s -c -S -o - -emit-llvm -fprofile-generate=/path/to | 
FileCheck -check-prefix=PROFILE-GEN-EQ %s
-// PROFILE-GEN-EQ: private constant [25 x i8] 
c"/path/to{{/|\\5C}}default.profraw\00"
-// PROFILE-GEN-EQ: call void @__llvm_profile_override_default_filename(i8* 
getelementptr inbounds ([25 x i8], [25 x i8]* @0, i32 0, i32 0))
-// PROFILE-GEN-EQ: declare void @__llvm_profile_override_default_filename(i8*)
+// PROFILE-GEN-EQ: constant [25 x i8] c"/path/to{{/|\\5C}}default.profraw\00"
 
 // Check that -fprofile-use=some/path reads some/path/default.profdata
 // RUN: rm -rf %t.dir


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

r276484 - [Profile] Enable profile merging with -fprofile-generat[=]

[Xinliang David Li via cfe-commits]

Author: davidxl
Date: Fri Jul 22 17:25:01 2016
New Revision: 276484

URL: http://llvm.org/viewvc/llvm-project?rev=276484&view=rev
Log:
[Profile] Enable profile merging with -fprofile-generat[=]

This patch enables raw profile merging for this option which is the
new intended behavior.



Modified:
cfe/trunk/docs/UsersManual.rst
cfe/trunk/lib/CodeGen/BackendUtil.cpp
cfe/trunk/lib/Driver/Tools.cpp
cfe/trunk/test/Driver/clang_f_opts.c
cfe/trunk/test/Profile/gcc-flag-compatibility.c

Modified: cfe/trunk/docs/UsersManual.rst
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/UsersManual.rst?rev=276484&r1=276483&r2=276484&view=diff
==
--- cfe/trunk/docs/UsersManual.rst (original)
+++ cfe/trunk/docs/UsersManual.rst Fri Jul 22 17:25:01 2016
@@ -1546,27 +1546,31 @@ profile creation and use.
   The ``-fprofile-generate`` and ``-fprofile-generate=`` flags will use
   an alterantive instrumentation method for profile generation. When
   given a directory name, it generates the profile file
-  ``default.profraw`` in the directory named ``dirname``. If ``dirname``
-  does not exist, it will be created at runtime. The environment variable
-  ``LLVM_PROFILE_FILE`` can be used to override the directory and
-  filename for the profile file at runtime. For example,
+  ``default_%m.profraw`` in the directory named ``dirname`` if specified.
+  If ``dirname`` does not exist, it will be created at runtime. ``%m`` 
specifier
+  will be substibuted with a unique id documented in step 2 above. In other 
words,
+  with ``-fprofile-generate[=]`` option, the "raw" profile data 
automatic
+  merging is turned on by default, so there will no longer any risk of profile
+  clobbering from different running processes.  For example,
 
   .. code-block:: console
 
 $ clang++ -O2 -fprofile-generate=yyy/zzz code.cc -o code
 
   When ``code`` is executed, the profile will be written to the file
-  ``yyy/zzz/default.profraw``. This can be altered at runtime via the
-  ``LLVM_PROFILE_FILE`` environment variable:
+  ``yyy/zzz/default_.profraw``.
 
-  .. code-block:: console
+  To generate the profile data file with the compiler readable format, the 
+  ``llvm-profdata`` tool can be used with the profile directory as the input:
+
+   .. code-block:: console
 
-$ LLVM_PROFILE_FILE=/tmp/myprofile/code.profraw ./code
+ $ llvm-profdata merge -output=code.profdata yyy/zzz/
 
-  The above invocation will produce the profile file
-  ``/tmp/myprofile/code.profraw`` instead of ``yyy/zzz/default.profraw``.
-  Notice that ``LLVM_PROFILE_FILE`` overrides the directory *and* the file
-  name for the profile file.
+ If the user wants to turn off the auto-merging feature, or simply override the
+ the profile dumping path specified at command line, the environment variable
+ ``LLVM_PROFILE_FILE`` can still be used to override
+ the directory and filename for the profile file at runtime.
 
 .. option:: -fprofile-use[=]
 

Modified: cfe/trunk/lib/CodeGen/BackendUtil.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/BackendUtil.cpp?rev=276484&r1=276483&r2=276484&view=diff
==
--- cfe/trunk/lib/CodeGen/BackendUtil.cpp (original)
+++ cfe/trunk/lib/CodeGen/BackendUtil.cpp Fri Jul 22 17:25:01 2016
@@ -456,7 +456,7 @@ void EmitAssemblyHelper::CreatePasses(le
 if (!CodeGenOpts.InstrProfileOutput.empty())
   PMBuilder.PGOInstrGen = CodeGenOpts.InstrProfileOutput;
 else
-  PMBuilder.PGOInstrGen = "default.profraw";
+  PMBuilder.PGOInstrGen = "default_%m.profraw";
   }
   if (CodeGenOpts.hasProfileIRUse())
 PMBuilder.PGOInstrUse = CodeGenOpts.ProfileInstrumentUsePath;

Modified: cfe/trunk/lib/Driver/Tools.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Tools.cpp?rev=276484&r1=276483&r2=276484&view=diff
==
--- cfe/trunk/lib/Driver/Tools.cpp (original)
+++ cfe/trunk/lib/Driver/Tools.cpp Fri Jul 22 17:25:01 2016
@@ -3609,7 +3609,7 @@ static void addPGOAndCoverageFlags(Compi
 if (PGOGenerateArg->getOption().matches(
 options::OPT_fprofile_generate_EQ)) {
   SmallString<128> Path(PGOGenerateArg->getValue());
-  llvm::sys::path::append(Path, "default.profraw");
+  llvm::sys::path::append(Path, "default_%m.profraw");
   CmdArgs.push_back(
   Args.MakeArgString(Twine("-fprofile-instrument-path=") + Path));
 }

Modified: cfe/trunk/test/Driver/clang_f_opts.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/clang_f_opts.c?rev=276484&r1=276483&r2=276484&view=diff
==
--- cfe/trunk/test/Driver/clang_f_opts.c (original)
+++ cfe/trunk/test/Driver/clang_f_opts.c Fri Jul 22 17:25:01 2016
@@ -99,7 +99,7 @@
 // RUN: %clang -### -S -fprofil

r276517 - [Profile] Use a flag to enable PGO rather than the profraw filename

[Xinliang David Li via cfe-commits]

Author: davidxl
Date: Fri Jul 22 23:28:59 2016
New Revision: 276517

URL: http://llvm.org/viewvc/llvm-project?rev=276517&view=rev
Log:
[Profile] Use a flag to enable PGO rather than the profraw filename

Patch by Jake VanAdrighem

Differential Revision: http://reviews.llvm.org/D22608



Modified:
cfe/trunk/lib/CodeGen/BackendUtil.cpp

Modified: cfe/trunk/lib/CodeGen/BackendUtil.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/BackendUtil.cpp?rev=276517&r1=276516&r2=276517&view=diff
==
--- cfe/trunk/lib/CodeGen/BackendUtil.cpp (original)
+++ cfe/trunk/lib/CodeGen/BackendUtil.cpp Fri Jul 22 23:28:59 2016
@@ -453,6 +453,7 @@ void EmitAssemblyHelper::CreatePasses(le
 MPM.add(createInstrProfilingLegacyPass(Options));
   }
   if (CodeGenOpts.hasProfileIRInstr()) {
+PMBuilder.EnablePGOInstrGen = true;
 if (!CodeGenOpts.InstrProfileOutput.empty())
   PMBuilder.PGOInstrGen = CodeGenOpts.InstrProfileOutput;
 else


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