[llvm-commits] [llvm] r162799 - in /llvm/trunk: include/llvm/Analysis/Passes.h include/llvm/Analysis/ProfileDataLoader.h include/llvm/InitializePasses.h include/llvm/LinkAllPasses.h lib/Analysis/Analysis.cpp lib/Analysis/CMakeLists.txt lib/Analysis/ProfileDataLoader.cpp lib/Analysis/ProfileDataLoaderPass.cpp lib/Analysis/ProfileInfo.cpp test/Analysis/Profiling/load-branch-weights-ifs.ll test/Analysis/Profiling/load-branch-weights-loops.ll test/Analysis/Profiling/load-branch-weights-switches.ll

[Alastair Murray]

Hi Bill, All,

This is a ping for the previous patch (to use a MemoryBuffer instead of 
fread() etc within the new profiling code).

Alternatively a second implementation of exactly the same concept is 
attached.  This second implementation gets rid of the template functions 
and the pointer casts.  The use of the MemoryBuffer is managed via a 
wrapper class which tracks how many bytes have been read so have and 
performs any required endianness conversions.  It also now uses memcpy 
instead of loop based copies.

I prefer this second implementation but the resulting code is longer 
(though simpler) than with the previous patch.  Functionally they are 
identical.

I do not have commit access, so if the patch is OK then please commit it.

Regards,
Alastair.

On 01/09/12 12:06, Alastair Murray wrote:
> Hi Bill, All,
>
> Thank you for committing the previous patch.
>
> Attached is a patch to make use of MemoryBuffer instead of fopen/fread.
>   This patch should be applied _after_ pulling down Benjamin Kramer's
> changes (he already committed those in rev 162799).
>
> The one line change in ProfileDataLoaderPass.cpp within the patch is
> technically unrelated, but is obvious.  It relates to producing correct
> error messages.
>
> For the MemoryBuffer changes, there is a ReadBuffer<T> function which is
> used to read arbitary types from the MemoryBuffer.  This contains
> pointer casts, which may be unacceptable.  I've contained this aspect
> within the one function, but please let me know your thoughts as I'm
> certainly open to a cleaner solution.
>
> I do not have commit access, so if the patch is okay then please commit it.
>
> Regards,
> Alastair.
>
> On 31/08/12 01:21, Bill Wendling wrote:
>> On Aug 30, 2012, at 4:23 PM, Alastair Murray
>> <alastairmurray42 at gmail.com> wrote:
>>
>>> Hi Bill,
>>>
>>> On 30/08/12 21:24, Bill Wendling wrote:
>>>> The patch looks fine. Thank you for doing it! Let me know if you
>>>> don't have commit access and I can apply it for you.
>>>
>>> I do not have commit access, so if you could please apply it it would
>>> be appreciated!
>>>
>> Okay. Done in r162979. Thanks!
>>
>>>> If the namespace issue is messing things up, then you can leave it.
>>>> It was a minor issue. I'm still not 100% convinced about the weight
>>>> code. I doubt that you're gaining much by expanding to uint64_t and
>>>> then back to unsigned. If anything, you should probably just use
>>>> uint64_t directly in the whole function instead of casting (which,
>>>> gross):
>>>>
>>>> static uint64_t AddCounts(uint64_t A, uint64_t B) {
>>>>    // ...
>>>> }
>>>>
>>>> Would this address your concerns?
>>>
>>> Yes, I could do this.  Reading between the lines, however, I could
>>> just not saturate on overflow but leave it unhandled?  Is that
>>> preferable?
>>>
>> If you could give an example of what you're trying to do, that might
>> be better. We may not be talking about the same thing. What does the
>> math look like that you're trying to handle by going to uint64_t?
>>
>> -bw
>>
>>> I'll try and get the MemoryBuffer patch written tomorrow.
>>>
>>> Regards,
>>> Alastair.
>>
>>
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: weights_6_membuffer_ver2.patch
Type: text/x-patch
Size: 9236 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121026/5e2dd7bd/attachment.bin>