[PATCH] D12781: PGO IR-level instrumentation infrastructure

[Sean Silva via llvm-commits]

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"`...


>
> 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/20151119/0d99fb0a/attachment.html>