[llvm-commits] [PATCH] RFC: fix some problems in GeneralHash

[Jay Foad]

On 22 February 2012 12:27, Chandler Carruth <chandlerc at google.com> wrote:
> Please leave these inline. They seem quite good candidates for inlining.
> Feel free to define them out of the class body, but still in the header
> file...

Fair enough. (I suppose my complaint is that GCC seemed to be inlining
too much, but that's not a good reason to change the source code.)

>> It also simplifies and combines addAligned and addUnaligned into a
>> single inline function addBytes that covers all cases, and relies on
>> the compiler to optimise away the calls to memcpy. This mostly seems
>> to work, except that GCC doesn't seem to be clever enough to optimise
>> away the whole of:
>>
>>    if (I != E) {
>>      Data = 0;
>>      std::memcpy(&Data, I, E - I);
>>      mix(Data);
>>    }
>>
>> ... when addBits is called with a type which is known to be at least
>>
>> 4-aligned. Can I change the code a bit to help the compiler out?
>
>
> No, the compiler should handle this.

I think maybe it can't remove the "if (I != E)" because it has to cope
with the possibility that I > E. But I'm not sure, and changing the
condition to "if (I < E)" didn't seem to help.

> +    // Use memcpy to work around strict aliasing rules.
>
> Not work around, this is simply required by the standard... I don't think
> you need this comment at all actually.

Well, I didn't say it was working around a *bug*, it's just working
around some rules in the standard that got in the way of a more
obvious coding.

>    // Add a range of bits from I to E.
>    template<typename T>
>    void addBits(const T *I, const T *E) {
> -    addBitsImpl<T, AlignOf<T>::Alignment_GreaterEqual_4Bytes>::add(*this,
> I, E);
> +    addBytes(reinterpret_cast<const char *>(I),
> +             reinterpret_cast<const char *>(E));
>
> Can we get rid of these reinterpret casts and retain the exact type all the
> way through? I don't see any real benefit to changing the types, and I think
> it'll make it more obvious to the reader.
>
> It may also fix some of the alignment issues by forcing an instantiation
> with a particular type pointer for which the compiler knows more about the
> alignment.

At some point I need to convert to char* so I can do byte-based
pointer arithmetic, specifically in these two lines:

+    for (; E - I >= ptrdiff_t(sizeof Data); I += sizeof Data) {

+      std::memcpy(&Data, I, E - I);

Do you have a better way of writing this?

Thanks,
Jay.