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] D84261: [PGO] Supporting code for always instrumenting entry block

[Xinliang David Li via cfe-commits]

I think you are right -- the two files need to be in sync.

On Mon, Aug 17, 2020 at 1:17 PM Pavel Kosov via Phabricator via
llvm-commits  wrote:

> kpdev42 added inline comments.
>
>
> 
> Comment at: llvm/include/llvm/ProfileData/InstrProfData.inc:676
>  #define VARIANT_MASK_CSIR_PROF (0x1ULL << 57)
> +#define VARIANT_MASK_INSTR_ENTRY (0x1ULL << 58)
>  #define INSTR_PROF_RAW_VERSION_VAR __llvm_profile_raw_version
> 
> This revision is closed, so excuse me for the question:
> Files `./llvm/include/llvm/ProfileData/InstrProfData.inc` and
> `./compiler-rt/include/profile/InstrProfData.inc` should be two identical
> copies, as stated in their description.
> ```
>  * The file has two identical copies. The master copy lives in LLVM and
>  * the other one  sits in compiler-rt/lib/profile directory. To make
> changes
>  * in this file, first modify the master copy and copy it over to
> compiler-rt.
>  * Testing of any change in this file can start only after the two copies
> are
>  * synced up.
> ```
>
>
> Should we add `VARIANT_MASK_INSTR_ENTRY` to `compiler-rt` or change
> description?
>
>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D84261/new/
>
> https://reviews.llvm.org/D84261
>
> ___
> llvm-commits mailing list
> llvm-comm...@lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D84782: [PGO] Include the mem ops into the function hash.

[Xinliang David Li via cfe-commits]

right. It occurred to me during review, but did not think of the hard coded
values in proftext depends on LE.

On Wed, Jul 29, 2020 at 3:04 PM Hiroshi Yamauchi via Phabricator <
revi...@reviews.llvm.org> wrote:

> yamauchi added a comment.
>
> Builtbot failure:
> http://lab.llvm.org:8011/builders/clang-ppc64be-linux/builds/51984
>
> I think it's an endian issue in the hash computation.
>
> Will revert.
>
>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D84782/new/
>
> https://reviews.llvm.org/D84782
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D63155: [clang][NewPM] Fix broken profile test

[Xinliang David Li via cfe-commits]

I agree that the  test coverage needs to be improved eg better check etc.

David


On Fri, Jun 28, 2019 at 5:21 PM Chandler Carruth via Phabricator via
llvm-commits  wrote:

> chandlerc added a comment.
>
> In D63155#1563275 , @xur wrote:
>
> > In D63155#1563266 , @chandlerc
> wrote:
> >
> > > I just think we also need to understand why *no other test failed*,
> and why the only test we have for doing PGO at O0 is this one and it could
> be "fixed" but such a deeply unrelated change
> >
> >
> > We have special code to do PGO at O0 in old pass manager. This is not
> done in the new pass manager.
>
>
> I'm not sure how this addresses my question about test coverage.
>
>
> Repository:
>   rL LLVM
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D63155/new/
>
> https://reviews.llvm.org/D63155
>
>
>
> ___
> llvm-commits mailing list
> llvm-comm...@lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D45454: Make __gcov_flush visible outside a shared library

[Xinliang David Li via cfe-commits]

I don't have an objection having another interface which is just a simple
wrapper to __gcov_flush but with default visibility. Also clearly document
its usage and behavior.

David

On Mon, Jun 25, 2018 at 10:12 AM, Chih-Hung Hsieh via Phabricator via
llvm-commits  wrote:

> chh added a comment.
>
> In https://reviews.llvm.org/D45454#1142197, @marco-c wrote:
>
> > In https://reviews.llvm.org/D45454#1070884, @belleyb wrote:
> >
> > > @chh I had a chance to try out your proposed changes. It's not causing
> us any trouble. In fact, `__gcov_flush()` is not even used at all (at least
> in LLVM 5.0.1).. I can recompile llvm, compiler_rt and clang and re-run all
> the tests with `__gcov_flush` commented out! No problem.
> > >
> > > I would suggest adding a bit more documentation to `__gcov_flush()`,
> thus describing what those "special cases" are...
> >
> >
> > __gcov_flush is only used if you actually call it (it's needed for
> example if you want to profile only part of your program).
> >
> > In GCC, __gcov_flush is not hidden, so perhaps we should do the same to
> keep the same behavior? I've also submitted https://reviews.llvm.org/
> D48538, which is making __gcov_flush flush counters for all shared
> libraries (like GCC does, with the same caveat:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83879).
>
>
> I have no problem keeping these functions compatible with GCC.
> My earlier proposal and David's comment in the mailing list seemed to be
> lost and not showing here.
> So, let me summarize the case here. This change should make `__gcov_flush`
> not hidden as before in GCC,
> but earlier change made it hidden as well as other `llvm_gov_*` functions.
> Could we have both `__gov_flush` and `llvm_gov_flush` functions, one
> unhidden and one hidden?
>
>
> https://reviews.llvm.org/D45454
>
>
>
> ___
> 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] D45454: Make __gcov_flush visible outside a shared library

[Xinliang David Li via cfe-commits]

On Wed, Apr 11, 2018 at 11:31 AM, Chih-Hung Hsieh via Phabricator via
llvm-commits  wrote:

> chh added a comment.
>
> Yes, calling `__gcov_flush` within .so files are different,
> but it's a revert of https://reviews.llvm.org/D38124.
> I think  https://bugs.llvm.org/show_bug.cgi?id=27224
> can be fixed by hiding only llvm_gcda_* functions,
> without any change to `__gcov_flush`.
>
>
The coverage dumping behavior for shared libraries (not dlopened) was also
wrong before D38124. D38124 fixed the crash as well as the dumping bug.

David





> (1) Before https://reviews.llvm.org/D38124:
>
> - Calling `__gcov_flush` within .so or main function dumps to main gcda
> file.
> - Android's dlsym() lookup/call of `__gcov_flush` dumps to .so file
> specific gcda files.
>
> (2) After https://reviews.llvm.org/D38124:
>
> - Android's dlsym() cannot find/call `__gcov_flush`.
> - Calling `__gcov_flush` from main works as in (1).
> - Calling `__gcov_flush` from .so works differently; it will dump to .so
> specific gcda file, not the main one.
>
> (3) With this change, we revert `__gcov_flush` behavior back to (1).
>
> Is there any application that needs to call `__gcov_flush` within .so
> and expects the output to .so specific gcda file like in (2)?
> I think the behavior of (1) is more desirable.
> If a main program wants to dump to .so specific gcda file, like Android,
> it can use dlsym() to look up .so specific `__gcov_flush`.
>
>
> https://reviews.llvm.org/D45454
>
>
>
> ___
> 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] D36562: [Bitfield] Make the bitfield a separate location if it has width of legal integer type and its bit offset is naturally aligned for the type

[Xinliang David Li via cfe-commits]

On Mon, Sep 4, 2017 at 1:57 AM, Chandler Carruth 
wrote:

> On Sun, Sep 3, 2017 at 10:41 PM Hal Finkel via llvm-commits <
> llvm-comm...@lists.llvm.org> wrote:
>
>> Nevertheless, I think that you've convinced me that this is a least-bad
>> solution. I'll want a flag preserving the old behavior. Something like
>> -fwiden-bitfield-accesses (modulo bikeshedding).
>>
> Several different approaches have been discussed in this thread, I'm not
> sure what you mean about "least-bad solution"...
>
> I remain completely unconvinced that we should change the default
> behavior. At most, I'm not strongly opposed to adding an attribute that
> indicates "please try to use narrow loads for this bitfield member" and is
> an error if applied to a misaligned or non-byte-sized bitfield member. But
> I remain strongly opposed to changing the default behavior.
>

Having an option and make it off by default for now is a fine solution. We
can also use this option to collect more empirical data on a wider range of
tests and architectures with the community's help.


> We have one or two cases that regress and are easily addressed by source
> changes (either to not use bitfields or to use an attribute). I don't think
> that is cause to change the lowering Clang has used for years and
> potentially regress many other use cases.
>

Note that this is not about that particular cases. It is more about doing
the right thing (to make compiler and HW work together to drive the best
performance for our users).

thanks,

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

Re: [PATCH] D36562: [Bitfield] Make the bitfield a separate location if it has width of legal integer type and its bit offset is naturally aligned for the type

[Xinliang David Li via cfe-commits]

On Mon, Sep 4, 2017 at 9:17 AM, Hal Finkel  wrote:

>
> On 09/04/2017 03:57 AM, Chandler Carruth wrote:
>
> On Sun, Sep 3, 2017 at 10:41 PM Hal Finkel via llvm-commits <
> llvm-comm...@lists.llvm.org> wrote:
>
>> Nevertheless, I think that you've convinced me that this is a least-bad
>> solution. I'll want a flag preserving the old behavior. Something like
>> -fwiden-bitfield-accesses (modulo bikeshedding).
>>
> Several different approaches have been discussed in this thread, I'm not
> sure what you mean about "least-bad solution"...
>
> I remain completely unconvinced that we should change the default
> behavior. At most, I'm not strongly opposed to adding an attribute that
> indicates "please try to use narrow loads for this bitfield member" and is
> an error if applied to a misaligned or non-byte-sized bitfield member.
>
>
> I like this solution too (modulo the fact that I dislike all of these
> solutions). Restricting this only to affecting the loads, and not the
> stores, is also an interesting thought. The attribute could also be on the
> access itself (which, at least from the theoretical perspective, I'd
> prefer).
>
> But I remain strongly opposed to changing the default behavior. We have
> one or two cases that regress and are easily addressed by source changes
> (either to not use bitfields or to use an attribute). I don't think that is
> cause to change the lowering Clang has used for years and potentially
> regress many other use cases.
>
>
> I have mixed feelings about all of the potential fixes here. To walk
> through my thoughts on this:
>
>  1. I don't like any solutions that require changes affecting source level
> semantics. Something that the compiler does automatically is fine, as is an
> attribute.
>
>  2. Next, regarding default behavior, we have a trade off:
>
>A. Breaking apart the accesses, as proposed here, falls into the
> category of "generally, it makes everything a little bit slower." But it's
> worse than that because it comes on a spectrum. I can easily construct
> variants of the provided benchmark which make the separate loads have a bad
> performance penalty. For example:
>
> $ cat ~/tmp/3m.cpp
> class A {
> public:
> #ifdef BF
>   unsigned long f7:8;
>   unsigned long f6:8;
>   unsigned long f5:8;
>   unsigned long f4:8;
>   unsigned long f3:8;
>   unsigned long f2:8;
>   unsigned long f1:8;
>   unsigned long f0:8;
> #else
>   unsigned char f7;
>   unsigned char f6;
>   unsigned char f5;
>   unsigned char f4;
>   unsigned char f3;
>   unsigned char f2;
>   unsigned char f1;
>   unsigned char f0;
> #endif
> };
> A a;
> bool b;
> unsigned long N = 10;
>
> __attribute__((noinline))
> void foo() {
>   a.f4 = 3;
> }
>
> __attribute__((noinline))
> void goo() {
>   b = (a.f0 == 0 && a.f1 == (unsigned char)-1 &&
>a.f2 == 0 && a.f3 == 0 && a.f4 == 0 && a.f5 == 0 &&
>a.f6 == 0 && a.f7 == (unsigned char)-1);
> }
>
> int main() {
>   unsigned long i;
>   foo();
>   for (i = 0; i < N; i++) {
> goo();
>   }
> }
>
> Run this and you'll find that our current scheme, on Haswell, beats
> the separate-loads scheme by nearly 60% (e.g., 2.77s for separate loads vs.
> 1.75s for the current bitfield lowering).
>
> So, how common is it to have a bitfield with a large number of fields
> that could be loaded separately (i.e. have the right size and alignment)
> and have code that loads a large number of them like this (i.e. without
> other work to hide the relative costs)? I have no idea, but regardless,
> there is definitely a high-cost end to this spectrum.
>


Not sure about the answer. The test case itself is a little extreme. There
are many ways to change it slightly to tilt the balance. For instance
1) add one more field with store forwarding issue
2) reduce the number of tested fields in goo by 2 or 3 or more
3) having 2 or 3 more field stores in foo()
4) store non zero value to a.f0 so that short circuiting can happen without
load widening.
or
5) having more stores in foo, and enabling inlining and more aggressive
constant/copy propagation




>
>   B. However, our current scheme can trigger expensive architectural
> hazards. Moreover, there's no local after-the-fact analysis that can fix
> this consistently. I think that Wei has convincingly demonstrated both of
> these things. How common is this? I have no idea. More specifically, how do
> the relative costs of hitting these hazards compare to the costs of the
> increased number of loads under the proposed scheme? I have no idea (and
> this certainly has a workload-dependent answer).
>
>  C. This situation seems unlikely to change in the future: it seems like a
> physics problem. The data surrounding the narrower store is simply not in
> the pipeline to be matched with the wider load. Keeping the data in the
> pipeline would have extra costs, perhaps significant. I'm guessing the
> basic structure of this hazard is here to stay.
>
>  D. In the long run, could this be a 

Re: [PATCH] D36562: [Bitfield] Make the bitfield a separate location if it has width of legal integer type and its bit offset is naturally aligned for the type

[Xinliang David Li via cfe-commits]

On Sun, Sep 3, 2017 at 9:23 PM, Hal Finkel  wrote:

>
> On 09/03/2017 11:06 PM, Xinliang David Li wrote:
>
> I think we can think this in another way.
>
> For modern CPU architectures which supports store forwarding with store
> queues, it is generally not "safe" to blindly do local optimizations to
> widen the load/stores
>
>
> Why not widen stores? Generally the problem with store forwarding is where
> the load is wider than the store (plus alignment issues).
>
>
True, but probably with some caveats which are target dependent.  Store
widening also requires additional bit operations (and possibly addition
load), so the it is tricky to model the the overall benefit.



> without sophisticated inter-procedural analysis. Doing so will run the
> risk of greatly reduce performance of some programs. Keep the naturally
> aligned load/store using its natural type is safer.
>
> Does it make sense?
>
>
> It makes sense. I could, however, say the same thing about inlining. We
> need to make inlining decisions locally, but they have global impact.
> Nevertheless, we need to make inlining decisions, and there's no practical
> way to make them in a truly non-local way.
>

Speaking of inlining, we are actually thinking of ways to make the
decisions more globally optimal, but that is off topic.


>
> We also don't pessimize common cases to improve performance in rare cases.
> In the common case, reducing pressure on the memory units, and reducing the
> critical path, seem likely to me to be optimal. If that's not true, or
> doing otherwise has negligible cost (but can prevent rare downsides), we
> should certainly consider those options.
>

Since we don't do load widening for non-bitfield cases (but the only the
very limited case of naturally aligned bitfields) so it is hard to say we
pessimize common cases for rare cases:

1) the upside doing widening such access is not high from experience with
other compiler (which does not do so)
2) there is obvious downside of introducing additional extract instructions
which degrades performance
3) there is obvious downside of severely degrading performance when store
forwarding is blocked.



> And none of this answers the question of whether it's better to have the
> store wider or the load split and narrow.
>


It seems safer to do store widening more aggressively to avoid store
forwarding stall issue, but doing this aggressively may also mean other
runtime overhead introduced (extra load, data merge etc).

Thanks,

David


>
> Thanks again,
> Hal
>
>
>
> David
>
>
>
> On Sun, Sep 3, 2017 at 8:55 PM, Hal Finkel  wrote:
>
>>
>> On 09/03/2017 10:38 PM, Xinliang David Li wrote:
>>
>> Store forwarding stall cost is usually much higher compared with a load
>> hitting L1 cache. For instance, on Haswell,  the latter is ~4 cycles, while
>> the store forwarding stalls cost about 10 cycles more than a successful
>> store forwarding, which is roughly 15 cycles. In some scenarios, the store
>> forwarding stalls can be as high as 50 cycles. See Agner's documentation.
>>
>>
>> I understand. As I understand it, there are two potential ways to fix
>> this problem:
>>
>>  1. You can make the store wider (to match the size of the wide load,
>> thus permitting forwarding).
>>  2. You can make the load smaller (to match the size of the small store,
>> thus permitting forwarding).
>>
>> At least in this benchmark, which is a better solution?
>>
>> Thanks again,
>> Hal
>>
>>
>>
>> In other words, the optimizer needs to be taught to avoid defeating  the
>> HW pipeline feature as much as possible.
>>
>> David
>>
>> On Sun, Sep 3, 2017 at 6:32 PM, Hal Finkel  wrote:
>>
>>>
>>> On 09/03/2017 03:44 PM, Wei Mi wrote:
>>>
 On Sat, Sep 2, 2017 at 6:04 PM, Hal Finkel  wrote:

> On 08/22/2017 10:56 PM, Wei Mi via llvm-commits wrote:
>
>> On Tue, Aug 22, 2017 at 7:03 PM, Xinliang David Li <
>> davi...@google.com>
>> wrote:
>>
>>>
>>> On Tue, Aug 22, 2017 at 6:37 PM, Chandler Carruth via Phabricator
>>>  wrote:
>>>
 chandlerc added a comment.

 I'm really not a fan of the degree of complexity and subtlety that
 this
 introduces into the frontend, all to allow particular backend
 optimizations.

 I feel like this is Clang working around a fundamental deficiency in
 LLVM
 and we should instead find a way to fix this in LLVM itself.

 As has been pointed out before, user code can synthesize large
 integers
 that small bit sequences are extracted from, and Clang and LLVM
 should
 handle those just as well as actual bitfields.

 Can we see how far we can push the LLVM side before we add
 complexity to
 Clang here? I understand that there remain challenges to LLVM's
 stuff,
 but I
 don't think 

Re: [PATCH] D36562: [Bitfield] Make the bitfield a separate location if it has width of legal integer type and its bit offset is naturally aligned for the type

[Xinliang David Li via cfe-commits]

I think we can think this in another way.

For modern CPU architectures which supports store forwarding with store
queues, it is generally not "safe" to blindly do local optimizations to
widen the load/stores without sophisticated inter-procedural analysis.
Doing so will run the risk of greatly reduce performance of some programs.
Keep the naturally aligned load/store using its natural type is safer.

Does it make sense?

David



On Sun, Sep 3, 2017 at 8:55 PM, Hal Finkel  wrote:

>
> On 09/03/2017 10:38 PM, Xinliang David Li wrote:
>
> Store forwarding stall cost is usually much higher compared with a load
> hitting L1 cache. For instance, on Haswell,  the latter is ~4 cycles, while
> the store forwarding stalls cost about 10 cycles more than a successful
> store forwarding, which is roughly 15 cycles. In some scenarios, the store
> forwarding stalls can be as high as 50 cycles. See Agner's documentation.
>
>
> I understand. As I understand it, there are two potential ways to fix this
> problem:
>
>  1. You can make the store wider (to match the size of the wide load, thus
> permitting forwarding).
>  2. You can make the load smaller (to match the size of the small store,
> thus permitting forwarding).
>
> At least in this benchmark, which is a better solution?
>
> Thanks again,
> Hal
>
>
>
> In other words, the optimizer needs to be taught to avoid defeating  the
> HW pipeline feature as much as possible.
>
> David
>
> On Sun, Sep 3, 2017 at 6:32 PM, Hal Finkel  wrote:
>
>>
>> On 09/03/2017 03:44 PM, Wei Mi wrote:
>>
>>> On Sat, Sep 2, 2017 at 6:04 PM, Hal Finkel  wrote:
>>>
 On 08/22/2017 10:56 PM, Wei Mi via llvm-commits wrote:

