[PATCH] D121510: [Support] Introduce the BLAKE3 hashing function implementation

[Argyrios Kyrtzidis via Phabricator via llvm-commits]

akyrtzi added inline comments.


================
Comment at: llvm/include/llvm/Support/BLAKE3.h:16
+
+#include "llvm-c/blake3.h"
+#include "llvm/ADT/ArrayRef.h"
----------------
dexonsmith wrote:
> akyrtzi wrote:
> > dexonsmith wrote:
> > > Might be nice to bury this `#include` in the source file to avoid polluting the global namespace with the C stuff. Inlining would still be available for release builds (assuming they use ThinLTO).
> > Including the header is useful to avoid needing to allocate the C structures on the heap.
> Good point.
> 
> Could separate it out (put a big enough aligned char buffer in the header), but maybe not worth it.
> 
> Also might be worth considering making the stack allocation optional. It's clocking in at 1912B, which is pretty big. Some users might want it on the heap.
> 
> That'd result in something like:
> ```
> lang=c++
> // Heap by default.
> class BLAKE3 {
> public:
>   BLAKE3() = default;
> 
> protected:
>   struct StorageT { alignas(alignof(uint64_t)) char Impl[1912]; };
>   BLAKE3(StorageT &Storage); // for SmallBLAKE3.
> 
> private:
>   // Allocate and init Hasher/OwnedHasher on first use.
>   llvm_blake3_hasher &getOrCreate();
> 
>   llvm_blake3_hasher *Hasher = nullptr;
>   std::unique_ptr<StorageT> OwnedHasher;
> };
> 
> // Stack.
> class SmallBLAKE3 : public BLAKE3 {
> public:
>   SmallBLAKE3() : BLAKE3(Storage) {}
> 
> private:
>   StorageT Storage;
> };
> 
> // BLAKE3.cpp
> #include <llvm-c/blake3.h>
> BLAKE3::BLAKE3(StorageT &Storage) {
>   static_assert(sizeof(Storage.Impl) >= sizeof(llvm_blake3_hasher));
>   static_assert(alignof(Storage.Impl) >= alignof(llvm_blake3_hasher));
>   Hasher = reinterpret_cast<llvm_blake3_hasher *>(Storage.Impl);
>   llvm_blake3_init(Hasher);
> }
> ```
> 
> But we can do one/both of those later if it's important.
> Also might be worth considering making the stack allocation optional. It's clocking in at 1912B, which is pretty big. Some users might want it on the heap.

If you want it on the heap you can do `new BLAKE3()`. I think it's better to have it on the stack by default and keep it simple with just one class that you need to be aware of, and, if necessary, put it on heap explicitly.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121510/new/

https://reviews.llvm.org/D121510