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

[Sean Silva via llvm-commits]

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/7c773678/attachment.html>