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

[Chris Lattner via Phabricator via llvm-commits]

lattner requested changes to this revision.
lattner added a comment.
This revision now requires changes to proceed.

thank you for working on integrating this!  Please check out how the regex stuff got integrated, I think there are learnings in it that aren't obvious (we should also document this somewhere).



================
Comment at: llvm/include/llvm-c/blake3.h:1
+#ifndef BLAKE3_H
+#define BLAKE3_H
----------------
Please add the correct license boilerplate to this file.   Plz point to the license file in the lib directory.


================
Comment at: llvm/include/llvm-c/blake3.h:41
+
+const char *blake3_version(void);
+void blake3_hasher_init(blake3_hasher *self);
----------------
How is this going to compose with apps that use LLVM and also use blake3 for their own things?  can we really take these global symbol names?  You'll note that in regex_impl.h we had to prefix the C symbols, I expect that we'll have the same challenges here.


================
Comment at: llvm/lib/Support/BLAKE3/README.md:1
+The official C implementation of BLAKE3.
+
----------------
I'd recommend dropping this file or significantly changing it, this isn't showing the preferred LLVM way to use this.


================
Comment at: llvm/lib/Support/BLAKE3/blake3.c:2
+#include <assert.h>
+#include <stdbool.h>
+#include <string.h>
----------------
Same


================
Comment at: llvm/lib/Support/BLAKE3/blake3.h:1
+#ifndef BLAKE3_H
+#define BLAKE3_H
----------------
Needs a file header, point to the license file etc.


================
Comment at: llvm/lib/Support/BLAKE3/blake3_sse41_x86-64_windows_gnu.S:1
+.intel_syntax noprefix
+.global blake3_hash_many_sse41
----------------
Are these files generated from something? Should we integrate the generator into the build instead of checking in massive .s files?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121510