[PATCH] D12781: PGO IR-level instrumentation infrastructure

[Rong Xu via llvm-commits]

I posted the revised patch in the old review link:
http://reviews.llvm.org/D12781

It's still marked as closed. Let me know if you think I need to open
another review.

Testing is still ongoing. No issues so far.

Thanks,

-Rong

On Wed, Nov 25, 2015 at 12:21 PM, Sean Silva <chisophugis at gmail.com> wrote:

>
>
> On Tue, Nov 24, 2015 at 8:50 PM, Rong Xu <xur at google.com> wrote:
>
>>
>>
>> On Tue, Nov 24, 2015 at 4:46 PM, Xinliang David Li <davidxl at google.com>
>> wrote:
>>
>>>
>>>
>>> On Tue, Nov 24, 2015 at 4:20 PM, Sean Silva <chisophugis at gmail.com>
>>> wrote:
>>>
>>>>
>>>>
>>>> On Tue, Nov 24, 2015 at 4:07 PM, Rong Xu <xur at google.com> wrote:
>>>>
>>>>> Hi Sean,
>>>>>
>>>>> Sorry about this. I revert the change a moment ago.
>>>>>
>>>>> As for the test. Here is the way I want to fix:
>>>>> In profile-generate tests, I will test (1) number of counters inserted
>>>>> in each function, and (2) If there is a instrumentation in a specific bb. I
>>>>> will skip the count index.
>>>>>
>>>>> In profile-use, I will invoke a new pass branch-prob that computes the
>>>>> branch probability.  I will check the branch probability rather the branch
>>>>> weight metadata.
>>>>>
>>>>> Let me know if you think this is reasonable, or you have a better way
>>>>> to do the test.
>>>>>
>>>>
>>>> You can use FileCheck variable captures (
>>>> http://llvm.org/docs/CommandGuide/FileCheck.html#filecheck-variables)
>>>> to match up the indexes (see the example where it captures a register
>>>> assignment and checks that a later instruction uses that register).
>>>> Together with good use of CHECK lines, you should be able to capture
>>>> exactly what you need and make the test very readable.
>>>>
>>>> The profile-use pass only adds metadata, so we should be checking the
>>>> metadata. We should not need to invoke branch-prob to test the behavior.
>>>>
>>>> Also, Hal's point is worth investigating: why is this pass not
>>>> deterministic across platforms? LLVM code should have deterministic output
>>>> independent of the host platform.
>>>>
>>>
>>> One thing I noticed is that std::stable_sort should be probably be used
>>> instead of std::sort.
>>>
>>
>> It seems all the test failures are caused by std::sort.
>> I used darwin to verify. Once using stable_sort, the instrumentation
>> order became
>> identical in two platforms and all the test turns green.
>>
>
> Great, could you open up the review again with the revised patch? (or
> create a new review if Phab does not support reopening)
>
> -- Sean Silva
>
>
>>
>> -Rong
>>
>>
>>>
>>> Another thing is that the dumping traverses a map indexed by address --
>>> this can result in different order on different host -- but the test cases
>>> do not depend on that (debug dump).
>>>
>>> david
>>>
>>>
>>>
>>>>
>>>> -- Sean Silva
>>>>
>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> -Rong
>>>>>
>>>>>
>>>>>
>>>>> On Tue, Nov 24, 2015 at 3:16 PM, Sean Silva <chisophugis at gmail.com>
>>>>> wrote:
>>>>>
>>>>>> Hi Rong,
>>>>>>
>>>>>> Typically, when there have been multiple active reviewers, it is
>>>>>> common courtesy to wait for all of them to LGTM the patch.
>>>>>>
>>>>>> I had further comments on the testing here (which the bots seem to
>>>>>> have caught you on anyway), so I recommend reverting this patch for the
>>>>>> moment and reopening the review.
>>>>>>
>>>>>> -- Sean Silva
>>>>>>
>>>>>>
>>>>>> On Tue, Nov 24, 2015 at 1:34 PM, Rong Xu <xur at google.com> wrote:
>>>>>>
>>>>>>> This revision was automatically updated to reflect the committed
>>>>>>> changes.
>>>>>>> Closed by commit rL254021: [PGO] MST based PGO instrumentation
>>>>>>> infrastructure (authored by xur).
>>>>>>>
>>>>>>> Changed prior to commit:
>>>>>>>   http://reviews.llvm.org/D12781?vs=41062&id=41079#toc
>>>>>>>
>>>>>>> Repository:
>>>>>>>   rL LLVM
>>>>>>>
>>>>>>> http://reviews.llvm.org/D12781
>>>>>>>
>>>>>>> Files:
>>>>>>>   llvm/trunk/include/llvm/IR/DiagnosticInfo.h
>>>>>>>   llvm/trunk/include/llvm/InitializePasses.h
>>>>>>>   llvm/trunk/include/llvm/LinkAllPasses.h
>>>>>>>   llvm/trunk/include/llvm/Transforms/Instrumentation.h
>>>>>>>   llvm/trunk/lib/IR/DiagnosticInfo.cpp
>>>>>>>   llvm/trunk/lib/Transforms/IPO/LLVMBuild.txt
>>>>>>>   llvm/trunk/lib/Transforms/Instrumentation/CFGMST.h
>>>>>>>   llvm/trunk/lib/Transforms/Instrumentation/CMakeLists.txt
>>>>>>>   llvm/trunk/lib/Transforms/Instrumentation/Instrumentation.cpp
>>>>>>>   llvm/trunk/lib/Transforms/Instrumentation/LLVMBuild.txt
>>>>>>>   llvm/trunk/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
>>>>>>>   llvm/trunk/test/Transforms/PGOProfile/Inputs/branch1.proftext
>>>>>>>   llvm/trunk/test/Transforms/PGOProfile/Inputs/branch2.proftext
>>>>>>>   llvm/trunk/test/Transforms/PGOProfile/Inputs/criticaledge.proftext
>>>>>>>   llvm/trunk/test/Transforms/PGOProfile/Inputs/landingpad.proftext
>>>>>>>   llvm/trunk/test/Transforms/PGOProfile/Inputs/loop1.proftext
>>>>>>>   llvm/trunk/test/Transforms/PGOProfile/Inputs/loop2.proftext
>>>>>>>   llvm/trunk/test/Transforms/PGOProfile/Inputs/loop3.proftext
>>>>>>>   llvm/trunk/test/Transforms/PGOProfile/Inputs/single_bb.proftext
>>>>>>>   llvm/trunk/test/Transforms/PGOProfile/Inputs/switch.proftext
>>>>>>>   llvm/trunk/test/Transforms/PGOProfile/branch1_gen.ll
>>>>>>>   llvm/trunk/test/Transforms/PGOProfile/branch1_use.ll
>>>>>>>   llvm/trunk/test/Transforms/PGOProfile/branch2_gen.ll
>>>>>>>   llvm/trunk/test/Transforms/PGOProfile/branch2_use.ll
>>>>>>>   llvm/trunk/test/Transforms/PGOProfile/checksum_mismatch.ll
>>>>>>>   llvm/trunk/test/Transforms/PGOProfile/criticaledge_gen.ll
>>>>>>>   llvm/trunk/test/Transforms/PGOProfile/criticaledge_use.ll
>>>>>>>   llvm/trunk/test/Transforms/PGOProfile/landingpad_gen.ll
>>>>>>>   llvm/trunk/test/Transforms/PGOProfile/landingpad_use.ll
>>>>>>>   llvm/trunk/test/Transforms/PGOProfile/loop1_gen.ll
>>>>>>>   llvm/trunk/test/Transforms/PGOProfile/loop1_use.ll
>>>>>>>   llvm/trunk/test/Transforms/PGOProfile/loop2_gen.ll
>>>>>>>   llvm/trunk/test/Transforms/PGOProfile/loop2_use.ll
>>>>>>>   llvm/trunk/test/Transforms/PGOProfile/loop3_gen.ll
>>>>>>>   llvm/trunk/test/Transforms/PGOProfile/loop3_use.ll
>>>>>>>   llvm/trunk/test/Transforms/PGOProfile/noprofile_use.ll
>>>>>>>   llvm/trunk/test/Transforms/PGOProfile/single_bb_gen.ll
>>>>>>>   llvm/trunk/test/Transforms/PGOProfile/switch_gen.ll
>>>>>>>   llvm/trunk/test/Transforms/PGOProfile/switch_use.ll
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151125/b3b28626/attachment.html>