> On Tue, Aug 22, 2017 at 7:03 PM, Xinliang David Li  >
> wrote:
>
>>
>> On Tue, Aug 22, 2017 at 6:37 PM, Chandler Carruth via Phabricator
>>  wrote:
>>
>>> chandlerc added a comment.
>>>
>>> I'm really not a fan of the degree of complexity and subtlety that
>>> this
>>> introduces into the frontend, all to allow particular backend
>>> optimizations.
>>>
>>> I feel like this is Clang working around a fundamental deficiency in
>>> LLVM
>>> and we should instead find a way to fix this in LLVM itself.
>>>
>>> As has been pointed out before, user code can synthesize large
>>> integers
>>> that small bit sequences are extracted from, and Clang and LLVM
>>> should
>>> handle those just as well as actual bitfields.
>>>
>>> Can we see how far we can push the LLVM side before we add
>>> complexity to
>>> Clang here? I understand that there remain challenges to LLVM's
>>> stuff,
>>> but I
>>> don't think those challenges make *all* of the LLVM improvements off
>>> the
>>> table, I don't think we've exhausted all ways of improving the LLVM
>>> changes
>>> being proposed, and I think we should still land all of those and
>>> re-evaluate how important these issues are when all of that is in
>>> place.
>>>
>>
>> The main challenge of doing  this in LLVM is that inter-procedural
>> analysis
>> (and possibly cross module) is needed (for store forwarding issues).
>>
>> Wei, perhaps you can provide concrete test case to illustrate the
>> issue
>> so
>> that reviewers have a good understanding.
>>
>> David
>>
> Here is a runable testcase:
>  1.cc 
> class A {
> public:
> unsigned long f1:2;
> unsigned long f2:6;
> unsigned long f3:8;
> unsigned long f4:4;
> };
> A a;
> unsigned long b;
> unsigned long N = 10;
>
> __attribute__((noinline))
> void foo() {
> a.f3 = 3;
> }
>
> __attribute__((noinline))
> void goo() {
> b = a.f3;
> }
>
> int main() {
> unsigned long i;
> for (i = 0; i < N; i++) {
>   foo();
>   goo();
> }
> }
> 
> Now trunk takes about twice running time compared with trunk + this
> patch. That is because trunk shrinks the store of a.f3 in foo (Done by
> DagCombiner) but not shrink the load of a.f3 in goo, so store
> forwarding will be blocked.
>

 I can confirm that this is true on Haswell and also on an POWER8.
 Interestingly, on a POWER7, the performance is the same either way (on
 the
 good side). I ran the test case as presented and where I replaced f3
 with a
 non-bitfield unsigned char member. Thinking that the POWER7 result
 might be
 because it's big-Endian, I flipped the order of the fields, and found
 that
 the version where f3 is not a bitfield is faster than otherwise, but
 only by
 12.5%.

 Why, in this case, don't 

Re: [PATCH] D36562: [Bitfield] Make the bitfield a separate location if it has width of legal integer type and its bit offset is naturally aligned for the type

[Xinliang David Li via cfe-commits]

Store forwarding stall cost is usually much higher compared with a load
hitting L1 cache. For instance, on Haswell,  the latter is ~4 cycles, while
the store forwarding stalls cost about 10 cycles more than a successful
store forwarding, which is roughly 15 cycles. In some scenarios, the store
forwarding stalls can be as high as 50 cycles. See Agner's documentation.

In other words, the optimizer needs to be taught to avoid defeating  the HW
pipeline feature as much as possible.

David

On Sun, Sep 3, 2017 at 6:32 PM, Hal Finkel  wrote:

>
> On 09/03/2017 03:44 PM, Wei Mi wrote:
>
>> On Sat, Sep 2, 2017 at 6:04 PM, Hal Finkel  wrote:
>>
>>> On 08/22/2017 10:56 PM, Wei Mi via llvm-commits wrote:
>>>
 On Tue, Aug 22, 2017 at 7:03 PM, Xinliang David Li 
 wrote:

>
> On Tue, Aug 22, 2017 at 6:37 PM, Chandler Carruth via Phabricator
>  wrote:
>
>> chandlerc added a comment.
>>
>> I'm really not a fan of the degree of complexity and subtlety that
>> this
>> introduces into the frontend, all to allow particular backend
>> optimizations.
>>
>> I feel like this is Clang working around a fundamental deficiency in
>> LLVM
>> and we should instead find a way to fix this in LLVM itself.
>>
>> As has been pointed out before, user code can synthesize large
>> integers
>> that small bit sequences are extracted from, and Clang and LLVM should
>> handle those just as well as actual bitfields.
>>
>> Can we see how far we can push the LLVM side before we add complexity
>> to
>> Clang here? I understand that there remain challenges to LLVM's stuff,
>> but I
>> don't think those challenges make *all* of the LLVM improvements off
>> the
>> table, I don't think we've exhausted all ways of improving the LLVM
>> changes
>> being proposed, and I think we should still land all of those and
>> re-evaluate how important these issues are when all of that is in
>> place.
>>
>
> The main challenge of doing  this in LLVM is that inter-procedural
> analysis
> (and possibly cross module) is needed (for store forwarding issues).
>
> Wei, perhaps you can provide concrete test case to illustrate the issue
> so
> that reviewers have a good understanding.
>
> David
>
 Here is a runable testcase:
  1.cc 
 class A {
 public:
 unsigned long f1:2;
 unsigned long f2:6;
 unsigned long f3:8;
 unsigned long f4:4;
 };
 A a;
 unsigned long b;
 unsigned long N = 10;

 __attribute__((noinline))
 void foo() {
 a.f3 = 3;
 }

 __attribute__((noinline))
 void goo() {
 b = a.f3;
 }

 int main() {
 unsigned long i;
 for (i = 0; i < N; i++) {
   foo();
   goo();
 }
 }
 
 Now trunk takes about twice running time compared with trunk + this
 patch. That is because trunk shrinks the store of a.f3 in foo (Done by
 DagCombiner) but not shrink the load of a.f3 in goo, so store
 forwarding will be blocked.

>>>
>>> I can confirm that this is true on Haswell and also on an POWER8.
>>> Interestingly, on a POWER7, the performance is the same either way (on
>>> the
>>> good side). I ran the test case as presented and where I replaced f3
>>> with a
>>> non-bitfield unsigned char member. Thinking that the POWER7 result might
>>> be
>>> because it's big-Endian, I flipped the order of the fields, and found
>>> that
>>> the version where f3 is not a bitfield is faster than otherwise, but
>>> only by
>>> 12.5%.
>>>
>>> Why, in this case, don't we shrink the load? It seems like we should
>>> (and it
>>> seems like a straightforward case).
>>>
>>> Thanks again,
>>> Hal
>>>
>>> Hal, thanks for trying the test.
>>
>> Yes, it is straightforward to shrink the load in the test. I can
>> change the testcase a little to show why it is sometime difficult to
>> shrink the load:
>>
>> class A {
>> public:
>>unsigned long f1:16;
>>unsigned long f2:16;
>>unsigned long f3:16;
>>unsigned long f4:8;
>> };
>> A a;
>> bool b;
>> unsigned long N = 10;
>>
>> __attribute__((noinline))
>> void foo() {
>>a.f4 = 3;
>> }
>>
>> __attribute__((noinline))
>> void goo() {
>>b = (a.f4 == 0 && a.f3 == (unsigned short)-1);
>> }
>>
>> int main() {
>>unsigned long i;
>>for (i = 0; i < N; i++) {
>>  foo();
>>  goo();
>>}
>> }
>>
>> For the load a.f4 in goo, it is diffcult to motivate its shrink after
>> instcombine because the comparison with a.f3 and the comparison with
>> a.f4 are merged:
>>
>> define void @_Z3goov() local_unnamed_addr #0 {
>>%1 = load i64, i64* bitcast (%class.A* @a to i64*), align 8
>>

Re: [PATCH] D36562: [Bitfield] Make the bitfield a separate location if it has width of legal integer type and its bit offset is naturally aligned for the type

[Xinliang David Li via cfe-commits]

On Tue, Aug 22, 2017 at 7:46 PM, Chandler Carruth <chandl...@gmail.com>
wrote:

>
>
> On Tue, Aug 22, 2017 at 7:18 PM Xinliang David Li via llvm-commits <
> llvm-comm...@lists.llvm.org> wrote:
>
>> On Tue, Aug 22, 2017 at 7:10 PM, Chandler Carruth via llvm-commits <
>> llvm-comm...@lists.llvm.org> wrote:
>>
>>> On Tue, Aug 22, 2017 at 7:03 PM Xinliang David Li via cfe-commits <
>>> cfe-commits@lists.llvm.org> wrote:
>>>
>>>> On Tue, Aug 22, 2017 at 6:37 PM, Chandler Carruth via Phabricator <
>>>> revi...@reviews.llvm.org> wrote:
>>>>
>>>>> chandlerc added a comment.
>>>>>
>>>>> I'm really not a fan of the degree of complexity and subtlety that
>>>>> this introduces into the frontend, all to allow particular backend
>>>>> optimizations.
>>>>>
>>>>> I feel like this is Clang working around a fundamental deficiency in
>>>>> LLVM and we should instead find a way to fix this in LLVM itself.
>>>>>
>>>>> As has been pointed out before, user code can synthesize large
>>>>> integers that small bit sequences are extracted from, and Clang and LLVM
>>>>> should handle those just as well as actual bitfields.
>>>>>
>>>>> Can we see how far we can push the LLVM side before we add complexity
>>>>> to Clang here? I understand that there remain challenges to LLVM's stuff,
>>>>> but I don't think those challenges make *all* of the LLVM improvements off
>>>>> the table, I don't think we've exhausted all ways of improving the LLVM
>>>>> changes being proposed, and I think we should still land all of those and
>>>>> re-evaluate how important these issues are when all of that is in place.
>>>>>
>>>>
>>>> The main challenge of doing  this in LLVM is that inter-procedural
>>>> analysis (and possibly cross module) is needed (for store forwarding
>>>> issues).
>>>>
>>>> Wei, perhaps you can provide concrete test case to illustrate the issue
>>>> so that reviewers have a good understanding.
>>>>
>>>
>>> It doesn't seem like all options for addressing that have been
>>> exhausted. And even then, I feel like trying to fix this with non-obvious
>>> (to the programmer) frontend heuristics isn't a good solution. I actually
>>> *prefer* the source work around of "don't use a bitfield if you *must* have
>>> narrow width access across modules where the optimizer cannot see enough to
>>> narrow them and you happen to know that there is a legal narrow access that
>>> works". Because that way the programmer has *control* over this rather than
>>> being at the whim of whichever side of the heuristic they end up on.
>>>
>>
>>
>> The source workaround solution *does not* scale. Most importantly, user
>> may not even be aware of the problem (and performance loss) unless
>>  compiling the code with another compiler and notice the performance
>> difference.
>>
>
> I don't really understand this argument...
>
> I don't think we can realistically chase all performance problems that
> users aren't aware of or can't measure. And our users *do* care and seem
> very good at benchmarking and noticing problems. =]
>
>
Here you assume that there is single hotspot that user can notice easily,
but this is usually not the case. In a very large scale, we have a long
tail of not so hot functions, but similar problems like this can add up.

Besides if the problem is obvious for user to detect, why wasn't this
problem detected in LLVM earlier?   I admit, in this particular case, we
actually relied on the competitive analysis -- that is why I said depending
on users for this is not reliable.

For yearsI have told our users (optimization) not to hack source code/or do
manual performance tuning (for instance, adding always inlining, adding
inline keyword, adding manually inserted prefetchings, manual loop
unrolling/peelings, etc ). Not only does this method does not scale (it
only helps user's own program), but more importantly it may get stale
overtime -- for instance, due to hardware change or shifting of hot spots.
It is not uncommon to see user's manual optimiztion which used to work
later becomes performance 'bug' without being noticed for years.   I am
totally fine with temporary source workaround, but they should not relied
upon longer term.



> I also think it is OK if in rare cases programmers have to adapt their
> source code to optimize well. We can and should make this as ra

Re: [PATCH] D36562: [Bitfield] Make the bitfield a separate location if it has width of legal integer type and its bit offset is naturally aligned for the type

[Xinliang David Li via cfe-commits]

On Tue, Aug 22, 2017 at 7:10 PM, Chandler Carruth via llvm-commits <
llvm-comm...@lists.llvm.org> wrote:

> On Tue, Aug 22, 2017 at 7:03 PM Xinliang David Li via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> On Tue, Aug 22, 2017 at 6:37 PM, Chandler Carruth via Phabricator <
>> revi...@reviews.llvm.org> wrote:
>>
>>> chandlerc added a comment.
>>>
>>> I'm really not a fan of the degree of complexity and subtlety that this
>>> introduces into the frontend, all to allow particular backend optimizations.
>>>
>>> I feel like this is Clang working around a fundamental deficiency in
>>> LLVM and we should instead find a way to fix this in LLVM itself.
>>>
>>> As has been pointed out before, user code can synthesize large integers
>>> that small bit sequences are extracted from, and Clang and LLVM should
>>> handle those just as well as actual bitfields.
>>>
>>> Can we see how far we can push the LLVM side before we add complexity to
>>> Clang here? I understand that there remain challenges to LLVM's stuff, but
>>> I don't think those challenges make *all* of the LLVM improvements off the
>>> table, I don't think we've exhausted all ways of improving the LLVM changes
>>> being proposed, and I think we should still land all of those and
>>> re-evaluate how important these issues are when all of that is in place.
>>>
>>
>> The main challenge of doing  this in LLVM is that inter-procedural
>> analysis (and possibly cross module) is needed (for store forwarding
>> issues).
>>
>> Wei, perhaps you can provide concrete test case to illustrate the issue
>> so that reviewers have a good understanding.
>>
>
> It doesn't seem like all options for addressing that have been exhausted.
> And even then, I feel like trying to fix this with non-obvious (to the
> programmer) frontend heuristics isn't a good solution. I actually *prefer*
> the source work around of "don't use a bitfield if you *must* have narrow
> width access across modules where the optimizer cannot see enough to narrow
> them and you happen to know that there is a legal narrow access that
> works". Because that way the programmer has *control* over this rather than
> being at the whim of whichever side of the heuristic they end up on.
>


The source workaround solution *does not* scale. Most importantly, user may
not even be aware of the problem (and performance loss) unless  compiling
the code with another compiler and notice the performance difference.

David

>
>
>>
>> David
>>
>>>
>>>
>>> Repository:
>>>   rL LLVM
>>>
>>> https://reviews.llvm.org/D36562
>>>
>>>
>>>
>>> ___
>> cfe-commits mailing list
>> cfe-commits@lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>
> ___
> 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] D36562: [Bitfield] Make the bitfield a separate location if it has width of legal integer type and its bit offset is naturally aligned for the type

[Xinliang David Li via cfe-commits]

On Tue, Aug 22, 2017 at 6:37 PM, Chandler Carruth via Phabricator <
revi...@reviews.llvm.org> wrote:

> chandlerc added a comment.
>
> I'm really not a fan of the degree of complexity and subtlety that this
> introduces into the frontend, all to allow particular backend optimizations.
>
> I feel like this is Clang working around a fundamental deficiency in LLVM
> and we should instead find a way to fix this in LLVM itself.
>
> As has been pointed out before, user code can synthesize large integers
> that small bit sequences are extracted from, and Clang and LLVM should
> handle those just as well as actual bitfields.
>
> Can we see how far we can push the LLVM side before we add complexity to
> Clang here? I understand that there remain challenges to LLVM's stuff, but
> I don't think those challenges make *all* of the LLVM improvements off the
> table, I don't think we've exhausted all ways of improving the LLVM changes
> being proposed, and I think we should still land all of those and
> re-evaluate how important these issues are when all of that is in place.
>

The main challenge of doing  this in LLVM is that inter-procedural analysis
(and possibly cross module) is needed (for store forwarding issues).

Wei, perhaps you can provide concrete test case to illustrate the issue so
that reviewers have a good understanding.

David

>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D36562
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r305325 - Preserve cold attribute for function decls

[Xinliang David Li via cfe-commits]

Author: davidxl
Date: Tue Jun 13 16:14:07 2017
New Revision: 305325

URL: http://llvm.org/viewvc/llvm-project?rev=305325=rev
Log:
Preserve cold attribute for function decls

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



Modified:
cfe/trunk/lib/CodeGen/CGCall.cpp
cfe/trunk/test/CodeGen/attributes.c

Modified: cfe/trunk/lib/CodeGen/CGCall.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGCall.cpp?rev=305325=305324=305325=diff
==
--- cfe/trunk/lib/CodeGen/CGCall.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGCall.cpp Tue Jun 13 16:14:07 2017
@@ -1795,6 +1795,8 @@ void CodeGenModule::ConstructAttributeLi
   FuncAttrs.addAttribute(llvm::Attribute::NoUnwind);
 if (TargetDecl->hasAttr())
   FuncAttrs.addAttribute(llvm::Attribute::NoReturn);
+if (TargetDecl->hasAttr())
+  FuncAttrs.addAttribute(llvm::Attribute::Cold);
 if (TargetDecl->hasAttr())
   FuncAttrs.addAttribute(llvm::Attribute::NoDuplicate);
 if (TargetDecl->hasAttr())

Modified: cfe/trunk/test/CodeGen/attributes.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/attributes.c?rev=305325=305324=305325=diff
==
--- cfe/trunk/test/CodeGen/attributes.c (original)
+++ cfe/trunk/test/CodeGen/attributes.c Tue Jun 13 16:14:07 2017
@@ -56,6 +56,13 @@ void t4() {}
 void t7() __attribute__((noreturn, nothrow));
 void t7() { while (1) {} }
 
+// CHECK: define void @t72() [[COLDDEF:#[0-9]+]] {
+void t71(void) __attribute__((cold));
+void t72() __attribute__((cold));
+void t72() { t71(); }
+// CHECK: call void @t71() [[COLDSITE:#[0-9]+]]
+// CHECK: declare void @t71() [[COLDDECL:#[0-9]+]]
+
 // CHECK: define void @t10() [[NUW]] section "SECT" {
 void t10(void) __attribute__((section("SECT")));
 void t10(void) {}
@@ -92,3 +99,6 @@ void __attribute__((section(".bar"))) t2
 
 // CHECK: attributes [[NUW]] = { noinline nounwind{{.*}} }
 // CHECK: attributes [[NR]] = { noinline noreturn nounwind{{.*}} }
+// CHECK: attributes [[COLDDEF]] = { cold {{.*}}}
+// CHECK: attributes [[COLDDECL]] = { cold {{.*}}}
+// CHECK: attributes [[COLDSITE]] = { cold {{.*}}}


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

r300304 - Fix use after free error

[Xinliang David Li via cfe-commits]

Author: davidxl
Date: Thu Apr 13 23:14:29 2017
New Revision: 300304

URL: http://llvm.org/viewvc/llvm-project?rev=300304=rev
Log:
Fix use after free error

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=300304=300303=300304=diff
==
--- cfe/trunk/lib/CodeGen/CoverageMappingGen.cpp (original)
+++ cfe/trunk/lib/CodeGen/CoverageMappingGen.cpp Thu Apr 13 23:14:29 2017
@@ -961,7 +961,7 @@ struct CounterCoverageMappingBuilder
   }
 };
 
-StringRef getCoverageSection(const CodeGenModule ) {
+std::string getCoverageSection(const CodeGenModule ) {
   return llvm::getInstrProfCoverageSectionName(());
 }
 


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

r300301 - Remove unused function /nfc

[Xinliang David Li via cfe-commits]

Author: davidxl
Date: Thu Apr 13 22:01:25 2017
New Revision: 300301

URL: http://llvm.org/viewvc/llvm-project?rev=300301=rev
Log:
Remove unused function /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=300301=300300=300301=diff
==
--- cfe/trunk/lib/CodeGen/CoverageMappingGen.cpp (original)
+++ cfe/trunk/lib/CodeGen/CoverageMappingGen.cpp Thu Apr 13 22:01:25 2017
@@ -961,10 +961,6 @@ struct CounterCoverageMappingBuilder
   }
 };
 
-bool isMachO(const CodeGenModule ) {
-  return CGM.getTarget().getTriple().isOSBinFormatMachO();
-}
-
 StringRef getCoverageSection(const CodeGenModule ) {
   return llvm::getInstrProfCoverageSectionName(());
 }


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

r300279 - [Profile] PE binary coverage bug fix

[Xinliang David Li via cfe-commits]

Author: davidxl
Date: Thu Apr 13 18:37:21 2017
New Revision: 300279

URL: http://llvm.org/viewvc/llvm-project?rev=300279=rev
Log:
[Profile] PE binary coverage bug fix

PR/32584

