[PATCH] D68570: Unify the two CRC implementations

[Hans Wennborg via Phabricator via llvm-commits]

hans marked 5 inline comments as done.
hans added a comment.

In D68570#1697872 <https://reviews.llvm.org/D68570#1697872>, @rupprecht wrote:

> In D68570#1697633 <https://reviews.llvm.org/D68570#1697633>, @hans wrote:
>
> > In D68570#1697588 <https://reviews.llvm.org/D68570#1697588>, @joerg wrote:
> >
> > > Why go back to the large tables for crc32? Just because JamCRC had that bug doesn't mean it should persist.
> >
> >
> > Because just using the table is much simpler and we already have it: no need for any run-time initialization and fancy code like call_once. Why do you consider it a bug? Generating a constant table like this at run-time -- again and again for each invocation of the program -- seems less than ideal to me.
>
>
> Do you have any benchmarks?


Of what? That generating the table is slower than just using it directly? I'd say no benchmark is needed to conclude that :-)

> A table is simpler in some regards, but also less readable in another sense (what are these random hex values?). Having benchmark results helps settle that debate.

It's the standard table for the CRC-32 polynomial. I'll add a comment with some references.

In D68570#1697933 <https://reviews.llvm.org/D68570#1697933>, @thakis wrote:

> Also, in practice most clients will build against zlib and not see the tables. +1 to the current approach :)


Windows builds will typically not use zlib though, so this code does get used :-)

In D68570#1697940 <https://reviews.llvm.org/D68570#1697940>, @mgorny wrote:

> I'd personally prefer either the non-table approach or having the tables generated at build time. Given this is only going to be used rarely, I don't think we should clutter the code with big tables.


It's not used rarely, it's used all the time on Windows. Generating the table at build time would be nice in theory, but writing a tablegen and hooking it up in the build system seems like overkill for this.



================
Comment at: llvm/include/llvm/Support/JamCRC.h:45
+    CRC ^= 0xFFFFFFFFU; // Undo CRC-32 Init.
+    CRC = llvm::crc32(CRC, Data);
+    CRC ^= 0xFFFFFFFFU; // Undo CRC-32 XorOut.
----------------
ruiu wrote:
> This is the only place where you pass non-zero value as the first argument, and the way how that value is handled is a little irregular. So how about moving this class to CRC.h and define `llvm::crc32` as `llvm::crc32(ArrayRef<uint8_t>)`?
Actually, lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp also passes a non-zero value (from ObjectFileELF::CalculateELFNotesSegmentsCRC32).

But that's not the common case. Let's make an overload for the common case, and moving JamCRC into CRC.h makes sense too.


================
Comment at: llvm/lib/Support/CRC.cpp:26
 
-uint32_t llvm::crc32(uint32_t CRC, StringRef S) {
-  static llvm::once_flag InitFlag;
-  static CRC32Table Tbl;
-  llvm::call_once(InitFlag, initCRC32Table, &Tbl);
+static const uint32_t CRCTable[256] = {
+    0x00000000, 0x77073096, 0xee0e612c, 0x990951ba,
----------------
hiraditya wrote:
> rupprecht wrote:
> > Can you leave a comment how this table was generated/how it could be regenerated if needed in the future? And/or a unit test to assert the values are correct?
> +1
I'm adding a comment with references for how the algorithm and the table works. There is already a unit test in llvm/unittests/Support/CRCTest.cpp


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

https://reviews.llvm.org/D68570