[PATCH] D68570: Unify the two CRC implementations

Mon Oct 7 10:06:09 PDT 2019

rupprecht added a comment.

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?

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.

That said, general +1 to removing code complexity/duplication

================
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,
----------------
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?

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

https://reviews.llvm.org/D68570