Differential Revision: https://reviews.llvm.org/D32023

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=300279=300278=300279=diff
==
--- cfe/trunk/lib/CodeGen/CoverageMappingGen.cpp (original)
+++ cfe/trunk/lib/CodeGen/CoverageMappingGen.cpp Thu Apr 13 18:37:21 2017
@@ -966,7 +966,7 @@ bool isMachO(const CodeGenModule ) {
 }
 
 StringRef getCoverageSection(const CodeGenModule ) {
-  return llvm::getInstrProfCoverageSectionName(isMachO(CGM));
+  return llvm::getInstrProfCoverageSectionName(());
 }
 
 std::string normalizeFilename(StringRef Filename) {


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

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=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=276516=276517=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

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=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=276483=276484=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=276483=276484=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=276483=276484=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=276483=276484=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 -fprofile-instr-generate -fcoverage-mapping 

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=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=276355=276356=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=276355=276356=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

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

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=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=276206=276207=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] 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

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

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

[Xinliang David Li via cfe-commits]

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 pgo (since
it is new option)
2) -fpgo-train=cfg (or some other name) turns on IR pgo
3) -fpgo-train=source turns on FE pgo

4) -fpgo-control=edge:icall to turn on edge profiling and icall profiling.
Edge will be turn on by default always, so it is redundant to specify. To
turn icall off:
-fpgo-control=^icall

5) -fpgo-train-file= to specify path of raw profile

I am not sure if we want -fpgo-apply= as an alias to -fprofile-instr-use=,
but not object to it.

6) I also like the capability to combine 1) and 5) by using tags to
disambiguate path and cfg|source, but that can be done later.

thanks,

David




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

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=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=270727=270728=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 ::getTr
 }
 
 bool CodeGenModule::supportsCOMDAT() const {
-  return !getTriple().isOSBinFormatMachO();
+  return getTriple().supportsCOMDAT();
 }
 
 const TargetCodeGenInfo ::getTargetCodeGenInfo() {


___
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

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

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 

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

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

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=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=261046=261047=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

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=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=261045=261046=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=261045=261046=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

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

[Xinliang David Li via cfe-commits]

On Tue, Feb 9, 2016 at 11:30 AM, David Blaikie  wrote:
>
>
> On Tue, Feb 9, 2016 at 11:26 AM, Xinliang David Li 
> wrote:
>>
>> On Tue, Feb 9, 2016 at 11:14 AM, David Blaikie  wrote:
>> >
>> >
>> > On Mon, Feb 8, 2016 at 9:23 PM, Xinliang David Li 
>> > wrote:
>> >>
>> >> Wrong in the sense the the coverage result for the default operators
>> >> (the line where they are declared) is marked as if they are not called
>> >> which can be confusing to the user.
>> >
>> >
>> > Presumably a user would have the same problem with implicit ops - the
>> > class
>> > header/name would be marked as if there was code that was not called
>> > there?
>>
>> that would be confusing though -- as data of many implicitly declared
>> functions will be lumped together and user won't know what that is .
>
>
> Presumably it's still going to be confusing, though - the line table will
> record that line and the counter won't be there, so the header of the type
> will be marked in red & a user worried about coverage may go through some
> contortions to try to discover and cover that code, no? (even though it may
> already be covered & is just being reported incorrectly due to their being
> no counters)

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

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

thanks,

David

>
>>
>>
>> David
>>
>> >
>> > - David
>> >
>> >>
>> >>
>> >> David
>> >>
>> >> On Mon, Feb 8, 2016 at 9:09 PM, David Blaikie 
>> >> wrote:
>> >> >
>> >> >
>> >> > On Mon, Feb 8, 2016 at 9:00 PM, Xinliang David Li
>> >> > 
>> >> > wrote:
>> >> >>
>> >> >> On Mon, Feb 8, 2016 at 8:46 PM, David Blaikie 
>> >> >> wrote:
>> >> >> >
>> >> >> > On Mon, Feb 8, 2016 at 7:39 PM, Xinliang David Li
>> >> >> > 
>> >> >> > wrote:
>> >> >> >>
>> >> >> >> I took a look at the problem. The implicitly defaulted operators
>> >> >> >> should not be instrumented as specified -- I actually I just
>> >> >> >> added
>> >> >> >> the
>> >> >> >> new test case for that (checking profile counter not generated)
>> >> >> >> right
>> >> >> >> after my previous reply and it still passes with this patch. The
>> >> >> >> reason is that those operators have 'implicit' bit set, and
>> >> >> >> profile
>> >> >> >> instrumentation in FE is implemented in two stages: 1) counter
>> >> >> >> assignment; 2) actual transformation.  For methods with implicit
>> >> >> >> bit
>> >> >> >> set, step 1) is skipped as designed, so step 2) simply becomes a
>> >> >> >> no-op.
>> >> >> >>
>> >> >> >> In short, the test case still needs explicit '=default', and the
>> >> >> >> implicit case is covered elsewhere.
>> >> >> >
>> >> >> >
>> >> >> > OK, thanks for the explanation!
>> >> >> >
>> >> >> > Why is that the case, though - why would an implicitly default
>> >> >> > function
>> >> >> > be
>> >> >> > any different from a profile (& profile-guided-optimization)
>> >> >> > perspective
>> >> >> > from an explicitly defaulted one?
>> >> >>
>> >> >> There are two factors to consider -- PGO and coverage testing.
>> >> >> Implicitly declared functions are usually small/single BB so
>> >> >> instrumenting them does not bring too much benefit (they will be
>> >> >> inlined most of the cases any way) while increasing instrumentation
>> >> >> overhead. They are not needed for coveraging test either (as there
>> >> >> are
>> >> >> no source lines to cover). This is a good tuning heuristic in most
>> >> >> cases, but can get wrong sometimes (IR based late instrumentation is
>> >> >> more focused on performance thus not depending on this tuning).
>> >> >>
>> >> >> Explicitly defaulted ones are different in the sense that if they
>> >> >> are
>> >> >> not instrumented, the coverage result will be wrong.
>> >> >
>> >> >
>> >> > wrong in what way? Both functions (explicitly or implicitly
>> >> > defaulted)
>> >> > will
>> >> > be emitted, with line tables (looks like the = defaulted one is
>> >> > attributed
>> >> > to the line where the = default was written, and the implicitly
>> >> > defaulted
>> >> > one is attributed to wherever the class name is written)
>> >> >
>> >> > - David
>> >> >
>> >> >>
>> >> >>
>> >> >> thanks,
>> >> >>
>> >> >> David
>> >> >>
>> >> >> >
>> >> >> >>
>> >> >> >>
>> >> >> >> thanks,
>> >> >> >>
>> >> >> >> David
>> >> >> >>
>> >> >> >> On Mon, Feb 8, 2016 at 5:23 PM, David Blaikie
>> >> >> >> 
>> >> >> >> wrote:
>> >> >> >> >
>> >> >> >> >
>> >> >> >> > On Mon, Feb 8, 2016 at 5:05 PM, Xinliang David Li
>> >> >> >> > 
>> >> >> >> > wrote:
>> >> >> >> >>
>> >> >> >> >> ha! somehow I kept thinking you are referring to implicit
>> >> >> >> >> declared
>> >> >> >> >> ctors.
>> >> 

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

[Xinliang David Li via cfe-commits]

On Tue, Feb 9, 2016 at 11:14 AM, David Blaikie  wrote:
>
>
> On Mon, Feb 8, 2016 at 9:23 PM, Xinliang David Li 
> wrote:
>>
>> Wrong in the sense the the coverage result for the default operators
>> (the line where they are declared) is marked as if they are not called
>> which can be confusing to the user.
>
>
> Presumably a user would have the same problem with implicit ops - the class
> header/name would be marked as if there was code that was not called there?

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

David

>
> - David
>
>>
>>
>> David
>>
>> On Mon, Feb 8, 2016 at 9:09 PM, David Blaikie  wrote:
>> >
>> >
>> > On Mon, Feb 8, 2016 at 9:00 PM, Xinliang David Li 
>> > wrote:
>> >>
>> >> On Mon, Feb 8, 2016 at 8:46 PM, David Blaikie 
>> >> wrote:
>> >> >
>> >> > On Mon, Feb 8, 2016 at 7:39 PM, Xinliang David Li
>> >> > 
>> >> > wrote:
>> >> >>
>> >> >> I took a look at the problem. The implicitly defaulted operators
>> >> >> should not be instrumented as specified -- I actually I just added
>> >> >> the
>> >> >> new test case for that (checking profile counter not generated)
>> >> >> right
>> >> >> after my previous reply and it still passes with this patch. The
>> >> >> reason is that those operators have 'implicit' bit set, and profile
>> >> >> instrumentation in FE is implemented in two stages: 1) counter
>> >> >> assignment; 2) actual transformation.  For methods with implicit bit
>> >> >> set, step 1) is skipped as designed, so step 2) simply becomes a
>> >> >> no-op.
>> >> >>
>> >> >> In short, the test case still needs explicit '=default', and the
>> >> >> implicit case is covered elsewhere.
>> >> >
>> >> >
>> >> > OK, thanks for the explanation!
>> >> >
>> >> > Why is that the case, though - why would an implicitly default
>> >> > function
>> >> > be
>> >> > any different from a profile (& profile-guided-optimization)
>> >> > perspective
>> >> > from an explicitly defaulted one?
>> >>
>> >> There are two factors to consider -- PGO and coverage testing.
>> >> Implicitly declared functions are usually small/single BB so
>> >> instrumenting them does not bring too much benefit (they will be
>> >> inlined most of the cases any way) while increasing instrumentation
>> >> overhead. They are not needed for coveraging test either (as there are
>> >> no source lines to cover). This is a good tuning heuristic in most
>> >> cases, but can get wrong sometimes (IR based late instrumentation is
>> >> more focused on performance thus not depending on this tuning).
>> >>
>> >> Explicitly defaulted ones are different in the sense that if they are
>> >> not instrumented, the coverage result will be wrong.
>> >
>> >
>> > wrong in what way? Both functions (explicitly or implicitly defaulted)
>> > will
>> > be emitted, with line tables (looks like the = defaulted one is
>> > attributed
>> > to the line where the = default was written, and the implicitly
>> > defaulted
>> > one is attributed to wherever the class name is written)
>> >
>> > - David
>> >
>> >>
>> >>
>> >> thanks,
>> >>
>> >> David
>> >>
>> >> >
>> >> >>
>> >> >>
>> >> >> thanks,
>> >> >>
>> >> >> David
>> >> >>
>> >> >> On Mon, Feb 8, 2016 at 5:23 PM, David Blaikie 
>> >> >> wrote:
>> >> >> >
>> >> >> >
>> >> >> > On Mon, Feb 8, 2016 at 5:05 PM, Xinliang David Li
>> >> >> > 
>> >> >> > wrote:
>> >> >> >>
>> >> >> >> ha! somehow I kept thinking you are referring to implicit
>> >> >> >> declared
>> >> >> >> ctors.
>> >> >> >
>> >> >> >
>> >> >> > Ah, glad we figured out the disconnect - thanks for bearing with
>> >> >> > me!
>> >> >> >
>> >> >> >>
>> >> >> >>
>> >> >> >> From your test case, it is seems that the implicit copy/move op
>> >> >> >> is
>> >> >> >> also broken and is fixed by this patch too. That means  a missing
>> >> >> >> test
>> >> >> >> case to me.  Will update the case when verified.
>> >> >> >
>> >> >> >
>> >> >> > Again, this is a case where I'd probably just simplify the test,
>> >> >> > as I
>> >> >> > asked
>> >> >> > earlier in the thread (I asked if it mattered if the op was
>> >> >> > explicitly
>> >> >> > or
>> >> >> > implicitly defaulted (& your response: "> Is the fix/codepath
>> >> >> > specifically
>> >> >> > about explicitly defaulted ops?
>> >> >> >
>> >> >> > yes -- explicitly defaulted. There are some test coverage already
>> >> >> > for
>> >> >> > implicitly declared ctors (but not assignment op -- probably worth
>> >> >> > adding some testing too).")
>> >> >> >
>> >> >> > So I'd just simplify the test by removing the "= default" - I
>> >> >> > don't
>> >> >> > think
>> >> >> > there's value in testing both the explicit default and implicit
>> >> >> > default
>> >> >> > if
>> >> >> > it's just the default-y-ness that's relevant here. Otherwise we
>> >> >> > could
>> 

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=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=260269=260270=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=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& operator=(const B &);
+};
+
+struct A {
+  A =(const A &) = default;
+  // PGOGEN: define {{.*}}@_ZN1AaSERKS_(
+  // PGOGEN: %pgocount = load {{.*}} @__profc__ZN1AaSERKS_
+  // PGOGEN: {{.*}}add{{.*}}%pgocount, 1
+  // PGOGEN: store{{.*}}@__profc__ZN1AaSERKS_
+  A =(A &&) = default;
+  // PGOGEN: define {{.*}}@_ZN1AaSEOS_
+  // PGOGEN: %pgocount = load {{.*}} @__profc__ZN1AaSEOS_
+  // PGOGEN: {{.*}}add{{.*}}%pgocount, 1
+  // PGOGEN: store{{.*}}@__profc__ZN1AaSEOS_
+
+  // Check that coverage mapping includes 6 function records including the
+  // defaulted copy and move operators: A::operator=
+  // COVMAP: @__llvm_coverage_mapping = {{.*}} { { i32, i32, i32, i32 }, [3 x 
<{{.*}}>],
+  B b;
+};
+
+int main() {
+  A a1, a2;
+  a1 = a2;
+  a2 = static_cast(a1);
+  return 0;
+}


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

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

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

Yes -- this patch only includes clang change.

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

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

David



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

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

[Xinliang David Li via cfe-commits]

Wrong in the sense the the coverage result for the default operators
(the line where they are declared) is marked as if they are not called
which can be confusing to the user.

David

On Mon, Feb 8, 2016 at 9:09 PM, David Blaikie  wrote:
>
>
> On Mon, Feb 8, 2016 at 9:00 PM, Xinliang David Li 
> wrote:
>>
>> On Mon, Feb 8, 2016 at 8:46 PM, David Blaikie  wrote:
>> >
>> > On Mon, Feb 8, 2016 at 7:39 PM, Xinliang David Li 
>> > wrote:
>> >>
>> >> I took a look at the problem. The implicitly defaulted operators
>> >> should not be instrumented as specified -- I actually I just added the
>> >> new test case for that (checking profile counter not generated) right
>> >> after my previous reply and it still passes with this patch. The
>> >> reason is that those operators have 'implicit' bit set, and profile
>> >> instrumentation in FE is implemented in two stages: 1) counter
>> >> assignment; 2) actual transformation.  For methods with implicit bit
>> >> set, step 1) is skipped as designed, so step 2) simply becomes a
>> >> no-op.
>> >>
>> >> In short, the test case still needs explicit '=default', and the
>> >> implicit case is covered elsewhere.
>> >
>> >
>> > OK, thanks for the explanation!
>> >
>> > Why is that the case, though - why would an implicitly default function
>> > be
>> > any different from a profile (& profile-guided-optimization) perspective
>> > from an explicitly defaulted one?
>>
>> There are two factors to consider -- PGO and coverage testing.
>> Implicitly declared functions are usually small/single BB so
>> instrumenting them does not bring too much benefit (they will be
>> inlined most of the cases any way) while increasing instrumentation
>> overhead. They are not needed for coveraging test either (as there are
>> no source lines to cover). This is a good tuning heuristic in most
>> cases, but can get wrong sometimes (IR based late instrumentation is
>> more focused on performance thus not depending on this tuning).
>>
>> Explicitly defaulted ones are different in the sense that if they are
>> not instrumented, the coverage result will be wrong.
>
>
> wrong in what way? Both functions (explicitly or implicitly defaulted) will
> be emitted, with line tables (looks like the = defaulted one is attributed
> to the line where the = default was written, and the implicitly defaulted
> one is attributed to wherever the class name is written)
>
> - David
>
>>
>>
>> thanks,
>>
>> David
>>
>> >
>> >>
>> >>
>> >> thanks,
>> >>
>> >> David
>> >>
>> >> On Mon, Feb 8, 2016 at 5:23 PM, David Blaikie 
>> >> wrote:
>> >> >
>> >> >
>> >> > On Mon, Feb 8, 2016 at 5:05 PM, Xinliang David Li
>> >> > 
>> >> > wrote:
>> >> >>
>> >> >> ha! somehow I kept thinking you are referring to implicit declared
>> >> >> ctors.
>> >> >
>> >> >
>> >> > Ah, glad we figured out the disconnect - thanks for bearing with me!
>> >> >
>> >> >>
>> >> >>
>> >> >> From your test case, it is seems that the implicit copy/move op is
>> >> >> also broken and is fixed by this patch too. That means  a missing
>> >> >> test
>> >> >> case to me.  Will update the case when verified.
>> >> >
>> >> >
>> >> > Again, this is a case where I'd probably just simplify the test, as I
>> >> > asked
>> >> > earlier in the thread (I asked if it mattered if the op was
>> >> > explicitly
>> >> > or
>> >> > implicitly defaulted (& your response: "> Is the fix/codepath
>> >> > specifically
>> >> > about explicitly defaulted ops?
>> >> >
>> >> > yes -- explicitly defaulted. There are some test coverage already for
>> >> > implicitly declared ctors (but not assignment op -- probably worth
>> >> > adding some testing too).")
>> >> >
>> >> > So I'd just simplify the test by removing the "= default" - I don't
>> >> > think
>> >> > there's value in testing both the explicit default and implicit
>> >> > default
>> >> > if
>> >> > it's just the default-y-ness that's relevant here. Otherwise we could
>> >> > end up
>> >> > testing all sorts of ways of writing/interacting with dtors which
>> >> > wouldn't
>> >> > be relevant to the code/fix/etc.
>> >> >
>> >> > This seems like the obvious test for the behavior:
>> >> >
>> >> > struct foo {
>> >> >   // non-trivial ops
>> >> >   foo =(const foo&);
>> >> >   foo =(foo&&);
>> >> > };
>> >> >
>> >> > struct bar {
>> >> >   foo f; // or derive bar from foo, but I think the member version is
>> >> > simpler
>> >> > };
>> >> >
>> >> > // force emission of bar's implicit special members, one way or
>> >> > another:
>> >> > bar &(bar::*x)(const bar&) = ::operator=;
>> >> > bar &(bar::*x)(bar&&) = ::operator=;
>> >> >
>> >> > (or just call them as you had in your test case - but that produces
>> >> > more
>> >> > code, etc in the module, extra functions/profile counters/etc)
>> >> >
>> >> > - Dave
>> >> >
>> >> >>
>> >> >>
>> >> >> thanks,
>> >> >>
>> >> >> David
>> >> >>
>> >> >>
>> >> >> On Mon, Feb 8, 2016 

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

[Xinliang David Li via cfe-commits]

I took a look at the problem. The implicitly defaulted operators
should not be instrumented as specified -- I actually I just added the
new test case for that (checking profile counter not generated) right
after my previous reply and it still passes with this patch. The
reason is that those operators have 'implicit' bit set, and profile
instrumentation in FE is implemented in two stages: 1) counter
assignment; 2) actual transformation.  For methods with implicit bit
set, step 1) is skipped as designed, so step 2) simply becomes a
no-op.

In short, the test case still needs explicit '=default', and the
implicit case is covered elsewhere.

thanks,

David

