[PATCH] D17066: libLTO: add a ThinLTOCodeGenerator on the model of LTOCodeGenerator.

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

Mostly comment typo and wording suggestions


================
Comment at: include/llvm-c/lto.h:581
@@ +580,3 @@
+/**
+ * Frees all code generator and all memory it internally allocated.
+ * Upon return the lto_code_gen_t is no longer valid.
----------------
"Frees the code generator..."

================
Comment at: include/llvm-c/lto.h:590
@@ +589,3 @@
+ * Add a module to a ThinLTO code generator. Identifier has to be unique among
+ * all the modules in a code generator. The data buffer stay owned by the
+ * client, and is expected to be available for the entire lifetime of the
----------------
typo s/stay/stays/

================
Comment at: include/llvm-c/lto.h:600
@@ +599,3 @@
+extern void thinlto_codegen_add_module(thinlto_code_gen_t cg,
+                                       const char *Identifier, const char *Data,
+                                       int Length);
----------------
Inconsistent capitalization. Some parameters here and in other interfaces are lower case, some here and other places start with upper case.

================
Comment at: include/llvm-c/lto.h:614
@@ +613,3 @@
+ *
+ * It usually match the number of input files, but this is not a guarantee of
+ * the API and future implementation may change, so the client should not
----------------
s/match/matches/

================
Comment at: include/llvm-c/lto.h:615
@@ +614,3 @@
+ * It usually match the number of input files, but this is not a guarantee of
+ * the API and future implementation may change, so the client should not
+ * assume it.
----------------
maybe "and in future implementations may change" or "and may change in future implementations"

================
Comment at: include/llvm-c/lto.h:652
@@ +651,3 @@
+ * Sets the path to a directory to use as a storage for temporary bitcode files.
+ * The intention is to make available for debugging the bitcode files after
+ * optimizations and before the CodeGen.
----------------
Maybe "The intention is to make the bitcode files available for debugging after..."

================
Comment at: include/llvm/LTO/ThinLTOCodeGenerator.h:54
@@ +53,3 @@
+   * code. If a symbol is not listed there, it might be inlined into every usage
+   * and optimized away.
+   */
----------------
Think this comment is clearer if the last sentence changed to "If a symbol is not listed there, it will be optimized away if it is inlined into every usage."

================
Comment at: include/llvm/LTO/ThinLTOCodeGenerator.h:11
@@ +10,3 @@
+// This file declares the ThinLTOCodeGenerator class, similar to the
+// LTOCodeGenerator but for the ThinLTO scheme. It provide an interface for
+// linker plugin.
----------------
s/provide/provides/

================
Comment at: include/llvm/LTO/ThinLTOCodeGenerator.h:44
@@ +43,3 @@
+
+/// This class define an interface similar to the LTOCodeGenerator, but adapted
+/// for ThinLTO processing.
----------------
s/provide/provides/

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:149
@@ +148,3 @@
+  if (!SaveTempsDir.empty()) {
+    // User asked to save temps, let dump the bitcode file after import.
+    auto SaveTempPath = SaveTempsDir + llvm::utostr(count) + ".imported.bc";
----------------
s/let/let's/ or s/let/so/

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:150
@@ +149,3 @@
+    // User asked to save temps, let dump the bitcode file after import.
+    auto SaveTempPath = SaveTempsDir + llvm::utostr(count) + ".imported.bc";
+    std::error_code EC;
----------------
This will be hard to correlate with the original file. How about using some part of the source file name saved in the module here and in the later opt.bc dump? I.e. use the base name of the path returned by getSourceFileName(), but still add in 'count' to avoid conflicts when the same file name is used at multiple paths.

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:208
@@ +207,3 @@
+    if (!SaveTempsDir.empty()) {
+      // User asked to save temps, let dump the optimized bitcode file.
+      // Write to a temporary to avoid race condition
----------------
s/let/let's/ or s/let/so/

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:226
@@ +225,3 @@
+
+    // Run optimize and codegen, possible dumping optimized bitcode at the same
+    // time. The resulting binary is in OutputBuffer.
----------------
s/possible/possibly/

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:482
@@ +481,3 @@
+  // Prepare the resulting object vector
+  ProducedBinaries.clear();
+  ProducedBinaries.resize(Modules.size());
----------------
Do we expect to reuse the same code generator multiple times?


http://reviews.llvm.org/D17066