[PATCH] D12781: PGO IR-level instrumentation infrastructure

Tue Nov 24 19:27:10 PST 2015

On Tue, Nov 24, 2015 at 7:18 PM, Rong Xu <xur at google.com> wrote:

> The checksum mismatch reported by Hal is a bug in my code.
>
> 272       FunctionHash = MST.AllEdges.size() << 32 | JC.getCRC();
>
> Here in x86-64, vector<...>::size_type is "unsigned long", So the assignment is OK.
>
> While in m32 host, vector<...>::size_type is unsigned int.
>
> (gdb) ptype std::vector<std::unique_ptr<(anonymous namespace)::PGOEdge, std::default_delete<(anonymous namespace)::PGOEdge> >, std::allocator<std::unique_ptr<(
> anonymous namespace)::PGOEdge, std::default_delete<(anonymous namespace)::PGOEdge> > > >::size_type
> type = unsigned int
>
> The shift becomes a null op.
>
> More precise, it's modulo. Anyway, it's not what I expected.

-Rong

> -Rong
>
>
> 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.
>>
>> 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/20151124/5cb5fb86/attachment.html>