[PATCH] D55585: RFC: [LLD][COFF] Parallel GHASH generation at link-time

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

In D55585#1354878 <https://reviews.llvm.org/D55585#1354878>, @aganea wrote:

> To the light of all this, does it still makes sense to compute and emit GHASH streams in clang? I'm pretty sure that the cost for serialization and I/O for those streams would be much higher that just computing GHASHes on-the-fly in the LLD. The only marginal benefit would be for incrementally linking, however even that is debatable.


I'd definitely consider it. At the very least, with this data, I don't think it ever makes sense to enable -gcodeview-ghash by default in clang-cl. Looking at your data, it looks like pre-computing hashes into the obj would still improve link time by 12-20%, and our measurements show it costs 6% object file size. It seems like it could still be worth it.

> If you have no major concerns over all this, I'll start sending smaller patches.

Looking forward to them. :)



================
Comment at: lld/trunk/COFF/Driver.cpp:608-613
+  if (ArgL == "md5")
+    Hasher = codeview::GloballyHashedType::HashType::MD5;
+  else if (ArgL == "sha1")
+    Hasher = codeview::GloballyHashedType::HashType::SHA1;
+  else if (ArgL == "cityhash")
+    Hasher = codeview::GloballyHashedType::HashType::CityHash;
----------------
ruiu wrote:
> What is the point of making it selectable to users? It feels to me that you should pick up the one that you think the best and just use it.
I think it's important to at least have the flexibility for developers to test different hashing schemes. We don't have to document all these flags. Users don't have to know about them, and we can remove them later.


================
Comment at: lld/trunk/COFF/PDB.cpp:936
+    return computeTypeHashes(cast<PCHDependency>(File)->refObj(), Stream.Types);
+  } break;
+  case UsingPCH: {
----------------
ruiu wrote:
> Formatting.
@ruiu This is still a proof of concept patch mostly to express ideas, not quite ready for review.
@aganea There is a bunch of unreachable, break-after-return in this switch that we won't want in the long run.


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

https://reviews.llvm.org/D55585