[PATCH] D44504: COFF: Implement string tail merging.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

Overall looking very nice.



================
Comment at: lld/COFF/Chunks.cpp:590-592
+    ArrayRef<uint8_t> Contents = C->getContents();
+    return StringRef{reinterpret_cast<const char *>(Contents.data()),
+                     Contents.size()};
----------------
I believe you can use `toStringRef(C->getContents())`.


================
Comment at: lld/COFF/Chunks.cpp:601
+  for (SectionChunk *C : Sections) {
+    if (C->isLive()) {
+      size_t Off = Builder.getOffset(Str(C));
----------------
I'd flip the condition and continue early.


================
Comment at: lld/COFF/Chunks.h:230-231
 
+// A chunk that contains SectionChunks representing string literals that may be
+// tail merged.
+struct MergeChunk : public Chunk {
----------------
I'd expand this comment a little bit to say that this is an lld-specific feature (not implemented in MSVC) to minimize the output size by finding string literals sharing tail parts and merge them. Maybe I'd explain how it work a bit too.


================
Comment at: lld/COFF/Chunks.h:232
+// tail merged.
+struct MergeChunk : public Chunk {
+  static std::map<uint32_t, MergeChunk *> Instances;
----------------
Do all members have to be public? Even that's the case, I feel comfortable if you make it not a struct but a class with no private member.


================
Comment at: lld/COFF/ICF.cpp:230
+  for (auto &P : MergeChunk::Instances)
+    for (auto *SC : P.second->Sections)
+      SC->Class[0] = NextId++;
----------------
auto -> MergeSection


================
Comment at: lld/COFF/Writer.cpp:429
 
+  for (auto &P : MergeChunk::Instances)
+    RData->addChunk(P.second);
----------------
auto -> MergeSection


https://reviews.llvm.org/D44504