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

[Chandler Carruth]

On Wed, Feb 22, 2012 at 4:15 AM, Jay Foad <jay.foad at gmail.com> wrote:

> What do you think of the attached patch? It:
>
> 1. Fixes the theoretical problem with GeneralHash::addUnaligned
> returning varying results on big-endian hosts.
> 2. Fixes the strict aliasing problems that foiled my attempt to apply
> Meador's patch to improve uniquing of struct and function types.
>

This is definitely the way to go. memcpy is the only safe technique here,
and compilers should optimize it harder.


> 3. Moves GeneralHash::finish and GeneralHash::mix out of line. This is
> just a matter of taste, but the inlined code looked a bit long-winded
> to me.
>

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

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 completely agree with the decision
to let the compile deduce and figure out alignment optimizations.

A few comments on the patch:

-  template<typename T, bool isAligned>
-  struct addBitsImpl {
-    static void add(GeneralHash &Hash, const T *I, const T *E) {
-      Hash.addUnaligned(
-        reinterpret_cast<const uint8_t *>(I),
-        reinterpret_cast<const uint8_t *>(E));
+  // Add a range of bytes from I to E.
+  void addBytes(const char *I, const char *E) {
+    uint32_t Data;
+    // 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.

+    for (; E - I >= ptrdiff_t(sizeof Data); I += sizeof Data) {
+      // A clever compiler should be able to turn this memcpy into a single
+      // aligned or unaligned load (depending on the alignment of the type
T
+      // that was used in the call to addBits).
+      std::memcpy(&Data, I, sizeof Data);
+      mix(Data);
     }
-  };
-
-  template<typename T>
-  struct addBitsImpl<T, true> {
-    static void add(GeneralHash &Hash, const T *I, const T *E) {
-      Hash.addAligned(
-        reinterpret_cast<const uint32_t *>(I),
-        reinterpret_cast<const uint32_t *>(E));
+    if (I != E) {
+      Data = 0;
+      std::memcpy(&Data, I, E - I);
+      mix(Data);
     }
-  };
+  }

   // 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120222/6fea8366/attachment.html>