On Mon, Feb 8, 2016 at 5:23 PM, David Blaikie  wrote:
>
>
> On Mon, Feb 8, 2016 at 5:05 PM, Xinliang David Li 
> wrote:
>>
>> ha! somehow I kept thinking you are referring to implicit declared ctors.
>
>
> Ah, glad we figured out the disconnect - thanks for bearing with me!
>
>>
>>
>> From your test case, it is seems that the implicit copy/move op is
>> also broken and is fixed by this patch too. That means  a missing test
>> case to me.  Will update the case when verified.
>
>
> Again, this is a case where I'd probably just simplify the test, as I asked
> earlier in the thread (I asked if it mattered if the op was explicitly or
> implicitly defaulted (& your response: "> Is the fix/codepath specifically
> about explicitly defaulted ops?
>
> yes -- explicitly defaulted. There are some test coverage already for
> implicitly declared ctors (but not assignment op -- probably worth
> adding some testing too).")
>
> So I'd just simplify the test by removing the "= default" - I don't think
> there's value in testing both the explicit default and implicit default if
> it's just the default-y-ness that's relevant here. Otherwise we could end up
> testing all sorts of ways of writing/interacting with dtors which wouldn't
> be relevant to the code/fix/etc.
>
> This seems like the obvious test for the behavior:
>
> struct foo {
>   // non-trivial ops
>   foo =(const foo&);
>   foo =(foo&&);
> };
>
> struct bar {
>   foo f; // or derive bar from foo, but I think the member version is
> simpler
> };
>
> // force emission of bar's implicit special members, one way or another:
> bar &(bar::*x)(const bar&) = ::operator=;
> bar &(bar::*x)(bar&&) = ::operator=;
>
> (or just call them as you had in your test case - but that produces more
> code, etc in the module, extra functions/profile counters/etc)
>
> - Dave
>
>>
>>
>> thanks,
>>
>> David
>>
>>
>> On Mon, Feb 8, 2016 at 4:58 PM, David Blaikie  wrote:
>> >
>> >
>> > On Mon, Feb 8, 2016 at 4:31 PM, Xinliang David Li 
>> > wrote:
>> >>
>> >> On Mon, Feb 8, 2016 at 4:05 PM, David Blaikie 
>> >> wrote:
>> >> >
>> >> >
>> >> > On Mon, Feb 8, 2016 at 3:58 PM, Xinliang David Li
>> >> > 
>> >> > wrote:
>> >> >>
>> >> >> To be clear, you are suggesting breaking the test into two (one for
>> >> >> copy, and one for move) ? I am totally fine with that.
>> >> >
>> >> >
>> >> > Nah, no need to split the test case - we try to keep the number of
>> >> > test
>> >> > files down (& group related tests into a single file) to reduce test
>> >> > execution time (a non-trivial about of check time is spent in process
>> >> > overhead).
>> >> >
>> >> >>
>> >> >>   I thought you
>> >> >> suggested removing the testing of move/op case because they might
>> >> >> share the same code path (clang's implementation) as the copy/op.
>> >> >
>> >> >
>> >> > I was suggesting that two cases is no big deal whether you test both
>> >> > or
>> >> > test
>> >> > one if they're the same codepath - if there were 5/many more things
>> >> > that
>> >> > shared the same codepath, I'd generally suggest testing a
>> >> > representative
>> >> > sample (possibly just a single one) rather than testing every client
>> >> > of
>> >> > the
>> >> > same code.
>> >> >
>> >> > Feel free to leave the two here as-is. (though if we're talking about
>> >> > test
>> >> > granularity, it's probably worth just putting these cases in the same
>> >> > file/type/etc as the ctor cases you mentioned were already covered)
>> >>
>> >> There is a balance somewhere:
>> >> 1) for small test cases like this, the overhead mainly comes from test
>> >> set up cost -- adding additional incremental test in the same file
>> >> probably almost comes for free (in terms of cost). However
>> >> 2) having too many cases grouped together also reduces the
>> >> debuggability when some test fails.
>> >
>> >
>> > Yep, for sure. In this case, testing the ctors and assignment ops in one
>> > file's probably not a bad tradeoff (you can see how Clang groups its
>> > tests -
>> > a file per language feature in many cases, exploring the myriad ways the
>> > feature can be used - this doesn't always work spectacularly (when you
>> > can't
>> > order the IR emission to happen mostly in the order 

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

[Xinliang David Li via cfe-commits]

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

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

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

thanks,

David

>
>>
>>
>> thanks,
>>
>> David
>>
>> On Mon, Feb 8, 2016 at 5:23 PM, David Blaikie  wrote:
>> >
>> >
>> > On Mon, Feb 8, 2016 at 5:05 PM, Xinliang David Li 
>> > wrote:
>> >>
>> >> ha! somehow I kept thinking you are referring to implicit declared
>> >> ctors.
>> >
>> >
>> > Ah, glad we figured out the disconnect - thanks for bearing with me!
>> >
>> >>
>> >>
>> >> From your test case, it is seems that the implicit copy/move op is
>> >> also broken and is fixed by this patch too. That means  a missing test
>> >> case to me.  Will update the case when verified.
>> >
>> >
>> > Again, this is a case where I'd probably just simplify the test, as I
>> > asked
>> > earlier in the thread (I asked if it mattered if the op was explicitly
>> > or
>> > implicitly defaulted (& your response: "> Is the fix/codepath
>> > specifically
>> > about explicitly defaulted ops?
>> >
>> > yes -- explicitly defaulted. There are some test coverage already for
>> > implicitly declared ctors (but not assignment op -- probably worth
>> > adding some testing too).")
>> >
>> > So I'd just simplify the test by removing the "= default" - I don't
>> > think
>> > there's value in testing both the explicit default and implicit default
>> > if
>> > it's just the default-y-ness that's relevant here. Otherwise we could
>> > end up
>> > testing all sorts of ways of writing/interacting with dtors which
>> > wouldn't
>> > be relevant to the code/fix/etc.
>> >
>> > This seems like the obvious test for the behavior:
>> >
>> > struct foo {
>> >   // non-trivial ops
>> >   foo =(const foo&);
>> >   foo =(foo&&);
>> > };
>> >
>> > struct bar {
>> >   foo f; // or derive bar from foo, but I think the member version is
>> > simpler
>> > };
>> >
>> > // force emission of bar's implicit special members, one way or another:
>> > bar &(bar::*x)(const bar&) = ::operator=;
>> > bar &(bar::*x)(bar&&) = ::operator=;
>> >
>> > (or just call them as you had in your test case - but that produces more
>> > code, etc in the module, extra functions/profile counters/etc)
>> >
>> > - Dave
>> >
>> >>
>> >>
>> >> thanks,
>> >>
>> >> David
>> >>
>> >>
>> >> On Mon, Feb 8, 2016 at 4:58 PM, David Blaikie 
>> >> wrote:
>> >> >
>> >> >
>> >> > On Mon, Feb 8, 2016 at 4:31 PM, Xinliang David Li
>> >> > 
>> >> > wrote:
>> >> >>
>> >> >> On Mon, Feb 8, 2016 at 4:05 PM, David Blaikie 
>> >> >> wrote:
>> >> >> >
>> >> >> >
>> >> >> > On Mon, Feb 8, 2016 at 3:58 PM, Xinliang David Li
>> >> >> > 
>> >> >> > wrote:
>> >> >> >>
>> >> >> >> To be clear, you are suggesting breaking the test into two (one
>> >> >> >> for
>> >> >> >> copy, and one for move) ? I am totally fine with that.
>> >> >> >
>> >> >> >
>> >> >> > Nah, no need to split the test case - we try to keep the number of
>> >> >> > test
>> >> >> > files down (& group related tests into a single file) to reduce
>> >> >> > test
>> >> >> > execution time (a non-trivial about of check time is spent in
>> >> >> > process
>> >> >> > overhead).
>> >> >> >
>> >> >> >>
>> >> >> >>   I thought you
>> >> >> >> suggested removing the testing of move/op case because they might
>> >> >> >> share the same code path 

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

[Xinliang David Li via cfe-commits]

On Mon, Feb 8, 2016 at 11:39 AM, David Blaikie  wrote:
>
>
> On Mon, Feb 8, 2016 at 9:25 AM, David Li via llvm-commits
>  wrote:
>>
>> davidxl updated this revision to Diff 47217.
>> davidxl added a comment.
>>
>> Simplified test case suggested by Vedant.
>>
>>
>> http://reviews.llvm.org/D16947
>>
>> Files:
>>   lib/CodeGen/CGClass.cpp
>>   test/Profile/def-assignop.cpp
>>
>> Index: test/Profile/def-assignop.cpp
>> ===
>> --- test/Profile/def-assignop.cpp
>> +++ test/Profile/def-assignop.cpp
>> @@ -0,0 +1,32 @@
>> +// RUN: %clang_cc1 -x c++ -std=c++11 %s -triple x86_64-unknown-linux-gnu
>> -main-file-name def-assignop.cpp -o - -emit-llvm -fprofile-instrument=clang
>> | FileCheck --check-prefix=PGOGEN %s
>> +// RUN: %clang_cc1 -x c++ -std=c++11 %s -triple x86_64-unknown-linux-gnu
>> -main-file-name def-assignop.cpp -o - -emit-llvm -fprofile-instrument=clang
>> -fcoverage-mapping | FileCheck --check-prefix=COVMAP %s
>> +
>> +struct B {
>> +  void operator=(const B ) {}
>> +  void operator=(const B &) {}
>
>
> Probably best to make these canonical to avoid confusion:
>
> B =(const B&);
> B =(B&&);
>
> (& they don't need definitions - just declarations)

Will change.

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

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

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

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

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

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

Other compiler generated ones are handled differently.

thanks,

David

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

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=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=260125=260126=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 ) {
-if (b2.B == 0) {
-  B = b2.B + 1;
-} else
-  B = b2.B;
-  }
+  Base(const struct Base ) {}
 };
 
 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=260125=260126=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 ) {
>> +if (b2.B == 0) {
>> +  B = b2.B + 1;
>> +} else
>> +  B = b2.B;
>> +return this;
>> +  }
>> +};
>> +
>> +struct A : public B {
>> +  A =(const A &) = default;
>> +// PGOGEN: define {{.*}}@_ZN1AaSERKS_(
>> +// PGOGEN: %pgocount = load {{.*}} @__profc__ZN1AaSERKS_
>> +// PGOGEN: {{.*}}add{{.*}}%pgocount, 1
>> +// PGOGEN: store{{.*}}@__profc__ZN1AaSERKS_
>> +  int I;
>> +  int J;
>> +  int getI() { return I; }
>> +};
>> +
>> +A aa;
>> +int g;
>> +int main() {
>> +  A aa2;
>> +  aa2 = aa;
>> +
>> +  g = aa2.getI();
>> +  return 0;
>> +}
>> Index: lib/CodeGen/CGClass.cpp
>> ===
>> --- lib/CodeGen/CGClass.cpp
>> +++ lib/CodeGen/CGClass.cpp
>> @@ -1608,6 +1608,7 @@
>>
>>LexicalScope Scope(*this, RootCS->getSourceRange());
>>
>> +  incrementProfileCounter(RootCS);
>>AssignmentMemcpyizer AM(*this, AssignOp, Args);
>>for (auto *I : RootCS->body())
>>  AM.emitAssignment(I);
>>
>>
>>
>> ___
>> cfe-commits mailing list
>> cfe-commits@lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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=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=260160=260161=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 =(const BaseBase &);
+ BaseBase =(BaseBase &&);
+};
+
+struct Base : public BaseBase {
   virtual void foo();
 };
 
 struct Derived : public Base {
   Derived();
+  Derived(const Derived &);
+  Derived(Derived &&);
+  Derived =(const Derived &);
+  Derived =(Derived &&);
 };
 
 Derived::Derived() {}
+Derived::Derived(const Derived ) : Base(d) {}
+Derived::Derived(Derived &) : Base(static_cast(d)) {}
+Derived& Derived::operator=(const Derived ) {
+  Base::operator=(d);
+  return *this;
+}
+Derived& Derived::operator=(Derived &) {
+  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 4:05 PM, David Blaikie  wrote:
>
>
> On Mon, Feb 8, 2016 at 3:58 PM, Xinliang David Li 
> wrote:
>>
>> To be clear, you are suggesting breaking the test into two (one for
>> copy, and one for move) ? I am totally fine with that.
>
>
> Nah, no need to split the test case - we try to keep the number of test
> files down (& group related tests into a single file) to reduce test
> execution time (a non-trivial about of check time is spent in process
> overhead).
>
>>
>>   I thought you
>> suggested removing the testing of move/op case because they might
>> share the same code path (clang's implementation) as the copy/op.
>
>
> I was suggesting that two cases is no big deal whether you test both or test
> one if they're the same codepath - if there were 5/many more things that
> shared the same codepath, I'd generally suggest testing a representative
> sample (possibly just a single one) rather than testing every client of the
> same code.
>
> Feel free to leave the two here as-is. (though if we're talking about test
> granularity, it's probably worth just putting these cases in the same
> file/type/etc as the ctor cases you mentioned were already covered)

There is a balance somewhere:
1) for small test cases like this, the overhead mainly comes from test
set up cost -- adding additional incremental test in the same file
probably almost comes for free (in terms of cost). However
2) having too many cases grouped together also reduces the
debuggability when some test fails.

>
> & I'm still curious/wondering if there's a common codepath that would
> provide a simpler fix/code that solved both implicit and explicitly
> defaulted ops.

I may take a look at that when I find time -- but there is no guarantee :)

thanks,

David



