[PATCH] D12781: PGO IR-level instrumentation infrastructure

[Sean Silva via llvm-commits]

On Thu, Nov 19, 2015 at 10:00 PM, Xinliang David Li <davidxl at google.com>
wrote:

>
>
> On Thu, Nov 19, 2015 at 7:22 PM, Sean Silva <chisophugis at gmail.com> wrote:
>
>>
>>
>> On Thu, Nov 19, 2015 at 6:59 PM, Xinliang David Li <davidxl at google.com>
>> wrote:
>>
>>> I thought about the size of the test case too. Initially I had the same
>>> concern, but in second look, it does not seem to be brittle -- because it
>>> just checks whether the key branches are properly annotated with the right
>>> profile count or not, or profile counter update instruction is inserted or
>>> not.
>>>
>>
>> It is brittle because it depends on checked in binary files that need to
>> be regenerated. For example, a person on Mac or Windows might have trouble
>> regenerating IR with `target triple = "x86_64-unknown-linux-gnu"`...
>>
>>
>
> This is the current common practice in testing PGO use and coverage. There
> are existing binary profile data checked in for both Instrument and Sample
> PGO.
>
> Also I am not sure what you mean by re-generating IR -- there is no need
> to re-generate IR.  Indexed profile data needs to be generated
> occasionally, but their format is system independent.  For the same program
> with the same training set, the indexed profile should be identical
> regardless whether the running system is 32bit or 64bit, LE or BE.
>

Sorry, I missed some words there, I meant something like "regenerating the
profiles from IR with IR with `target triple =
"x86_64-unknown-linux-gnu"`". For a binary profile file, that is probably
an easier route than trying to edit the file directly.
Using the text format as Justin suggests will hopefully avoid some of these
issues because text files are easier to edit/modify/understand.

-- Sean Silva


>
> David
>
>
>
>>
>>> I think adding C++ unit tests are independent tasks which can be done
>>> once the driver part of the changes land.
>>>
>>
>> I assume by "driver" you mean the clang driver. Note that the IR level
>> instrumentation must be independent of what frontend generated the IR, so I
>> don't see the connection between the testing for this patch and any work in
>> clang.
>>
>> (FYI "unit tests" in LLVM-land means the C++ tests in unittests/; the
>> rest are "lit tests"; I said "C++ unit tests" previously because it seemed
>> that Rong was unfamiliar with the LLVM terminology and I didn't want to
>> distract by clarifying this
>> )
>>
>> -- Sean Silva
>>
>>
>>>
>>> David
>>>
>>>
>>>
>>> On Thu, Nov 19, 2015 at 6:40 PM, Sean Silva <chisophugis at gmail.com>
>>> wrote:
>>>
>>>> silvas added a comment.
>>>>
>>>> These test files look like they are just a dump of IR generated from a
>>>> C/C++, which is extremely verbose and has a lot of inessential details. I
>>>> also don't like checking in binary profdata files. Overall these tests seem
>>>> extremely brittle. I also don't understand why the test files have names
>>>> like "for" or "goto" or "ifelse". Those concepts don't exist in the IR.
>>>> Surely the tests should have names like "criticaledge" etc.
>>>>
>>>> It seems like this testing might be more readable as C++ unit tests.
>>>> Using the new pass manager this should be easy to wire up. I think the
>>>> hardest part would be to stub out IndexedInstrProfReader.
>>>>
>>>>
>>>> http://reviews.llvm.org/D12781
>>>>
>>>>
>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151120/564d72fc/attachment.html>