[PATCH] D20993: [lit] Add support for PGO profile and code coverage collection

Sat Jun 4 17:14:35 PDT 2016

Well said!

thanks,

David

On Sat, Jun 4, 2016 at 5:04 PM, Sean Silva <chisophugis at gmail.com> wrote:

>
>
> On Sat, Jun 4, 2016 at 8:32 AM, Xinliang David Li <davidxl at google.com>
> wrote:
>
>>
>>
>> On Sat, Jun 4, 2016 at 1:29 AM, Sean Silva <chisophugis at gmail.com> wrote:
>>
>>>
>>>
>>> On Fri, Jun 3, 2016 at 9:47 PM, Xinliang David Li via llvm-commits <
>>> llvm-commits at lists.llvm.org> wrote:
>>>
>>>>
>>>>
>>>> On Fri, Jun 3, 2016 at 9:37 PM, Matthias Braun <matze at braunis.de>
>>>> wrote:
>>>>
>>>>>
>>>>> On Jun 3, 2016, at 9:34 PM, Xinliang David Li via llvm-commits <
>>>>> llvm-commits at lists.llvm.org> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On Fri, Jun 3, 2016 at 9:32 PM, Matthias Braun <matze at braunis.de>
>>>>> wrote:
>>>>>
>>>>>> MatzeB added a comment.
>>>>>>
>>>>>> In http://reviews.llvm.org/D20993#449116, @vsk wrote:
>>>>>>
>>>>>> > In http://reviews.llvm.org/D20993#449092, @MatzeB wrote:
>>>>>> >
>>>>>> > > This adds a bigger bunch of code to lit, I wonder if it is
>>>>>> necessary:
>>>>>> > >
>>>>>> > > - I assume the necessity for the logic inside lit is just the
>>>>>> fact that the profile data overwrites each other after running a command.
>>>>>> Is setting LLVM_PROFILE_FILE with a '%p' placeholder not enough to avoid
>>>>>> this?
>>>>>> >
>>>>>> >
>>>>>> > Using PID substitution gets close to solving the problem of having
>>>>>> overwritten profiles, but not all the way. On 32-bit systems PID wraparound
>>>>>> would pose a real problem. In this patch I include the hash of the test
>>>>>> command to minimize loss of profiles.
>>>>>>
>>>>>>
>>>>>> This is a really annoying problem to have... Maybe we should find a
>>>>>> solution within the profile infrastructure itself (can we add another flag
>>>>>> that adds a unique suffix to the filename if it already exists?) so not
>>>>>> every user of the profiling infrastructure has to jump through the same
>>>>>> hoops (I've done a similar dance in the test-suite profile support).
>>>>>>
>>>>>> >
>>>>>>
>>>>>> >
>>>>>>
>>>>>> > > The final profile data merging can always be done outside of
>>>>>> llvm-lit afterwards.
>>>>>>
>>>>>> >
>>>>>>
>>>>>> >
>>>>>>
>>>>>> > On a practical level, I don't think this is possible. Raw profiles
>>>>>> are too large. Turning off the cleanup step and running check-llvm produces
>>>>>> over half a terrabyte of data. There are a few ways to address this without
>>>>>> touching lit:
>>>>>>
>>>>>> >
>>>>>>
>>>>>> > 1. Reduce the size of raw profiles. This would make the compiler
>>>>>> runtime larger and more complex.
>>>>>>
>>>>>> > 2. Run a monitor process that does the merging/cleanup. I don't
>>>>>> think that approach has any advantages compared to modifying lit.
>>>>>>
>>>>>> >
>>>>>>
>>>>>> >   Alternatively, we could do in-place raw profile merging in the
>>>>>> compiler runtime. I don't think that's preferable because we'd have to
>>>>>> introduce a lot of complexity to compiler-rt (e.g portable mandatory file
>>>>>> locking).
>>>>>>
>>>>>>
>>>>>> Ah, so that seems harder to solve without an external driver merging
>>>>>> the profile data. I'm still not a big fan of injecting this stuff into the
>>>>>> core of lit, but we may have no other choice today.
>>>>>>
>>>>>
>>>>> This is handled by the compiler-rt -- it should be totally transparent
>>>>> to the user.
>>>>>
>>>>>
>>>>> That would indeed be a big usability improvement. Does it also free
>>>>> you from running an extra tool afterwards to convert from .profraw to
>>>>> .profdata format?
>>>>>
>>>>
>>>> The merging is only for raw format profiles which will be  super fast.
>>>>   The conversion from raw to index format is still needed -- but the
>>>> overhead of that is very low because thousands of raw profiles have already
>>>> being reduced to a single raw file (per-executable).
>>>>
>>>> If we really want to simplify the use model, the compiler can be taught
>>>> to recognize raw profile (and convert it to indexed format on the fly)
>>>> without user knowing yet. I am yet to see a good justification for this
>>>> support.
>>>>
>>>
>>> It also sounds like it would have a scaling problem. It would lead to
>>> O(profile size) work done by each compiler invocation, instead of O(data
>>> needed by the compiler). Since O(profile size) is generally O(number
>>> compiler invocations), this would be quadratic in the project size.
>>>
>>
>> yes. The implementation can however 'cache' the indexed profile data in a
>> file that is discoverable by later compiler invocations The file should
>> contain info of the raw profile data's signature.  If the cached file is
>> found and matches the raw data, it will be used instead.
>>
>
> My experience when working on Clang's C++ modules for PS4 is that this
> kind of automatic "caching" by the compiler is of limited applicability. It:
> - makes the compiler invocations non-functional (i.e. non-pure)
> - serializes the build behind the back of the build system and contrary to
> user expectations
> - circumvents the build system's dependency management; "make clean" will
> not work as expected
> - the compiler has to start having a policy about cache expiration /
> pruning otherwise user's disk will fill up with files
> - compiler invocations need to be assumed to have a shared file system
> (otherwise the "cache" is pointless and quadratic behavior remains)
> - subject to these limitations, there is a large documentation burden for
> explaining the limitations to users the feature, thus negating much of the
> simplicity it is supposed to provide.
>
> So I have to agree with your statement that "I am yet to see a good
> justification for this support". Perhaps my experiences bias me towards
> being extra cautious about adding a feature like this.
>
> -- Sean Silva
>
>
>>
>> David
>>
>>>
>>> -- Sean Silva
>>>
>>>
>>>>
>>>> thanks,
>>>>
>>>> David
>>>>
>>>>
>>>>>
>>>>> - Matthias
>>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at lists.llvm.org
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160604/d641a8b8/attachment.html>