>
> - Dave
>
>>
>>
>> thanks,
>>
>> David
>>
>> On Mon, Feb 8, 2016 at 3:52 PM, David Blaikie  wrote:
>> >
>> >
>> > On Mon, Feb 8, 2016 at 3:46 PM, Xinliang David Li 
>> > wrote:
>> >>
>> >> On Mon, Feb 8, 2016 at 3:35 PM, David Blaikie 
>> >> wrote:
>> >> >
>> >> >
>> >> > On Mon, Feb 8, 2016 at 3:21 PM, Xinliang David Li
>> >> > 
>> >> > wrote:
>> >> >>
>> >> >> On Mon, Feb 8, 2016 at 3:17 PM, David Blaikie 
>> >> >> wrote:
>> >> >> >
>> >> >> >
>> >> >> > On Mon, Feb 8, 2016 at 12:07 PM, Xinliang David Li
>> >> >> > 
>> >> >> > wrote:
>> >> >> >>
>> >> >> >> On Mon, Feb 8, 2016 at 11:39 AM, David Blaikie
>> >> >> >> 
>> >> >> >> wrote:
>> >> >> >> >
>> >> >> >> >
>> >> >> >> > On Mon, Feb 8, 2016 at 9:25 AM, David Li via llvm-commits
>> >> >> >> >  wrote:
>> >> >> >> >>
>> >> >> >> >> davidxl updated this revision to Diff 47217.
>> >> >> >> >> davidxl added a comment.
>> >> >> >> >>
>> >> >> >> >> Simplified test case suggested by Vedant.
>> >> >> >> >>
>> >> >> >> >>
>> >> >> >> >> http://reviews.llvm.org/D16947
>> >> >> >> >>
>> >> >> >> >> Files:
>> >> >> >> >>   lib/CodeGen/CGClass.cpp
>> >> >> >> >>   test/Profile/def-assignop.cpp
>> >> >> >> >>
>> >> >> >> >> Index: test/Profile/def-assignop.cpp
>> >> >> >> >>
>> >> >> >> >>
>> >> >> >> >> ===
>> >> >> >> >> --- test/Profile/def-assignop.cpp
>> >> >> >> >> +++ test/Profile/def-assignop.cpp
>> >> >> >> >> @@ -0,0 +1,32 @@
>> >> >> >> >> +// RUN: %clang_cc1 -x c++ -std=c++11 %s -triple
>> >> >> >> >> x86_64-unknown-linux-gnu
>> >> >> >> >> -main-file-name def-assignop.cpp -o - -emit-llvm
>> >> >> >> >> -fprofile-instrument=clang
>> >> >> >> >> | FileCheck --check-prefix=PGOGEN %s
>> >> >> >> >> +// RUN: %clang_cc1 -x c++ -std=c++11 %s -triple
>> >> >> >> >> x86_64-unknown-linux-gnu
>> >> >> >> >> -main-file-name def-assignop.cpp -o - -emit-llvm
>> >> >> >> >> -fprofile-instrument=clang
>> >> >> >> >> -fcoverage-mapping | FileCheck --check-prefix=COVMAP %s
>> >> >> >> >> +
>> >> >> >> >> +struct B {
>> >> >> >> >> +  void operator=(const B ) {}
>> >> >> >> >> +  void operator=(const B &) {}
>> >> >> >> >
>> >> >> >> >
>> >> >> >> > Probably best to make these canonical to avoid confusion:
>> >> >> >> >
>> >> >> >> > B =(const B&);
>> >> >> >> > B =(B&&);
>> >> >> >> >
>> >> >> >> > (& they don't need definitions - just declarations)
>> >> >> >>
>> >> >> >> Will change.
>> >> >> >>
>> >> >> >> >
>> >> >> >> > Also, neither of these are the move /constructor/, just the
>> >> >> >> > move
>> >> >> >> > operator.
>> >> >> >> > Not sure if Vedant just used the wrong terminology, or whether
>> >> >> >> > it's
>> >> >> >> > worth
>> >> >> >> > testing the move/copy ctors too, to check that they do the
>> >> >> >> > right
>> >> >> >> > thing
>> >> >> >> > as
>> >> >> >>
>> >> >> >> I added tests for copy ctors, and plan to add move ctor test
>> >> >> >> soon.
>> >> >> >>
>> >> >> >> > well. (if all of these things use the same codepath, I don't
>> >> 

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

[Xinliang David Li via cfe-commits]

On Mon, Feb 8, 2016 at 3:17 PM, David Blaikie  wrote:
>
>
> On Mon, Feb 8, 2016 at 12:07 PM, Xinliang David Li 
> wrote:
>>
>> On Mon, Feb 8, 2016 at 11:39 AM, David Blaikie  wrote:
>> >
>> >
>> > On Mon, Feb 8, 2016 at 9:25 AM, David Li via llvm-commits
>> >  wrote:
>> >>
>> >> davidxl updated this revision to Diff 47217.
>> >> davidxl added a comment.
>> >>
>> >> Simplified test case suggested by Vedant.
>> >>
>> >>
>> >> http://reviews.llvm.org/D16947
>> >>
>> >> Files:
>> >>   lib/CodeGen/CGClass.cpp
>> >>   test/Profile/def-assignop.cpp
>> >>
>> >> Index: test/Profile/def-assignop.cpp
>> >> ===
>> >> --- test/Profile/def-assignop.cpp
>> >> +++ test/Profile/def-assignop.cpp
>> >> @@ -0,0 +1,32 @@
>> >> +// RUN: %clang_cc1 -x c++ -std=c++11 %s -triple
>> >> x86_64-unknown-linux-gnu
>> >> -main-file-name def-assignop.cpp -o - -emit-llvm
>> >> -fprofile-instrument=clang
>> >> | FileCheck --check-prefix=PGOGEN %s
>> >> +// RUN: %clang_cc1 -x c++ -std=c++11 %s -triple
>> >> x86_64-unknown-linux-gnu
>> >> -main-file-name def-assignop.cpp -o - -emit-llvm
>> >> -fprofile-instrument=clang
>> >> -fcoverage-mapping | FileCheck --check-prefix=COVMAP %s
>> >> +
>> >> +struct B {
>> >> +  void operator=(const B ) {}
>> >> +  void operator=(const B &) {}
>> >
>> >
>> > Probably best to make these canonical to avoid confusion:
>> >
>> > B =(const B&);
>> > B =(B&&);
>> >
>> > (& they don't need definitions - just declarations)
>>
>> Will change.
>>
>> >
>> > Also, neither of these are the move /constructor/, just the move
>> > operator.
>> > Not sure if Vedant just used the wrong terminology, or whether it's
>> > worth
>> > testing the move/copy ctors too, to check that they do the right thing
>> > as
>>
>> I added tests for copy ctors, and plan to add move ctor test soon.
>>
>> > well. (if all of these things use the same codepath, I don't see a great
>> > benefit in having separate tests for them (but you can add them here if
>> > you
>> > like) - I'm just suggesting a manual verification in case those need a
>> > separate fix
>>
>> the ctor and assignment op do not share the same path -- the ctor path
>> is working as expected without the fix -- or do you mean there is no
>> need to cover both copy and move variants?
>
>
> I wouldn't necessarily bother testing multiple instances of the same
> codepath (so the copy and move ctor for example) - but 2 instances is no big
> deal (if there were several more, I might be inclined to just test one as a
> representative sample). I don't mind either way, though. The number is small
> & the test cases are arguably distinct.

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

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

Sorry I am not sure what you mean here.

David

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

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

[Xinliang David Li via cfe-commits]

On Mon, Feb 8, 2016 at 3:35 PM, David Blaikie  wrote:
>
>
> On Mon, Feb 8, 2016 at 3:21 PM, Xinliang David Li 
> wrote:
>>
>> On Mon, Feb 8, 2016 at 3:17 PM, David Blaikie  wrote:
>> >
>> >
>> > On Mon, Feb 8, 2016 at 12:07 PM, Xinliang David Li 
>> > wrote:
>> >>
>> >> On Mon, Feb 8, 2016 at 11:39 AM, David Blaikie 
>> >> wrote:
>> >> >
>> >> >
>> >> > On Mon, Feb 8, 2016 at 9:25 AM, David Li via llvm-commits
>> >> >  wrote:
>> >> >>
>> >> >> davidxl updated this revision to Diff 47217.
>> >> >> davidxl added a comment.
>> >> >>
>> >> >> Simplified test case suggested by Vedant.
>> >> >>
>> >> >>
>> >> >> http://reviews.llvm.org/D16947
>> >> >>
>> >> >> Files:
>> >> >>   lib/CodeGen/CGClass.cpp
>> >> >>   test/Profile/def-assignop.cpp
>> >> >>
>> >> >> Index: test/Profile/def-assignop.cpp
>> >> >> ===
>> >> >> --- test/Profile/def-assignop.cpp
>> >> >> +++ test/Profile/def-assignop.cpp
>> >> >> @@ -0,0 +1,32 @@
>> >> >> +// RUN: %clang_cc1 -x c++ -std=c++11 %s -triple
>> >> >> x86_64-unknown-linux-gnu
>> >> >> -main-file-name def-assignop.cpp -o - -emit-llvm
>> >> >> -fprofile-instrument=clang
>> >> >> | FileCheck --check-prefix=PGOGEN %s
>> >> >> +// RUN: %clang_cc1 -x c++ -std=c++11 %s -triple
>> >> >> x86_64-unknown-linux-gnu
>> >> >> -main-file-name def-assignop.cpp -o - -emit-llvm
>> >> >> -fprofile-instrument=clang
>> >> >> -fcoverage-mapping | FileCheck --check-prefix=COVMAP %s
>> >> >> +
>> >> >> +struct B {
>> >> >> +  void operator=(const B ) {}
>> >> >> +  void operator=(const B &) {}
>> >> >
>> >> >
>> >> > Probably best to make these canonical to avoid confusion:
>> >> >
>> >> > B =(const B&);
>> >> > B =(B&&);
>> >> >
>> >> > (& they don't need definitions - just declarations)
>> >>
>> >> Will change.
>> >>
>> >> >
>> >> > Also, neither of these are the move /constructor/, just the move
>> >> > operator.
>> >> > Not sure if Vedant just used the wrong terminology, or whether it's
>> >> > worth
>> >> > testing the move/copy ctors too, to check that they do the right
>> >> > thing
>> >> > as
>> >>
>> >> I added tests for copy ctors, and plan to add move ctor test soon.
>> >>
>> >> > well. (if all of these things use the same codepath, I don't see a
>> >> > great
>> >> > benefit in having separate tests for them (but you can add them here
>> >> > if
>> >> > you
>> >> > like) - I'm just suggesting a manual verification in case those need
>> >> > a
>> >> > separate fix
>> >>
>> >> the ctor and assignment op do not share the same path -- the ctor path
>> >> is working as expected without the fix -- or do you mean there is no
>> >> need to cover both copy and move variants?
>> >
>> >
>> > I wouldn't necessarily bother testing multiple instances of the same
>> > codepath (so the copy and move ctor for example) - but 2 instances is no
>> > big
>> > deal (if there were several more, I might be inclined to just test one
>> > as a
>> > representative sample). I don't mind either way, though. The number is
>> > small
>> > & the test cases are arguably distinct.
>>
>> Sorry I disagree with your general statement here. I treat such test
>> cases as 'black box testing' that do not know about the internal
>> implementation (code path). It may or may not share the same code path
>> today -- same is true in the future.
>
>
> While there's merit in both approaches, practically speaking it seems
> difficult to test in that way in general - any feature could interact with
> any other.

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

>The LLVM regression suite is far more narrowly targeted than that
> - we don't test combinations of optimizations, for example - we test each
> optimization in isolation. The same would be true of two independent
> features on an interface such as this, I think.

This is a weakness of the test system -- a problem at a different dimension.

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

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

[Xinliang David Li via cfe-commits]

To be clear, you are suggesting breaking the test into two (one for
copy, and one for move) ? I am totally fine with that.  I thought you
suggested removing the testing of move/op case because they might
share the same code path (clang's implementation) as the copy/op.

thanks,

David

On Mon, Feb 8, 2016 at 3:52 PM, David Blaikie  wrote:
>
>
> On Mon, Feb 8, 2016 at 3:46 PM, Xinliang David Li 
> wrote:
>>
>> On Mon, Feb 8, 2016 at 3:35 PM, David Blaikie  wrote:
>> >
>> >
>> > On Mon, Feb 8, 2016 at 3:21 PM, Xinliang David Li 
>> > wrote:
>> >>
>> >> On Mon, Feb 8, 2016 at 3:17 PM, David Blaikie 
>> >> wrote:
>> >> >
>> >> >
>> >> > On Mon, Feb 8, 2016 at 12:07 PM, Xinliang David Li
>> >> > 
>> >> > wrote:
>> >> >>
>> >> >> On Mon, Feb 8, 2016 at 11:39 AM, David Blaikie 
>> >> >> wrote:
>> >> >> >
>> >> >> >
>> >> >> > On Mon, Feb 8, 2016 at 9:25 AM, David Li via llvm-commits
>> >> >> >  wrote:
>> >> >> >>
>> >> >> >> davidxl updated this revision to Diff 47217.
>> >> >> >> davidxl added a comment.
>> >> >> >>
>> >> >> >> Simplified test case suggested by Vedant.
>> >> >> >>
>> >> >> >>
>> >> >> >> http://reviews.llvm.org/D16947
>> >> >> >>
>> >> >> >> Files:
>> >> >> >>   lib/CodeGen/CGClass.cpp
>> >> >> >>   test/Profile/def-assignop.cpp
>> >> >> >>
>> >> >> >> Index: test/Profile/def-assignop.cpp
>> >> >> >>
>> >> >> >> ===
>> >> >> >> --- test/Profile/def-assignop.cpp
>> >> >> >> +++ test/Profile/def-assignop.cpp
>> >> >> >> @@ -0,0 +1,32 @@
>> >> >> >> +// RUN: %clang_cc1 -x c++ -std=c++11 %s -triple
>> >> >> >> x86_64-unknown-linux-gnu
>> >> >> >> -main-file-name def-assignop.cpp -o - -emit-llvm
>> >> >> >> -fprofile-instrument=clang
>> >> >> >> | FileCheck --check-prefix=PGOGEN %s
>> >> >> >> +// RUN: %clang_cc1 -x c++ -std=c++11 %s -triple
>> >> >> >> x86_64-unknown-linux-gnu
>> >> >> >> -main-file-name def-assignop.cpp -o - -emit-llvm
>> >> >> >> -fprofile-instrument=clang
>> >> >> >> -fcoverage-mapping | FileCheck --check-prefix=COVMAP %s
>> >> >> >> +
>> >> >> >> +struct B {
>> >> >> >> +  void operator=(const B ) {}
>> >> >> >> +  void operator=(const B &) {}
>> >> >> >
>> >> >> >
>> >> >> > Probably best to make these canonical to avoid confusion:
>> >> >> >
>> >> >> > B =(const B&);
>> >> >> > B =(B&&);
>> >> >> >
>> >> >> > (& they don't need definitions - just declarations)
>> >> >>
>> >> >> Will change.
>> >> >>
>> >> >> >
>> >> >> > Also, neither of these are the move /constructor/, just the move
>> >> >> > operator.
>> >> >> > Not sure if Vedant just used the wrong terminology, or whether
>> >> >> > it's
>> >> >> > worth
>> >> >> > testing the move/copy ctors too, to check that they do the right
>> >> >> > thing
>> >> >> > as
>> >> >>
>> >> >> I added tests for copy ctors, and plan to add move ctor test soon.
>> >> >>
>> >> >> > well. (if all of these things use the same codepath, I don't see a
>> >> >> > great
>> >> >> > benefit in having separate tests for them (but you can add them
>> >> >> > here
>> >> >> > if
>> >> >> > you
>> >> >> > like) - I'm just suggesting a manual verification in case those
>> >> >> > need
>> >> >> > a
>> >> >> > separate fix
>> >> >>
>> >> >> the ctor and assignment op do not share the same path -- the ctor
>> >> >> path
>> >> >> is working as expected without the fix -- or do you mean there is no
>> >> >> need to cover both copy and move variants?
>> >> >
>> >> >
>> >> > I wouldn't necessarily bother testing multiple instances of the same
>> >> > codepath (so the copy and move ctor for example) - but 2 instances is
>> >> > no
>> >> > big
>> >> > deal (if there were several more, I might be inclined to just test
>> >> > one
>> >> > as a
>> >> > representative sample). I don't mind either way, though. The number
>> >> > is
>> >> > small
>> >> > & the test cases are arguably distinct.
>> >>
>> >> Sorry I disagree with your general statement here. I treat such test
>> >> cases as 'black box testing' that do not know about the internal
>> >> implementation (code path). It may or may not share the same code path
>> >> today -- same is true in the future.
>> >
>> >
>> > While there's merit in both approaches, practically speaking it seems
>> > difficult to test in that way in general - any feature could interact
>> > with
>> > any other.
>>
>> The language features are well specified -- so writing small test
>> cases to cover them is a general accepted way of testing.
>
>
> I'm not sure I follow the distinction you're drawing between the middle end
> optimization tests and the features you're testing here. If the features are
> relatively independent, even within the same API/feature area, they're
> generally tested independently (even two features within a single middle end
> optimization - a test case is written to ensure that, say, 

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

[Xinliang David Li via cfe-commits]

ha! somehow I kept thinking you are referring to implicit declared ctors.

From your test case, it is seems that the implicit copy/move op is
also broken and is fixed by this patch too. That means  a missing test
case to me.  Will update the case when verified.

thanks,

David


On Mon, Feb 8, 2016 at 4:58 PM, David Blaikie  wrote:
>
>
> On Mon, Feb 8, 2016 at 4:31 PM, Xinliang David Li 
> wrote:
>>
>> On Mon, Feb 8, 2016 at 4:05 PM, David Blaikie  wrote:
>> >
>> >
>> > On Mon, Feb 8, 2016 at 3:58 PM, Xinliang David Li 
>> > wrote:
>> >>
>> >> To be clear, you are suggesting breaking the test into two (one for
>> >> copy, and one for move) ? I am totally fine with that.
>> >
>> >
>> > Nah, no need to split the test case - we try to keep the number of test
>> > files down (& group related tests into a single file) to reduce test
>> > execution time (a non-trivial about of check time is spent in process
>> > overhead).
>> >
>> >>
>> >>   I thought you
>> >> suggested removing the testing of move/op case because they might
>> >> share the same code path (clang's implementation) as the copy/op.
>> >
>> >
>> > I was suggesting that two cases is no big deal whether you test both or
>> > test
>> > one if they're the same codepath - if there were 5/many more things that
>> > shared the same codepath, I'd generally suggest testing a representative
>> > sample (possibly just a single one) rather than testing every client of
>> > the
>> > same code.
>> >
>> > Feel free to leave the two here as-is. (though if we're talking about
>> > test
>> > granularity, it's probably worth just putting these cases in the same
>> > file/type/etc as the ctor cases you mentioned were already covered)
>>
>> There is a balance somewhere:
>> 1) for small test cases like this, the overhead mainly comes from test
>> set up cost -- adding additional incremental test in the same file
>> probably almost comes for free (in terms of cost). However
>> 2) having too many cases grouped together also reduces the
>> debuggability when some test fails.
>
>
> Yep, for sure. In this case, testing the ctors and assignment ops in one
> file's probably not a bad tradeoff (you can see how Clang groups its tests -
> a file per language feature in many cases, exploring the myriad ways the
> feature can be used - this doesn't always work spectacularly (when you can't
> order the IR emission to happen mostly in the order that the source is
> written (rather being interleaved))
>
> Anyway, up to you - that part isn't something I'm terribly worried about
> either way.
>
>>
>>
>> >
>> > & I'm still curious/wondering if there's a common codepath that would
>> > provide a simpler fix/code that solved both implicit and explicitly
>> > defaulted ops.
>>
>> I may take a look at that when I find time -- but there is no guarantee :)
>
>
> A quick test of putting "assert(false)" in
> emitImplicitAssignmentOperatorBody and running Clang over this code:
>
> struct foo {
>   foo =(const foo &);
> };
>
> struct bar {
>   foo f;
> };
>
> auto (bar::*x)(const bar&) = ::operator=;
>
> Fires the assertion - this seems to me to indicate that the codepath you
> changed is used for both the explicitly (based on the change fixing your
> test case) and implicitly defaulted (based on my test case) cases.
>
> Is it possible that you end up with duplicate counters by accident in this
> path, then? Or at least that whatever codepath was handling the implicitly
> defaulted ones is now redundant with this one?
>
> Actually, so far as I can tell this doesn't work for implicitly defaulted
> move ops (the above test case doesn't have an add pgocount in it) - perhaps
> I'm missing something/doing it wrong? or was just not communicating clearly
> regarding explicit versus implicitly defaulted special members.
>
> - Dave
>
>>
>>
>> thanks,
>>
>> David
>>
>>
>>
>> >
>> > - Dave
>> >
>> >>
>> >>
>> >> thanks,
>> >>
>> >> David
>> >>
>> >> On Mon, Feb 8, 2016 at 3:52 PM, David Blaikie 
>> >> wrote:
>> >> >
>> >> >
>> >> > On Mon, Feb 8, 2016 at 3:46 PM, Xinliang David Li
>> >> > 
>> >> > wrote:
>> >> >>
>> >> >> On Mon, Feb 8, 2016 at 3:35 PM, David Blaikie 
>> >> >> wrote:
>> >> >> >
>> >> >> >
>> >> >> > On Mon, Feb 8, 2016 at 3:21 PM, Xinliang David Li
>> >> >> > 
>> >> >> > wrote:
>> >> >> >>
>> >> >> >> On Mon, Feb 8, 2016 at 3:17 PM, David Blaikie
>> >> >> >> 
>> >> >> >> wrote:
>> >> >> >> >
>> >> >> >> >
>> >> >> >> > On Mon, Feb 8, 2016 at 12:07 PM, Xinliang David Li
>> >> >> >> > 
>> >> >> >> > wrote:
>> >> >> >> >>
>> >> >> >> >> On Mon, Feb 8, 2016 at 11:39 AM, David Blaikie
>> >> >> >> >> 
>> >> >> >> >> wrote:
>> >> >> >> >> >
>> >> >> >> >> >
>> >> >> >> >> > On Mon, Feb 8, 2016 at 9:25 AM, David Li via llvm-commits
>> >> >> >> >> >  

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=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=260021=260022=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=260021=260022=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

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=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=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 ) {
+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=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

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=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=259954=259955=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=259954=259955=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

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=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=259818=259819=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  = 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

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 

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 

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

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=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=259066=259067=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=259066=259067=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=259066=259067=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=259066=259067=diff
==
--- cfe/trunk/test/Profile/cxx-linkage.cpp (original)
+++ cfe/trunk/test/Profile/cxx-linkage.cpp Thu Jan 28 12:25:53 2016
@@ -1,9 +1,13 @@
 // RUN: %clang_cc1 

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

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

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=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=258260=258261=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

r256714 - [PGO] Cleanup: Use covmap header definition in the template file

[Xinliang David Li via cfe-commits]

Author: davidxl
Date: Sun Jan  3 13:25:54 2016
New Revision: 256714

URL: http://llvm.org/viewvc/llvm-project?rev=256714=rev
Log:
[PGO] Cleanup: Use covmap header definition in the template file

This is one last remaining instrumentatation related structure
that needs to be migrate to use the centralized template
definition.  With this change, instrumentation code 
related to coverage module header will be kept in sync
with the coverage mapping reader. The remaining code
which makes implicit assumption about covmap control
structure layout in the the lowering pass will cleaned
up in a different patch. This patch is not intended to
have no functional change.



Modified:
cfe/trunk/lib/CodeGen/CoverageMappingGen.cpp
cfe/trunk/test/CoverageMapping/ir.c

Modified: cfe/trunk/lib/CodeGen/CoverageMappingGen.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CoverageMappingGen.cpp?rev=256714=256713=256714=diff
==
--- cfe/trunk/lib/CodeGen/CoverageMappingGen.cpp (original)
+++ cfe/trunk/lib/CodeGen/CoverageMappingGen.cpp Sun Jan  3 13:25:54 2016
@@ -993,24 +993,30 @@ void CoverageMappingModuleGen::emit() {
   llvm::ArrayType::get(FunctionRecordTy, FunctionRecords.size());
   auto RecordsVal = llvm::ConstantArray::get(RecordsTy, FunctionRecords);
 
+  llvm::Type *CovDataHeaderTypes[] = {
+#define COVMAP_HEADER(Type, LLVMType, Name, Init) LLVMType,
+#include "llvm/ProfileData/InstrProfData.inc"
+  };
+  auto CovDataHeaderTy =
+  llvm::StructType::get(Ctx, makeArrayRef(CovDataHeaderTypes));
+  llvm::Constant *CovDataHeaderVals[] = {
+#define COVMAP_HEADER(Type, LLVMType, Name, Init) Init,
+#include "llvm/ProfileData/InstrProfData.inc"
+  };
+  auto CovDataHeaderVal = llvm::ConstantStruct::get(
+  CovDataHeaderTy, makeArrayRef(CovDataHeaderVals));
+
   // Create the coverage data record
-  llvm::Type *CovDataTypes[] = {Int32Ty,   Int32Ty,
-Int32Ty,   Int32Ty,
-RecordsTy, FilenamesAndMappingsVal->getType()};
+  llvm::Type *CovDataTypes[] = {CovDataHeaderTy, RecordsTy,
+FilenamesAndMappingsVal->getType()};
   auto CovDataTy = llvm::StructType::get(Ctx, makeArrayRef(CovDataTypes));
-  llvm::Constant *TUDataVals[] = {
-  llvm::ConstantInt::get(Int32Ty, FunctionRecords.size()),
-  llvm::ConstantInt::get(Int32Ty, FilenamesSize),
-  llvm::ConstantInt::get(Int32Ty, CoverageMappingSize),
-  llvm::ConstantInt::get(Int32Ty,
- /*Version=*/CoverageMappingVersion1),
-  RecordsVal, FilenamesAndMappingsVal};
+  llvm::Constant *TUDataVals[] = {CovDataHeaderVal, RecordsVal,
+  FilenamesAndMappingsVal};
   auto CovDataVal =
   llvm::ConstantStruct::get(CovDataTy, makeArrayRef(TUDataVals));
-  auto CovData = new llvm::GlobalVariable(CGM.getModule(), CovDataTy, true,
-  llvm::GlobalValue::InternalLinkage,
-  CovDataVal,
-  llvm::getCoverageMappingVarName());
+  auto CovData = new llvm::GlobalVariable(
+  CGM.getModule(), CovDataTy, true, llvm::GlobalValue::InternalLinkage,
+  CovDataVal, llvm::getCoverageMappingVarName());
 
   CovData->setSection(getCoverageSection(CGM));
   CovData->setAlignment(8);

Modified: cfe/trunk/test/CoverageMapping/ir.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CoverageMapping/ir.c?rev=256714=256713=256714=diff
==
--- cfe/trunk/test/CoverageMapping/ir.c (original)
+++ cfe/trunk/test/CoverageMapping/ir.c Sun Jan  3 13:25:54 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 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 <{ 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]+}} }>]


___
cfe-commits mailing list
cfe-commits@lists.llvm.org

r255587 - [PGO] make profile prefix even shorter and more readable

[Xinliang David Li via cfe-commits]

Author: davidxl
Date: Mon Dec 14 18:33:12 2015
New Revision: 255587

URL: http://llvm.org/viewvc/llvm-project?rev=255587=rev
Log:
[PGO] make profile prefix even shorter and more readable

Modified:
cfe/trunk/test/CoverageMapping/ir.c
cfe/trunk/test/CoverageMapping/unused_names.c
cfe/trunk/test/Profile/c-captured.c
cfe/trunk/test/Profile/c-general.c
cfe/trunk/test/Profile/c-linkage-available_externally.c
cfe/trunk/test/Profile/c-linkage.c
cfe/trunk/test/Profile/c-unreachable-after-switch.c
cfe/trunk/test/Profile/cxx-class.cpp
cfe/trunk/test/Profile/cxx-implicit.cpp
cfe/trunk/test/Profile/cxx-lambda.cpp
cfe/trunk/test/Profile/cxx-linkage.cpp
cfe/trunk/test/Profile/cxx-rangefor.cpp
cfe/trunk/test/Profile/cxx-structors.cpp
cfe/trunk/test/Profile/cxx-templates.cpp
cfe/trunk/test/Profile/cxx-throws.cpp
cfe/trunk/test/Profile/cxx-virtual-destructor-calls.cpp
cfe/trunk/test/Profile/objc-general.m

Modified: cfe/trunk/test/CoverageMapping/ir.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CoverageMapping/ir.c?rev=255587=255586=255587=diff
==
--- cfe/trunk/test/CoverageMapping/ir.c (original)
+++ cfe/trunk/test/CoverageMapping/ir.c Mon Dec 14 18:33:12 2015
@@ -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 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]* @__prf_nm_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]* @__prf_nm_main, i32 0, i32 0), i32 
4, i32 9, i64 {{[0-9]+}} }>]
+// CHECK: @__llvm_coverage_mapping = internal constant { i32, i32, i32, i32, 
[2 x <{ i8*, i32, i32, i64 }>], [{{[0-9]+}} x i8] } { 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]+}} }>]

