[PATCH] D18415: [LTO] Basic support for internalize.

[Rui Ueyama via llvm-commits]

ruiu added a subscriber: ruiu.

================
Comment at: ELF/LTO.cpp:104
@@ -97,1 +103,3 @@
           }
+          // FIXME: Is the check for shared libraries correct?
+          if (!Config->Shared && !Used.count(GV) && !B->isUsedInRegularObj())
----------------
I don't want to sprinkle FIXMEs to new patches because it wouldn't help readers but instead asking a question to readers. If you are not sure what the correct thing to do but decided to go with some code, then please justify your decision in comment, like

  // We are not sure what to do if we are creating a shared object file.
  // For now, we do blah blah because blah blah.

================
Comment at: ELF/LTO.cpp:127-131
@@ +126,7 @@
+
+  for (const auto &Name : Internalize) {
+    GlobalValue *GV = Combined.getNamedValue(Name.first());
+    if (GV)
+      internalize(*GV);
+  }
+
----------------
  for (const auto &Name : Internalize)
    if (GlobalValue *GV = ...)
      internalize(*GV);

Also, please do not add a blank line at beginning of a function.

================
Comment at: ELF/LTO.h:48
@@ -46,2 +47,3 @@
   std::unique_ptr<MemoryBuffer> MB;
+  llvm::StringSet<> Internalize;
 };
----------------
StringSet copies strings to its internal buffer. Is this what you want?

"Internalize" is a verb. I think you want to name it a noun.

================
Comment at: ELF/SymbolTable.cpp:217
@@ -216,1 +216,3 @@
 
+  // Needed for internalize step of symbols.
+  if (auto *BC = dyn_cast<DefinedBitcode>(Existing))
----------------
This comment needs to be more descriptive for those who don't understand what is going on in LTO.


http://reviews.llvm.org/D18415