Modified: cfe/trunk/test/CoverageMapping/unused_names.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CoverageMapping/unused_names.c?rev=255587=255586=255587=diff
==
--- cfe/trunk/test/CoverageMapping/unused_names.c (original)
+++ cfe/trunk/test/CoverageMapping/unused_names.c Mon Dec 14 18:33:12 2015
@@ -4,11 +4,11 @@
 
 // Since foo is never emitted, there should not be a profile name for it.
 
-// CHECK-DAG: @__prf_nm_bar = {{.*}} [3 x i8] c"bar", section 
"{{.*}}__llvm_prf_names"
-// CHECK-DAG: @__prf_nm_baz = {{.*}} [3 x i8] c"baz", section 
"{{.*}}__llvm_prf_names"
-// CHECK-DAG: @__prf_nm_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", 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"
 
-// SYSHEADER-NOT: @__prf_nm_foo =
+// SYSHEADER-NOT: @__profn_foo =
 
 
 #ifdef IS_SYSHEADER

Modified: cfe/trunk/test/Profile/c-captured.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Profile/c-captured.c?rev=255587=255586=255587=diff
==
--- cfe/trunk/test/Profile/c-captured.c (original)
+++ cfe/trunk/test/Profile/c-captured.c Mon Dec 14 18:33:12 2015
@@ -3,9 +3,9 @@
 // RUN: llvm-profdata merge %S/Inputs/c-captured.proftext -o %t.profdata
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.9 -main-file-name 
c-captured.c %s -o - -emit-llvm -fprofile-instr-use=%t.profdata | FileCheck 
-check-prefix=PGOUSE -check-prefix=PGOALL %s
 
-// PGOGEN: @[[DCC:__prf_cn_debug_captured]] = private global [3 x i64] 
zeroinitializer
-// PGOGEN: @[[CSC:__prf_cn_c_captured.c___captured_stmt]] = private global [2 
x i64] zeroinitializer
-// PGOGEN: @[[C1C:__prf_cn_c_captured.c___captured_stmt.1]] = private global 
[3 x i64] zeroinitializer
+// PGOGEN: @[[DCC:__profc_debug_captured]] = private global [3 x i64] 
zeroinitializer
+// PGOGEN: @[[CSC:__profc_c_captured.c___captured_stmt]] = private global [2 x 
i64] zeroinitializer
+// PGOGEN: @[[C1C:__profc_c_captured.c___captured_stmt.1]] = private global [3 
x i64] zeroinitializer
 
 // PGOALL-LABEL: define void @debug_captured()
 // PGOGEN: store {{.*}} @[[DCC]], i64 0, i64 0

Modified: cfe/trunk/test/Profile/c-general.c
URL: 

r255576 - [PGO] Shorten profile symbol prefixes

[Xinliang David Li via cfe-commits]

Author: davidxl
Date: Mon Dec 14 17:26:46 2015
New Revision: 255576

URL: http://llvm.org/viewvc/llvm-project?rev=255576=rev
Log:
[PGO] Shorten profile symbol prefixes

(test case update)
Profile symbols have long prefixes which waste space and creating pressure for 
linker.
This patch shortens the prefixes to minimal length without losing verbosity.

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


Modified:
cfe/trunk/test/CoverageMapping/ir.c
cfe/trunk/test/CoverageMapping/unused_names.c
cfe/trunk/test/Profile/c-captured.c
cfe/trunk/test/Profile/c-general.c
cfe/trunk/test/Profile/c-linkage-available_externally.c
cfe/trunk/test/Profile/c-linkage.c
cfe/trunk/test/Profile/c-unreachable-after-switch.c
cfe/trunk/test/Profile/cxx-class.cpp
cfe/trunk/test/Profile/cxx-implicit.cpp
cfe/trunk/test/Profile/cxx-lambda.cpp
cfe/trunk/test/Profile/cxx-linkage.cpp
cfe/trunk/test/Profile/cxx-rangefor.cpp
cfe/trunk/test/Profile/cxx-structors.cpp
cfe/trunk/test/Profile/cxx-templates.cpp
cfe/trunk/test/Profile/cxx-throws.cpp
cfe/trunk/test/Profile/cxx-virtual-destructor-calls.cpp
cfe/trunk/test/Profile/objc-general.m

Modified: cfe/trunk/test/CoverageMapping/ir.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CoverageMapping/ir.c?rev=255576=255575=255576=diff
==
--- cfe/trunk/test/CoverageMapping/ir.c (original)
+++ cfe/trunk/test/CoverageMapping/ir.c Mon Dec 14 17:26:46 2015
@@ -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 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]* @__llvm_profile_name_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]* @__llvm_profile_name_main, i32 0, 
i32 0), i32 4, i32 9, i64 {{[0-9]+}} }>]
+// CHECK: @__llvm_coverage_mapping = internal constant { i32, i32, i32, i32, 
[2 x <{ i8*, i32, i32, i64 }>], [{{[0-9]+}} x i8] } { 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]* @__prf_nm_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]* @__prf_nm_main, i32 0, i32 0), i32 
4, i32 9, i64 {{[0-9]+}} }>]

Modified: cfe/trunk/test/CoverageMapping/unused_names.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CoverageMapping/unused_names.c?rev=255576=255575=255576=diff
==
--- cfe/trunk/test/CoverageMapping/unused_names.c (original)
+++ cfe/trunk/test/CoverageMapping/unused_names.c Mon Dec 14 17:26:46 2015
@@ -4,11 +4,11 @@
 
 // Since foo is never emitted, there should not be a profile name for it.
 
-// CHECK-DAG: @__llvm_profile_name_bar = {{.*}} [3 x i8] c"bar", section 
"{{.*}}__llvm_prf_names"
-// CHECK-DAG: @__llvm_profile_name_baz = {{.*}} [3 x i8] c"baz", section 
"{{.*}}__llvm_prf_names"
-// CHECK-DAG: @__llvm_profile_name_unused_names.c_qux = {{.*}} [18 x i8] 
c"unused_names.c:qux", section "{{.*}}__llvm_prf_names"
+// CHECK-DAG: @__prf_nm_bar = {{.*}} [3 x i8] c"bar", section 
"{{.*}}__llvm_prf_names"
+// CHECK-DAG: @__prf_nm_baz = {{.*}} [3 x i8] c"baz", section 
"{{.*}}__llvm_prf_names"
+// CHECK-DAG: @__prf_nm_unused_names.c_qux = {{.*}} [18 x i8] 
c"unused_names.c:qux", section "{{.*}}__llvm_prf_names"
 
-// SYSHEADER-NOT: @__llvm_profile_name_foo =
+// SYSHEADER-NOT: @__prf_nm_foo =
 
 
 #ifdef IS_SYSHEADER

Modified: cfe/trunk/test/Profile/c-captured.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Profile/c-captured.c?rev=255576=255575=255576=diff
==
--- cfe/trunk/test/Profile/c-captured.c (original)
+++ cfe/trunk/test/Profile/c-captured.c Mon Dec 14 17:26:46 2015
@@ -3,9 +3,9 @@
 // RUN: llvm-profdata merge %S/Inputs/c-captured.proftext -o %t.profdata
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.9 -main-file-name 
c-captured.c %s -o - -emit-llvm -fprofile-instr-use=%t.profdata | FileCheck 
-check-prefix=PGOUSE -check-prefix=PGOALL %s
 
-// PGOGEN: @[[DCC:__llvm_profile_counters_debug_captured]] = private global [3 
x i64] zeroinitializer
-// PGOGEN: @[[CSC:__llvm_profile_counters_c_captured.c___captured_stmt]] = 
private global [2 x i64] zeroinitializer
-// PGOGEN: @[[C1C:__llvm_profile_counters_c_captured.c___captured_stmt.1]] = 
private global [3 x i64] zeroinitializer
+// PGOGEN: @[[DCC:__prf_cn_debug_captured]] = private global [3 x i64] 
zeroinitializer
+// PGOGEN: @[[CSC:__prf_cn_c_captured.c___captured_stmt]] = private global [2 
x i64] zeroinitializer

Re: [PATCH] D15163: Attach maximum function count to Module when using PGO mode.

[Xinliang David Li via cfe-commits]

On Fri, Dec 11, 2015 at 6:19 PM, Justin Bogner  wrote:
> Easwaran Raman  writes:
>> eraman updated this revision to Diff 42549.
>> eraman added a comment.
>>
>> Added a test case.
>>
>>
>> Repository:
>>   rL LLVM
>>
>> http://reviews.llvm.org/D15163
>>
>> Files:
>>   lib/CodeGen/CodeGenModule.cpp
>>   test/CodeGen/pgo-max-function-count.c
>>
>> Index: test/CodeGen/pgo-max-function-count.c
>> ===
>> --- /dev/null
>> +++ test/CodeGen/pgo-max-function-count.c
>> @@ -0,0 +1,9 @@
>> +// RUN: %clang -fprofile-generate -o %t -O2 %s
>> +// RUN: env LLVM_PROFILE_FILE=%t.profraw  %t
>
> The clang tests are run in environments where the generated binaries
> can't run on the host system, so you can't do this. Instead of this kind
> of full integration test you should provide handwritten profile data to
> feed into the test. See the tests in test/Profile for examples, and also
> consider putting this test in that directory with the others.
>

This is a good suggestion -- I previously suggested adding a test in
compiler-rt and get rid of this one, but looks like we can do what
Justin suggested. You can generate profile data in text format like
this:

llvm-profdata merge -o pgo-max-function-count.proftxt -text


After adding pgo-max-function-count.proftxt into
tools/clang/test/Profile/Inputs, the test can be modified to do
profile-use compile only.

David



>> +// RUN: llvm-profdata merge -o %t.profdata %t.profraw
>> +// RUN: %clang -fprofile-use=%t.profdata -o - -S -emit-llvm %s | FileCheck 
>> %s
>> +// Check
>> +int main() {
>> +  return 0;
>> +}
>> +// CHECK: !{{[0-9]+}} = !{i32 1, !"MaxFunctionCount", i32 1}
>> Index: lib/CodeGen/CodeGenModule.cpp
>> ===
>> --- lib/CodeGen/CodeGenModule.cpp
>> +++ lib/CodeGen/CodeGenModule.cpp
>> @@ -376,6 +376,9 @@
>>}
>>if (PGOReader && PGOStats.hasDiagnostics())
>>  PGOStats.reportDiagnostics(getDiags(), getCodeGenOpts().MainFileName);
>> +  // In PGO mode, attach maximum function count to the module.
>
> This comment isn't helpful - it's just stating exactly what the code
> that follows does.
>
>> +  if (PGOReader)
>> +
>> getModule().setMaximumFunctionCount(PGOReader->getMaximumFunctionCount());
>
> It would read better to fold the two `if (PGOReader)` checks
> together. ie:
>
>   if (PGOReader) {
> getModule().setMaximumFunctionCount(PGOReader->getMaximumFunctionCount());
> if (PGOStats.hasDiagnostics())
>   PGOStats.reportDiagnostics(getDiags(), getCodeGenOpts().MainFileName);
>   }
>
> That said, wouldn't it make more sense to set this within PGOReader
> itself? It feels pretty awkward for this to be happening in
> CodeGenModule::Release().
>
>>EmitCtorList(GlobalCtors, "llvm.global_ctors");
>>EmitCtorList(GlobalDtors, "llvm.global_dtors");
>>EmitGlobalAnnotations();
>>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D15163: Attach maximum function count to Module when using PGO mode.

[Xinliang David Li via cfe-commits]

On Fri, Dec 11, 2015 at 6:19 PM, Justin Bogner  wrote:
> Easwaran Raman  writes:
>> eraman updated this revision to Diff 42549.
>> eraman added a comment.
>>
>> Added a test case.
>>
>>
>> Repository:
>>   rL LLVM
>>
>> http://reviews.llvm.org/D15163
>>
>> Files:
>>   lib/CodeGen/CodeGenModule.cpp
>>   test/CodeGen/pgo-max-function-count.c
>>
>> Index: test/CodeGen/pgo-max-function-count.c
>> ===
>> --- /dev/null
>> +++ test/CodeGen/pgo-max-function-count.c
>> @@ -0,0 +1,9 @@
>> +// RUN: %clang -fprofile-generate -o %t -O2 %s
>> +// RUN: env LLVM_PROFILE_FILE=%t.profraw  %t
>
> The clang tests are run in environments where the generated binaries
> can't run on the host system, so you can't do this. Instead of this kind
> of full integration test you should provide handwritten profile data to
> feed into the test. See the tests in test/Profile for examples, and also
> consider putting this test in that directory with the others.
>
>> +// RUN: llvm-profdata merge -o %t.profdata %t.profraw
>> +// RUN: %clang -fprofile-use=%t.profdata -o - -S -emit-llvm %s | FileCheck 
>> %s
>> +// Check
>> +int main() {
>> +  return 0;
>> +}
>> +// CHECK: !{{[0-9]+}} = !{i32 1, !"MaxFunctionCount", i32 1}
>> Index: lib/CodeGen/CodeGenModule.cpp
>> ===
>> --- lib/CodeGen/CodeGenModule.cpp
>> +++ lib/CodeGen/CodeGenModule.cpp
>> @@ -376,6 +376,9 @@
>>}
>>if (PGOReader && PGOStats.hasDiagnostics())
>>  PGOStats.reportDiagnostics(getDiags(), getCodeGenOpts().MainFileName);
>> +  // In PGO mode, attach maximum function count to the module.
>
> This comment isn't helpful - it's just stating exactly what the code
> that follows does.
>
>> +  if (PGOReader)
>> +
>> getModule().setMaximumFunctionCount(PGOReader->getMaximumFunctionCount());
>
> It would read better to fold the two `if (PGOReader)` checks
> together. ie:
>
>   if (PGOReader) {
> getModule().setMaximumFunctionCount(PGOReader->getMaximumFunctionCount());
> if (PGOStats.hasDiagnostics())
>   PGOStats.reportDiagnostics(getDiags(), getCodeGenOpts().MainFileName);
>   }
>
> That said, wouldn't it make more sense to set this within PGOReader
> itself? It feels pretty awkward for this to be happening in
> CodeGenModule::Release().

The reader does not know about module context. I think it is better to
put this in CodeGenModule constructor when PGOReader is created.

David


>
>>EmitCtorList(GlobalCtors, "llvm.global_ctors");
>>EmitCtorList(GlobalDtors, "llvm.global_dtors");
>>EmitGlobalAnnotations();
>>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r255447 - Revert r255445: adding a new test case

[Xinliang David Li via cfe-commits]

Author: davidxl
Date: Sat Dec 12 22:45:49 2015
New Revision: 255447

URL: http://llvm.org/viewvc/llvm-project?rev=255447=rev
Log:
Revert r255445: adding a new test case

Removed:
cfe/trunk/test/Profile/cxx-static.cpp

Removed: cfe/trunk/test/Profile/cxx-static.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Profile/cxx-static.cpp?rev=255446=auto
==
--- cfe/trunk/test/Profile/cxx-static.cpp (original)
+++ cfe/trunk/test/Profile/cxx-static.cpp (removed)
@@ -1,11 +0,0 @@
-// REQUIRES: x86-registered-target
-// RUN: %clang -target i386-unknown-linux -std=c++11 -o %t.o -c 
-no-integrated-as -fprofile-instr-generate %s
-
-__attribute__((noinline)) static int bar() { return 1; }
-
-int foo(int a, int b) {
-  auto Func = [](int a, int b) { return a > b; };
-
-  return Func(a, b) + bar();
-}
-


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

r255435 - [PGO] Stop using invalid char in instr variable names.

[Xinliang David Li via cfe-commits]

Author: davidxl
Date: Sat Dec 12 11:28:37 2015
New Revision: 255435

URL: http://llvm.org/viewvc/llvm-project?rev=255435=rev
Log:
[PGO] Stop using invalid char in instr variable names.

(This is part-2 of the patch of r255434 -- 
fixing test cases, second try)


Modified:
cfe/trunk/test/CoverageMapping/unused_names.c
cfe/trunk/test/Profile/c-captured.c
cfe/trunk/test/Profile/c-general.c
cfe/trunk/test/Profile/c-linkage.c
cfe/trunk/test/Profile/cxx-lambda.cpp
cfe/trunk/test/Profile/objc-general.m

Modified: cfe/trunk/test/CoverageMapping/unused_names.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CoverageMapping/unused_names.c?rev=255435=255434=255435=diff
==
--- cfe/trunk/test/CoverageMapping/unused_names.c (original)
+++ cfe/trunk/test/CoverageMapping/unused_names.c Sat Dec 12 11:28:37 2015
@@ -6,7 +6,7 @@
 
 // CHECK-DAG: @__llvm_profile_name_bar = {{.*}} [3 x i8] c"bar", section 
"{{.*}}__llvm_prf_names"
 // CHECK-DAG: @__llvm_profile_name_baz = {{.*}} [3 x i8] c"baz", section 
"{{.*}}__llvm_prf_names"
-// CHECK-DAG: @"__llvm_profile_name_unused_names.c:qux" = {{.*}} [18 x i8] 
c"unused_names.c:qux", section "{{.*}}__llvm_prf_names"
+// CHECK-DAG: @__llvm_profile_name_unused_names.c_qux = {{.*}} [18 x i8] 
c"unused_names.c:qux", section "{{.*}}__llvm_prf_names"
 
 // SYSHEADER-NOT: @__llvm_profile_name_foo =
 

Modified: cfe/trunk/test/Profile/c-captured.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Profile/c-captured.c?rev=255435=255434=255435=diff
==
--- cfe/trunk/test/Profile/c-captured.c (original)
+++ cfe/trunk/test/Profile/c-captured.c Sat Dec 12 11:28:37 2015
@@ -4,8 +4,8 @@
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.9 -main-file-name 
c-captured.c %s -o - -emit-llvm -fprofile-instr-use=%t.profdata | FileCheck 
-check-prefix=PGOUSE -check-prefix=PGOALL %s
 
 // PGOGEN: @[[DCC:__llvm_profile_counters_debug_captured]] = private global [3 
x i64] zeroinitializer
-// PGOGEN: @[[CSC:"__llvm_profile_counters_c-captured.c:__captured_stmt"]] = 
private global [2 x i64] zeroinitializer
-// PGOGEN: @[[C1C:"__llvm_profile_counters_c-captured.c:__captured_stmt.1"]] = 
private global [3 x i64] zeroinitializer
+// PGOGEN: @[[CSC:__llvm_profile_counters_c_captured.c___captured_stmt]] = 
private global [2 x i64] zeroinitializer
+// PGOGEN: @[[C1C:__llvm_profile_counters_c_captured.c___captured_stmt.1]] = 
private global [3 x i64] zeroinitializer
 
 // PGOALL-LABEL: define void @debug_captured()
 // PGOGEN: store {{.*}} @[[DCC]], i64 0, i64 0

Modified: cfe/trunk/test/Profile/c-general.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Profile/c-general.c?rev=255435=255434=255435=diff
==
--- cfe/trunk/test/Profile/c-general.c (original)
+++ cfe/trunk/test/Profile/c-general.c Sat Dec 12 11:28:37 2015
@@ -19,7 +19,7 @@
 // PGOGEN: @[[COC:__llvm_profile_counters_conditional_operator]] = private 
global [3 x i64] zeroinitializer
 // PGOGEN: @[[DFC:__llvm_profile_counters_do_fallthrough]] = private global [4 
x i64] zeroinitializer
 // PGOGEN: @[[MAC:__llvm_profile_counters_main]] = private global [1 x i64] 
zeroinitializer
-// PGOGEN: @[[STC:"__llvm_profile_counters_c-general.c:static_func"]] = 
private global [2 x i64] zeroinitializer
+// PGOGEN: @[[STC:__llvm_profile_counters_c_general.c_static_func]] = private 
global [2 x i64] zeroinitializer
 
 // PGOGEN-LABEL: @simple_loops()
 // PGOUSE-LABEL: @simple_loops()

Modified: cfe/trunk/test/Profile/c-linkage.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Profile/c-linkage.c?rev=255435=255434=255435=diff
==
--- cfe/trunk/test/Profile/c-linkage.c (original)
+++ cfe/trunk/test/Profile/c-linkage.c Sat Dec 12 11:28:37 2015
@@ -4,7 +4,7 @@
 // CHECK: @__llvm_profile_name_foo = private constant [3 x i8] c"foo"
 // CHECK: @__llvm_profile_name_foo_weak = weak hidden constant [8 x i8] 
c"foo_weak"
 // CHECK: @__llvm_profile_name_main = private constant [4 x i8] c"main"
-// CHECK: @"__llvm_profile_name_c-linkage.c:foo_internal" = private constant 
[24 x i8] c"c-linkage.c:foo_internal"
+// CHECK: @__llvm_profile_name_c_linkage.c_foo_internal = private constant [24 
x i8] c"c-linkage.c:foo_internal"
 
 void foo(void) { }
 

Modified: cfe/trunk/test/Profile/cxx-lambda.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Profile/cxx-lambda.cpp?rev=255435=255434=255435=diff
==
--- cfe/trunk/test/Profile/cxx-lambda.cpp (original)
+++ cfe/trunk/test/Profile/cxx-lambda.cpp Sat Dec 12 11:28:37 2015
@@ -11,7 +11,7 @@
 
 // PGOGEN: @[[LWC:__llvm_profile_counters__Z7lambdasv]] = private global [4 x 
i64] zeroinitializer
 // PGOGEN: 

r255436 - [PGO] add a test case with -no-integrated-as

[Xinliang David Li via cfe-commits]

Author: davidxl
Date: Sat Dec 12 11:39:38 2015
New Revision: 255436

URL: http://llvm.org/viewvc/llvm-project?rev=255436=rev
Log:
[PGO] add a test case with -no-integrated-as

Added:
cfe/trunk/test/Profile/cxx-static.cpp

Added: cfe/trunk/test/Profile/cxx-static.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Profile/cxx-static.cpp?rev=255436=auto
==
--- cfe/trunk/test/Profile/cxx-static.cpp (added)
+++ cfe/trunk/test/Profile/cxx-static.cpp Sat Dec 12 11:39:38 2015
@@ -0,0 +1,13 @@
+// RUN: %clang -std=c++11 -o %t.o -c -no-integrated-as 
-fprofile-instr-generate %s
+
+__attribute__((noinline)) static int bar() {
+return 1;
+}
+
+int foo(int a, int b)
+{
+auto Func = [](int a, int b) { return a > b; };
+
+return Func(a,b) + bar();
+}
+


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

r255437 - Revert 255436 : remove test that needs to be refined

[Xinliang David Li via cfe-commits]

Author: davidxl
Date: Sat Dec 12 12:49:37 2015
New Revision: 255437

URL: http://llvm.org/viewvc/llvm-project?rev=255437=rev
Log:
Revert 255436 : remove test that needs to be refined

Removed:
cfe/trunk/test/Profile/cxx-static.cpp

Removed: cfe/trunk/test/Profile/cxx-static.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Profile/cxx-static.cpp?rev=255436=auto
==
--- cfe/trunk/test/Profile/cxx-static.cpp (original)
+++ cfe/trunk/test/Profile/cxx-static.cpp (removed)
@@ -1,13 +0,0 @@
-// RUN: %clang -std=c++11 -o %t.o -c -no-integrated-as 
-fprofile-instr-generate %s
-
-__attribute__((noinline)) static int bar() {
-return 1;
-}
-
-int foo(int a, int b)
-{
-auto Func = [](int a, int b) { return a > b; };
-
-return Func(a,b) + bar();
-}
-


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

r255445 - Resubmit new test case after adding more constraint

[Xinliang David Li via cfe-commits]

Author: davidxl
Date: Sat Dec 12 21:03:35 2015
New Revision: 255445

URL: http://llvm.org/viewvc/llvm-project?rev=255445=rev
Log:
Resubmit new test case after adding more constraint

Added:
cfe/trunk/test/Profile/cxx-static.cpp

Added: cfe/trunk/test/Profile/cxx-static.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Profile/cxx-static.cpp?rev=255445=auto
==
--- cfe/trunk/test/Profile/cxx-static.cpp (added)
+++ cfe/trunk/test/Profile/cxx-static.cpp Sat Dec 12 21:03:35 2015
@@ -0,0 +1,11 @@
+// REQUIRES: x86-registered-target
+// RUN: %clang -target i386-unknown-linux -std=c++11 -o %t.o -c 
-no-integrated-as -fprofile-instr-generate %s
+
+__attribute__((noinline)) static int bar() { return 1; }
+
+int foo(int a, int b) {
+  auto Func = [](int a, int b) { return a > b; };
+
+  return Func(a, b) + bar();
+}
+


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

r255366 - [PGO] Stop using invalid char in instr variable names.

[Xinliang David Li via cfe-commits]

Author: davidxl
Date: Fri Dec 11 13:53:35 2015
New Revision: 255366

URL: http://llvm.org/viewvc/llvm-project?rev=255366=rev
Log:
[PGO] Stop using invalid char in instr variable names.

(This is part-2 of the patch -- fixing test cases)

Before the patch, -fprofile-instr-generate compile will fail
if no integrated-as is specified when the file contains
any static functions (the -S output is also invalid).

This patch fixed the issue. With the change, the index format
version will be bumped up by 1. Backward compatibility is 
preserved with this change.

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



Modified:
cfe/trunk/test/CoverageMapping/unused_names.c
cfe/trunk/test/Profile/Inputs/c-captured.proftext
cfe/trunk/test/Profile/Inputs/c-general.proftext
cfe/trunk/test/Profile/Inputs/cxx-lambda.proftext
cfe/trunk/test/Profile/Inputs/objc-general.proftext
cfe/trunk/test/Profile/c-captured.c
cfe/trunk/test/Profile/c-general.c
cfe/trunk/test/Profile/c-linkage.c
cfe/trunk/test/Profile/cxx-lambda.cpp
cfe/trunk/test/Profile/objc-general.m

Modified: cfe/trunk/test/CoverageMapping/unused_names.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CoverageMapping/unused_names.c?rev=255366=255365=255366=diff
==
--- cfe/trunk/test/CoverageMapping/unused_names.c (original)
+++ cfe/trunk/test/CoverageMapping/unused_names.c Fri Dec 11 13:53:35 2015
@@ -6,7 +6,7 @@
 
 // CHECK-DAG: @__llvm_profile_name_bar = {{.*}} [3 x i8] c"bar", section 
"{{.*}}__llvm_prf_names"
 // CHECK-DAG: @__llvm_profile_name_baz = {{.*}} [3 x i8] c"baz", section 
"{{.*}}__llvm_prf_names"
-// CHECK-DAG: @"__llvm_profile_name_unused_names.c:qux" = {{.*}} [18 x i8] 
c"unused_names.c:qux", section "{{.*}}__llvm_prf_names"
+// CHECK-DAG: @__llvm_profile_name_unused_names.c__qux = {{.*}} [19 x i8] 
c"unused_names.c__qux", section "{{.*}}__llvm_prf_names"
 
 // SYSHEADER-NOT: @__llvm_profile_name_foo =
 

Modified: cfe/trunk/test/Profile/Inputs/c-captured.proftext
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Profile/Inputs/c-captured.proftext?rev=255366=255365=255366=diff
==
--- cfe/trunk/test/Profile/Inputs/c-captured.proftext (original)
+++ cfe/trunk/test/Profile/Inputs/c-captured.proftext Fri Dec 11 13:53:35 2015
@@ -1,10 +1,10 @@
-c-captured.c:__captured_stmt
+c-captured.ccaptured_stmt
 10
 2
 1
 1
 
-c-captured.c:__captured_stmt.1
+c-captured.ccaptured_stmt.1
 266
 3
 1

Modified: cfe/trunk/test/Profile/Inputs/c-general.proftext
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Profile/Inputs/c-general.proftext?rev=255366=255365=255366=diff
==
--- cfe/trunk/test/Profile/Inputs/c-general.proftext (original)
+++ cfe/trunk/test/Profile/Inputs/c-general.proftext Fri Dec 11 13:53:35 2015
@@ -149,7 +149,7 @@ main
 1
 1
 
-c-general.c:static_func
+c-general.c__static_func
 4
 2
 1

Modified: cfe/trunk/test/Profile/Inputs/cxx-lambda.proftext
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Profile/Inputs/cxx-lambda.proftext?rev=255366=255365=255366=diff
==
--- cfe/trunk/test/Profile/Inputs/cxx-lambda.proftext (original)
+++ cfe/trunk/test/Profile/Inputs/cxx-lambda.proftext Fri Dec 11 13:53:35 2015
@@ -1,4 +1,4 @@
-cxx-lambda.cpp:_ZZ7lambdasvENK3$_0clEi
+cxx-lambda.cpp___ZZ7lambdasvENK3$_0clEi
 654
 3
 10

Modified: cfe/trunk/test/Profile/Inputs/objc-general.proftext
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Profile/Inputs/objc-general.proftext?rev=255366=255365=255366=diff
==
--- cfe/trunk/test/Profile/Inputs/objc-general.proftext (original)
+++ cfe/trunk/test/Profile/Inputs/objc-general.proftext Fri Dec 11 13:53:35 2015
@@ -1,10 +1,10 @@
-objc-general.m:__13+[A foreach:]_block_invoke
+objc-general.m13+[A foreach:]_block_invoke
 10
 2
 2
 1
 
-objc-general.m:+[A foreach:]
+objc-general.m__+[A foreach:]
 6
 2
 1

Modified: cfe/trunk/test/Profile/c-captured.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Profile/c-captured.c?rev=255366=255365=255366=diff
==
--- cfe/trunk/test/Profile/c-captured.c (original)
+++ cfe/trunk/test/Profile/c-captured.c Fri Dec 11 13:53:35 2015
@@ -4,8 +4,8 @@
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.9 -main-file-name 
c-captured.c %s -o - -emit-llvm -fprofile-instr-use=%t.profdata | FileCheck 
-check-prefix=PGOUSE -check-prefix=PGOALL %s
 
 // PGOGEN: @[[DCC:__llvm_profile_counters_debug_captured]] = private global [3 
x i64] zeroinitializer
-// PGOGEN: @[[CSC:"__llvm_profile_counters_c-captured.c:__captured_stmt"]] = 
private global [2 x i64] 

r255368 - [PGO] Revert r255366: solution incomplete, not handling lambda yet

[Xinliang David Li via cfe-commits]

Author: davidxl
Date: Fri Dec 11 14:23:12 2015
New Revision: 255368

URL: http://llvm.org/viewvc/llvm-project?rev=255368=rev
Log:
[PGO] Revert r255366: solution incomplete, not handling lambda yet

Modified:
cfe/trunk/test/CoverageMapping/unused_names.c
cfe/trunk/test/Profile/Inputs/c-captured.proftext
cfe/trunk/test/Profile/Inputs/c-general.proftext
cfe/trunk/test/Profile/Inputs/cxx-lambda.proftext
cfe/trunk/test/Profile/Inputs/objc-general.proftext
cfe/trunk/test/Profile/c-captured.c
cfe/trunk/test/Profile/c-general.c
cfe/trunk/test/Profile/c-linkage.c
cfe/trunk/test/Profile/cxx-lambda.cpp
cfe/trunk/test/Profile/objc-general.m

Modified: cfe/trunk/test/CoverageMapping/unused_names.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CoverageMapping/unused_names.c?rev=255368=255367=255368=diff
==
--- cfe/trunk/test/CoverageMapping/unused_names.c (original)
+++ cfe/trunk/test/CoverageMapping/unused_names.c Fri Dec 11 14:23:12 2015
@@ -6,7 +6,7 @@
 
 // CHECK-DAG: @__llvm_profile_name_bar = {{.*}} [3 x i8] c"bar", section 
"{{.*}}__llvm_prf_names"
 // CHECK-DAG: @__llvm_profile_name_baz = {{.*}} [3 x i8] c"baz", section 
"{{.*}}__llvm_prf_names"
-// CHECK-DAG: @__llvm_profile_name_unused_names.c__qux = {{.*}} [19 x i8] 
c"unused_names.c__qux", section "{{.*}}__llvm_prf_names"
+// CHECK-DAG: @"__llvm_profile_name_unused_names.c:qux" = {{.*}} [18 x i8] 
c"unused_names.c:qux", section "{{.*}}__llvm_prf_names"
 
 // SYSHEADER-NOT: @__llvm_profile_name_foo =
 

Modified: cfe/trunk/test/Profile/Inputs/c-captured.proftext
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Profile/Inputs/c-captured.proftext?rev=255368=255367=255368=diff
==
--- cfe/trunk/test/Profile/Inputs/c-captured.proftext (original)
+++ cfe/trunk/test/Profile/Inputs/c-captured.proftext Fri Dec 11 14:23:12 2015
@@ -1,10 +1,10 @@
-c-captured.ccaptured_stmt
+c-captured.c:__captured_stmt
 10
 2
 1
 1
 
-c-captured.ccaptured_stmt.1
+c-captured.c:__captured_stmt.1
 266
 3
 1

Modified: cfe/trunk/test/Profile/Inputs/c-general.proftext
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Profile/Inputs/c-general.proftext?rev=255368=255367=255368=diff
==
--- cfe/trunk/test/Profile/Inputs/c-general.proftext (original)
+++ cfe/trunk/test/Profile/Inputs/c-general.proftext Fri Dec 11 14:23:12 2015
@@ -149,7 +149,7 @@ main
 1
 1
 
-c-general.c__static_func
+c-general.c:static_func
 4
 2
 1

Modified: cfe/trunk/test/Profile/Inputs/cxx-lambda.proftext
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Profile/Inputs/cxx-lambda.proftext?rev=255368=255367=255368=diff
==
--- cfe/trunk/test/Profile/Inputs/cxx-lambda.proftext (original)
+++ cfe/trunk/test/Profile/Inputs/cxx-lambda.proftext Fri Dec 11 14:23:12 2015
@@ -1,4 +1,4 @@
-cxx-lambda.cpp___ZZ7lambdasvENK3$_0clEi
+cxx-lambda.cpp:_ZZ7lambdasvENK3$_0clEi
 654
 3
 10

Modified: cfe/trunk/test/Profile/Inputs/objc-general.proftext
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Profile/Inputs/objc-general.proftext?rev=255368=255367=255368=diff
==
--- cfe/trunk/test/Profile/Inputs/objc-general.proftext (original)
+++ cfe/trunk/test/Profile/Inputs/objc-general.proftext Fri Dec 11 14:23:12 2015
@@ -1,10 +1,10 @@
-objc-general.m13+[A foreach:]_block_invoke
+objc-general.m:__13+[A foreach:]_block_invoke
 10
 2
 2
 1
 
-objc-general.m__+[A foreach:]
+objc-general.m:+[A foreach:]
 6
 2
 1

Modified: cfe/trunk/test/Profile/c-captured.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Profile/c-captured.c?rev=255368=255367=255368=diff
==
--- cfe/trunk/test/Profile/c-captured.c (original)
+++ cfe/trunk/test/Profile/c-captured.c Fri Dec 11 14:23:12 2015
@@ -4,8 +4,8 @@
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.9 -main-file-name 
c-captured.c %s -o - -emit-llvm -fprofile-instr-use=%t.profdata | FileCheck 
-check-prefix=PGOUSE -check-prefix=PGOALL %s
 
 // PGOGEN: @[[DCC:__llvm_profile_counters_debug_captured]] = private global [3 
x i64] zeroinitializer
-// PGOGEN: @[[CSC:__llvm_profile_counters_c-captured.ccaptured_stmt]] = 
private global [2 x i64] zeroinitializer
-// PGOGEN: @[[C1C:__llvm_profile_counters_c-captured.ccaptured_stmt.1]] = 
private global [3 x i64] zeroinitializer
+// PGOGEN: @[[CSC:"__llvm_profile_counters_c-captured.c:__captured_stmt"]] = 
private global [2 x i64] zeroinitializer
+// PGOGEN: @[[C1C:"__llvm_profile_counters_c-captured.c:__captured_stmt.1"]] = 
private global [3 x i64] zeroinitializer
 
 // PGOALL-LABEL: define void @debug_captured()
 // 

r255326 - [PGO] Add a test case to cover version-3 format

[Xinliang David Li via cfe-commits]

Author: davidxl
Date: Thu Dec 10 22:02:57 2015
New Revision: 255326

URL: http://llvm.org/viewvc/llvm-project?rev=255326=rev
Log:
[PGO] Add a test case to cover version-3 format

Added:
cfe/trunk/test/Profile/Inputs/c-general.profdata.v3   (with props)
Modified:
cfe/trunk/test/Profile/c-general.c

Added: cfe/trunk/test/Profile/Inputs/c-general.profdata.v3
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Profile/Inputs/c-general.profdata.v3?rev=255326=auto
==
Binary file - no diff available.

Propchange: cfe/trunk/test/Profile/Inputs/c-general.profdata.v3
--
svn:mime-type = application/octet-stream

Modified: cfe/trunk/test/Profile/c-general.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Profile/c-general.c?rev=255326=255325=255326=diff
==
--- cfe/trunk/test/Profile/c-general.c (original)
+++ cfe/trunk/test/Profile/c-general.c Thu Dec 10 22:02:57 2015
@@ -4,7 +4,7 @@
 
 // RUN: llvm-profdata merge %S/Inputs/c-general.proftext -o %t.profdata
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.9 -main-file-name c-general.c 
%s -o - -emit-llvm -fprofile-instr-use=%t.profdata | FileCheck 
-check-prefix=PGOUSE %s
-
+// RUN: %clang_cc1 -triple x86_64-apple-macosx10.9 -main-file-name c-general.c 
%s -o - -emit-llvm -fprofile-instr-use=%S/Inputs/c-general.profdata.v3 | 
FileCheck -check-prefix=PGOUSE %s
 // Also check compatibility with older profiles.
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.9 -main-file-name c-general.c 
%s -o - -emit-llvm -fprofile-instr-use=%S/Inputs/c-general.profdata.v1 | 
FileCheck -check-prefix=PGOUSE %s
 


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

r254839 - Pass profile version info to name API (NFC)

[Xinliang David Li via cfe-commits]

Author: davidxl
Date: Fri Dec  4 23:37:15 2015
New Revision: 254839

URL: http://llvm.org/viewvc/llvm-project?rev=254839=rev
Log:
Pass profile version info to name 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=254839=254838=254839=diff
==
--- cfe/trunk/lib/CodeGen/CodeGenPGO.cpp (original)
+++ cfe/trunk/lib/CodeGen/CodeGenPGO.cpp Fri Dec  4 23:37:15 2015
@@ -28,7 +28,10 @@ using namespace CodeGen;
 
 void CodeGenPGO::setFuncName(StringRef Name,
  llvm::GlobalValue::LinkageTypes Linkage) {
-  FuncName = llvm::getPGOFuncName(Name, Linkage, 
CGM.getCodeGenOpts().MainFileName);
+  llvm::IndexedInstrProfReader *PGOReader = CGM.getPGOReader();
+  FuncName = llvm::getPGOFuncName(
+  Name, Linkage, CGM.getCodeGenOpts().MainFileName,
+  PGOReader ? PGOReader->getVersion() : llvm::IndexedInstrProf::Version);
 
   // If we're generating a profile, create a variable for the name.
   if (CGM.getCodeGenOpts().ProfileInstrGenerate)


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

r253886 - Disable frame pointer elimination when using -pg

[Xinliang David Li via cfe-commits]

Author: davidxl
Date: Mon Nov 23 11:30:31 2015
New Revision: 253886

URL: http://llvm.org/viewvc/llvm-project?rev=253886=rev
Log:
Disable frame pointer elimination when using -pg 

(Re-apply patch after bug fixing)

This diff makes sure that the driver does not pass
-fomit-frame-pointer or -momit-leaf-frame-pointer to
the frontend when -pg is used. Currently, clang gives 
an error if -fomit-frame-pointer is used in combination 
with -pg, but -momit-leaf-frame-pointer was forgotten.
Also, disable frame pointer elimination in the frontend 
when -pg is set.

Patch by Stefan Kempf.

Added:
cfe/trunk/test/CodeGen/x86_64-profiling-keep-fp.c
Modified:
cfe/trunk/lib/Driver/Tools.cpp
cfe/trunk/lib/Frontend/CompilerInvocation.cpp

Modified: cfe/trunk/lib/Driver/Tools.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Tools.cpp?rev=253886=253885=253886=diff
==
--- cfe/trunk/lib/Driver/Tools.cpp (original)
+++ cfe/trunk/lib/Driver/Tools.cpp Mon Nov 23 11:30:31 2015
@@ -2794,6 +2794,8 @@ static bool shouldUseFramePointer(const
   if (Arg *A = Args.getLastArg(options::OPT_fno_omit_frame_pointer,
options::OPT_fomit_frame_pointer))
 return A->getOption().matches(options::OPT_fno_omit_frame_pointer);
+  if (Args.hasArg(options::OPT_pg))
+return true;
 
   return shouldUseFramePointerForTarget(Args, Triple);
 }
@@ -2803,6 +2805,8 @@ static bool shouldUseLeafFramePointer(co
   if (Arg *A = Args.getLastArg(options::OPT_mno_omit_leaf_frame_pointer,
options::OPT_momit_leaf_frame_pointer))
 return A->getOption().matches(options::OPT_mno_omit_leaf_frame_pointer);
+  if (Args.hasArg(options::OPT_pg))
+return true;
 
   if (Triple.isPS4CPU())
 return false;

Modified: cfe/trunk/lib/Frontend/CompilerInvocation.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInvocation.cpp?rev=253886=253885=253886=diff
==
--- cfe/trunk/lib/Frontend/CompilerInvocation.cpp (original)
+++ cfe/trunk/lib/Frontend/CompilerInvocation.cpp Mon Nov 23 11:30:31 2015
@@ -453,7 +453,8 @@ static bool ParseCodeGenArgs(CodeGenOpti
   Opts.CXXCtorDtorAliases = Args.hasArg(OPT_mconstructor_aliases);
   Opts.CodeModel = getCodeModel(Args, Diags);
   Opts.DebugPass = Args.getLastArgValue(OPT_mdebug_pass);
-  Opts.DisableFPElim = Args.hasArg(OPT_mdisable_fp_elim);
+  Opts.DisableFPElim =
+  (Args.hasArg(OPT_mdisable_fp_elim) || Args.hasArg(OPT_pg));
   Opts.DisableFree = Args.hasArg(OPT_disable_free);
   Opts.DisableTailCalls = Args.hasArg(OPT_mdisable_tail_calls);
   Opts.FloatABI = Args.getLastArgValue(OPT_mfloat_abi);

Added: cfe/trunk/test/CodeGen/x86_64-profiling-keep-fp.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/x86_64-profiling-keep-fp.c?rev=253886=auto
==
--- cfe/trunk/test/CodeGen/x86_64-profiling-keep-fp.c (added)
+++ cfe/trunk/test/CodeGen/x86_64-profiling-keep-fp.c Mon Nov 23 11:30:31 2015
@@ -0,0 +1,14 @@
+// REQUIRES: x86-registered-target
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -O3 -pg -S -o - %s | \
+// RUN:   FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -O3 
-momit-leaf-frame-pointer -pg -S -o - %s | \
+// RUN:   FileCheck %s
+
+// Test that the frame pointer is kept when compiling with
+// profiling.
+
+//CHECK: pushq %rbp
+int main(void)
+{
+  return 0;
+}


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

r253851 - Revert r253846 (build bot failure))

[Xinliang David Li via cfe-commits]

Author: davidxl
Date: Sun Nov 22 23:41:05 2015
New Revision: 253851

URL: http://llvm.org/viewvc/llvm-project?rev=253851=rev
Log:
Revert r253846 (build bot failure))

Removed:
cfe/trunk/test/CodeGen/x86_64-profiling-keep-fp.c
Modified:
cfe/trunk/lib/Driver/Tools.cpp
cfe/trunk/lib/Frontend/CompilerInvocation.cpp

Modified: cfe/trunk/lib/Driver/Tools.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Tools.cpp?rev=253851=253850=253851=diff
==
--- cfe/trunk/lib/Driver/Tools.cpp (original)
+++ cfe/trunk/lib/Driver/Tools.cpp Sun Nov 22 23:41:05 2015
@@ -2794,8 +2794,6 @@ static bool shouldUseFramePointer(const
   if (Arg *A = Args.getLastArg(options::OPT_fno_omit_frame_pointer,
options::OPT_fomit_frame_pointer))
 return A->getOption().matches(options::OPT_fno_omit_frame_pointer);
-  if (Args.hasArg(options::OPT_pg))
-return true;
 
   return shouldUseFramePointerForTarget(Args, Triple);
 }
@@ -2805,8 +2803,6 @@ static bool shouldUseLeafFramePointer(co
   if (Arg *A = Args.getLastArg(options::OPT_mno_omit_leaf_frame_pointer,
options::OPT_momit_leaf_frame_pointer))
 return A->getOption().matches(options::OPT_mno_omit_leaf_frame_pointer);
-  if (Args.hasArg(options::OPT_pg))
-return true;
 
   if (Triple.isPS4CPU())
 return false;

Modified: cfe/trunk/lib/Frontend/CompilerInvocation.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInvocation.cpp?rev=253851=253850=253851=diff
==
--- cfe/trunk/lib/Frontend/CompilerInvocation.cpp (original)
+++ cfe/trunk/lib/Frontend/CompilerInvocation.cpp Sun Nov 22 23:41:05 2015
@@ -453,8 +453,7 @@ static bool ParseCodeGenArgs(CodeGenOpti
   Opts.CXXCtorDtorAliases = Args.hasArg(OPT_mconstructor_aliases);
   Opts.CodeModel = getCodeModel(Args, Diags);
   Opts.DebugPass = Args.getLastArgValue(OPT_mdebug_pass);
-  Opts.DisableFPElim =
-  (Args.hasArg(OPT_mdisable_fp_elim) || Args.hasArg(OPT_pg));
+  Opts.DisableFPElim = Args.hasArg(OPT_mdisable_fp_elim);
   Opts.DisableFree = Args.hasArg(OPT_disable_free);
   Opts.DisableTailCalls = Args.hasArg(OPT_mdisable_tail_calls);
   Opts.FloatABI = Args.getLastArgValue(OPT_mfloat_abi);

Removed: cfe/trunk/test/CodeGen/x86_64-profiling-keep-fp.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/x86_64-profiling-keep-fp.c?rev=253850=auto
==
--- cfe/trunk/test/CodeGen/x86_64-profiling-keep-fp.c (original)
+++ cfe/trunk/test/CodeGen/x86_64-profiling-keep-fp.c (removed)
@@ -1,13 +0,0 @@
-// RUN: %clang_cc1 -triple x86_64-unknown-unknown -O3 -pg -S -o - %s | \
-// RUN:   FileCheck %s
-// RUN: %clang_cc1 -triple x86_64-unknown-unknown -O3 
-momit-leaf-frame-pointer -pg -S -o - %s | \
-// RUN:   FileCheck %s
-
-// Test that the frame pointer is kept when compiling with
-// profiling.
-
-//CHECK: pushq %rbp
-int main(void)
-{
-  return 0;
-}


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

r253846 - Disable frame pointer elimination when using -pg

[Xinliang David Li via cfe-commits]

Author: davidxl
Date: Sun Nov 22 23:09:10 2015
New Revision: 253846

URL: http://llvm.org/viewvc/llvm-project?rev=253846=rev
Log:
Disable frame pointer elimination when using -pg

This diff makes sure that the driver does not pass
-fomit-frame-pointer or -momit-leaf-frame-pointer to
the frontend when -pg is used. Currently, clang gives 
an error if -fomit-frame-pointer is used in combination 
with -pg, but -momit-leaf-frame-pointer was forgotten.
Also, disable frame pointer elimination in the frontend 
when -pg is set.

Patch by Stefan Kempf.

Added:
cfe/trunk/test/CodeGen/x86_64-profiling-keep-fp.c
Modified:
cfe/trunk/lib/Driver/Tools.cpp
cfe/trunk/lib/Frontend/CompilerInvocation.cpp

Modified: cfe/trunk/lib/Driver/Tools.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Tools.cpp?rev=253846=253845=253846=diff
==
--- cfe/trunk/lib/Driver/Tools.cpp (original)
+++ cfe/trunk/lib/Driver/Tools.cpp Sun Nov 22 23:09:10 2015
@@ -2794,6 +2794,8 @@ static bool shouldUseFramePointer(const
   if (Arg *A = Args.getLastArg(options::OPT_fno_omit_frame_pointer,
options::OPT_fomit_frame_pointer))
 return A->getOption().matches(options::OPT_fno_omit_frame_pointer);
+  if (Args.hasArg(options::OPT_pg))
+return true;
 
   return shouldUseFramePointerForTarget(Args, Triple);
 }
@@ -2803,6 +2805,8 @@ static bool shouldUseLeafFramePointer(co
   if (Arg *A = Args.getLastArg(options::OPT_mno_omit_leaf_frame_pointer,
options::OPT_momit_leaf_frame_pointer))
 return A->getOption().matches(options::OPT_mno_omit_leaf_frame_pointer);
+  if (Args.hasArg(options::OPT_pg))
+return true;
 
   if (Triple.isPS4CPU())
 return false;

Modified: cfe/trunk/lib/Frontend/CompilerInvocation.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInvocation.cpp?rev=253846=253845=253846=diff
==
--- cfe/trunk/lib/Frontend/CompilerInvocation.cpp (original)
+++ cfe/trunk/lib/Frontend/CompilerInvocation.cpp Sun Nov 22 23:09:10 2015
@@ -453,7 +453,8 @@ static bool ParseCodeGenArgs(CodeGenOpti
   Opts.CXXCtorDtorAliases = Args.hasArg(OPT_mconstructor_aliases);
   Opts.CodeModel = getCodeModel(Args, Diags);
   Opts.DebugPass = Args.getLastArgValue(OPT_mdebug_pass);
-  Opts.DisableFPElim = Args.hasArg(OPT_mdisable_fp_elim);
+  Opts.DisableFPElim =
+  (Args.hasArg(OPT_mdisable_fp_elim) || Args.hasArg(OPT_pg));
   Opts.DisableFree = Args.hasArg(OPT_disable_free);
   Opts.DisableTailCalls = Args.hasArg(OPT_mdisable_tail_calls);
   Opts.FloatABI = Args.getLastArgValue(OPT_mfloat_abi);

Added: cfe/trunk/test/CodeGen/x86_64-profiling-keep-fp.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/x86_64-profiling-keep-fp.c?rev=253846=auto
==
--- cfe/trunk/test/CodeGen/x86_64-profiling-keep-fp.c (added)
+++ cfe/trunk/test/CodeGen/x86_64-profiling-keep-fp.c Sun Nov 22 23:09:10 2015
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -O3 -pg -S -o - %s | \
+// RUN:   FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -O3 
-momit-leaf-frame-pointer -pg -S -o - %s | \
+// RUN:   FileCheck %s
+
+// Test that the frame pointer is kept when compiling with
+// profiling.
+
+//CHECK: pushq %rbp
+int main(void)
+{
+  return 0;
+}


___
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=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=252433=252434=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

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=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=252146=252147=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 ,
 }
 
 void CoverageMappingModuleGen::addFunctionMappingRecord(
-llvm::GlobalVariable *FunctionName, StringRef FunctionNameValue,
-uint64_t FunctionHash, const std::string ) {
+llvm::GlobalVariable *NamePtr, StringRef NameValue,
+uint64_t FuncHash, const std::string ) {
   llvm::LLVMContext  = 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

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=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=251610=251611=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

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=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=251384=251385=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 ,
+ llvm::opt::ArgStringList ) 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 , const llvm::Triple ,

Modified: cfe/trunk/lib/Driver/ToolChains.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains.h?rev=251385=251384=251385=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 ) const override;
   bool isPIEDefault() const override;
   SanitizerMask getSupportedSanitizers() const override;
+  void addProfileRTLibs(const llvm::opt::ArgList ,
+llvm::opt::ArgStringList ) 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

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

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=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=250993=250994=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 ,
+ StringRef Component,
+ bool Shared = false) const;
+  /// needsProfileRT - returns true if instrumentation profile is on.
+  static bool needsProfileRT(const llvm::opt::ArgList );
+
   /// 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 ,
+  virtual bool AddFastMathRuntimeIfAvailable(
+  const llvm::opt::ArgList , llvm::opt::ArgStringList ) const;
+  /// addProfileRTLibs - When -fprofile-instr-profile is specified, add profile
+  /// runtime library, otherwise return false.
+  virtual void addProfileRTLibs(const llvm::opt::ArgList ,
 llvm::opt::ArgStringList ) 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=250993=250994=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 ,
+  StringRef Component,
+  bool Shared) const {
+  return Args.MakeArgString(getCompilerRT(Args, Component, Shared));
+}
+
+bool ToolChain::needsProfileRT(const ArgList ) {
+  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 ) 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 ) const {}
 
+void ToolChain::addProfileRTLibs(const llvm::opt::ArgList ,
+ llvm::opt::ArgStringList ) const {
+  if (!needsProfileRT(Args)) return;
+
+  CmdArgs.push_back(getCompilerRTArgString(Args, "profile"));
+  return;
+}
+
 ToolChain::RuntimeLibType ToolChain::GetRuntimeLibType(
-  const ArgList ) const
-{
+const ArgList ) 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=250993=250994=diff
==
--- cfe/trunk/lib/Driver/ToolChains.cpp 

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=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=251071=251072=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=251071=251072=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 ) {
-  return isMachO(CGM) ? "__DATA,__llvm_covmap" : "__llvm_covmap";
+  return llvm::getInstrProfCoverageSectionName(isMachO(CGM));
 }
 
 static void dump(llvm::raw_ostream , 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] D13326: [PGO]: Eliminate __llvm_profile_register calls for Linux (clang changes)

[Xinliang David Li via cfe-commits]

Clang FE refactoring change is not needed anymore.

In commit 0d32e7d952bc80830183e0c2c6ec5027ca6b1450, Vasileios
Kalintiris did the same thing for multi-lib support.

David

On Tue, Oct 6, 2015 at 12:13 AM, Justin Bogner  wrote:
> David Li  writes:
>> davidxl updated this revision to Diff 36316.
>> davidxl added a comment.
>>
>> I have modified the implementation to not use linker script, so this
>> clang patch becomes strictly refactoring with NFC. I think it is still
>> a good thing to have this in so that similar tunings like this can be
>> done in the future.
>
> Sure. I have a couple of nitpicks but this basically LGTM.
>
>>
>> http://reviews.llvm.org/D13326
>>
>> Files:
>>   include/clang/Driver/ToolChain.h
>>   lib/Driver/SanitizerArgs.cpp
>>   lib/Driver/ToolChain.cpp
>>   lib/Driver/ToolChains.cpp
>>   lib/Driver/ToolChains.h
>>   lib/Driver/Tools.cpp
>>
>> Index: lib/Driver/Tools.cpp
>> ===
>> --- lib/Driver/Tools.cpp
>> +++ lib/Driver/Tools.cpp
>> @@ -2402,83 +2402,12 @@
>>}
>>  }
>>
>> -// Until ARM libraries are build separately, we have them all in one library
>> -static StringRef getArchNameForCompilerRTLib(const ToolChain ,
>> - const ArgList ) {
>> -  const llvm::Triple  = TC.getTriple();
>> -  bool IsWindows = Triple.isOSWindows();
>> -
>> -  if (Triple.isWindowsMSVCEnvironment() && TC.getArch() == 
>> llvm::Triple::x86)
>> -return "i386";
>> -
>> -  if (TC.getArch() == llvm::Triple::arm || TC.getArch() == 
>> llvm::Triple::armeb)
>> -return (arm::getARMFloatABI(TC, Args) == arm::FloatABI::Hard && 
>> !IsWindows)
>> -   ? "armhf"
>> -   : "arm";
>> -
>> -  return TC.getArchName();
>> -}
>> -
>> -static SmallString<128> getCompilerRTLibDir(const ToolChain ) {
>> -  // The runtimes are located in the OS-specific resource directory.
>> -  SmallString<128> Res(TC.getDriver().ResourceDir);
>> -  const llvm::Triple  = TC.getTriple();
>> -  // TC.getOS() yield "freebsd10.0" whereas "freebsd" is expected.
>> -  StringRef OSLibName =
>> -  (Triple.getOS() == llvm::Triple::FreeBSD) ? "freebsd" : TC.getOS();
>> -  llvm::sys::path::append(Res, "lib", OSLibName);
>> -  return Res;
>> -}
>> -
>> -SmallString<128> tools::getCompilerRT(const ToolChain , const ArgList 
>> ,
>> -  StringRef Component, bool Shared) {
>> -  const char *Env = TC.getTriple().getEnvironment() == llvm::Triple::Android
>> -? "-android"
>> -: "";
>> -
>> -  bool IsOSWindows = TC.getTriple().isOSWindows();
>> -  bool IsITANMSVCWindows = TC.getTriple().isWindowsMSVCEnvironment() ||
>> -   TC.getTriple().isWindowsItaniumEnvironment();
>> -  StringRef Arch = getArchNameForCompilerRTLib(TC, Args);
>> -  const char *Prefix = IsITANMSVCWindows ? "" : "lib";
>> -  const char *Suffix =
>> -  Shared ? (IsOSWindows ? ".dll" : ".so") : (IsITANMSVCWindows ? ".lib" 
>> : ".a");
>> -
>> -  SmallString<128> Path = getCompilerRTLibDir(TC);
>> -  llvm::sys::path::append(Path, Prefix + Twine("clang_rt.") + Component + 
>> "-" +
>> -Arch + Env + Suffix);
>> -
>> -  return Path;
>> -}
>> -
>> -static const char *getCompilerRTArgString(const ToolChain ,
>> -  const llvm::opt::ArgList ,
>> -  StringRef Component,
>> -  bool Shared = false) {
>> -  return Args.MakeArgString(getCompilerRT(TC, Args, Component, Shared));
>> -}
>> -
>>  // This adds the static libclang_rt.builtins-arch.a directly to the command 
>> line
>>  // FIXME: Make sure we can also emit shared objects if they're requested
>>  // and available, check for possible errors, etc.
>>  static void addClangRT(const ToolChain , const ArgList ,
>> ArgStringList ) {
>> -  CmdArgs.push_back(getCompilerRTArgString(TC, Args, "builtins"));
>> -}
>> -
>> -static void addProfileRT(const ToolChain , const ArgList ,
>> - ArgStringList ) {
>> -  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;
>> -
>> -  CmdArgs.push_back(getCompilerRTArgString(TC, Args, "profile"));
>> +  CmdArgs.push_back(TC.getCompilerRTArgString(Args, "builtins"));
>>  }
>>
>>  namespace {
>> @@ -2559,7 +2488,7 @@
>>// whole-archive.
>>if (!IsShared)
>>  

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 e...@efcs.ca 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