[PATCH] D11722: [ThinLTO] Bitcode reading/writing support for ThinLTO function summary/index

[David Li via llvm-commits]

davidxl added a subscriber: davidxl.

================
Comment at: include/llvm/Bitcode/ReaderWriter.h:72
@@ +71,3 @@
+      DiagnosticHandlerFunction DiagnosticHandler,
+      bool ParseFuncSummary = true);
+
----------------
Add a comment here to make it clear that lazy summary parsing is needed for reading combined index in the function import phase.

================
Comment at: include/llvm/Bitcode/ReaderWriter.h:76
@@ +75,3 @@
+  /// the given index. This is used when we already parsed the rest of
+  /// the index with getThinLTOIndex and ParseFuncSummary=false.
+  std::error_code readThinLTOFunctionInfo(
----------------
Make the comment clearer that:

1) When reading combined index (ThinLTOSummaryIndex), getThinLTOIndex is first called with ParseFuncSummary=false
2) The method supports lazily reading Function summary data associated for the function specified by the function name.

================
Comment at: include/llvm/Bitcode/ReaderWriter.h:77
@@ +76,3 @@
+  /// the index with getThinLTOIndex and ParseFuncSummary=false.
+  std::error_code readThinLTOFunctionInfo(
+      MemoryBufferRef Buffer, LLVMContext &Context,
----------------
Should it really be 'readThinLTOFunctionSummary'?

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:425
@@ +424,3 @@
+  /// \returns true if an error occurred.
+  std::error_code parseBitcodeInto(std::unique_ptr<DataStreamer> Streamer,
+                                   ThinLTOFunctionSummaryIndex *I);
----------------
parseBitcodeInto sounds too generic. Perhaps parseSummaryIndex?

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:427
@@ +426,3 @@
+                                   ThinLTOFunctionSummaryIndex *I);
+  std::error_code parseFunction(std::unique_ptr<DataStreamer> Streamer,
+                                ThinLTOFunctionSummaryIndex *I,
----------------
Is it better to call it  'parseFunctionSummary'? Also move this interface decl after parseThinLTOSummary() below to show their order in the parsing hierarchy.

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:431
@@ +430,3 @@
+
+  std::error_code parseThinLTOBlock();
+  std::error_code parseThinLTOSymtab();
----------------
Do these parsing interfaces need to be public, other than the top level parseBitcodeInto and the lazy reading parseFunction?

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:433
@@ +432,3 @@
+  std::error_code parseThinLTOSymtab();
+  std::error_code parseThinLTOSummary();
+  std::error_code parseThinLTOModuleStringTable();
----------------
Is it clearer to call it

parseThinLTOSummaries or parseThinLTOSummaryBlock?

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:437
@@ +436,3 @@
+private:
+  std::error_code parseModule();
+  std::error_code parseValueSymbolTable();
----------------
parseBitCodeInto is a top level interface which is basically a wrapper to parseModule after seeking to the start of module block -- so it seems that parseModule can be inlined into parseBitcodeInto to avoid confusion.

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:4864
@@ +4863,3 @@
+// Specialized value symbol table parser used when reading ThinLTO
+// blocks where we don't actually create global values.
+std::error_code ThinLTOBitcodeReader::parseValueSymbolTable() {
----------------
Add comment about the purpose of the function.

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:4930
@@ +4929,3 @@
+        break;
+      case bitc::THINLTO_BLOCK_ID:
+        assert(SeenValueSymbolTable);
----------------
Reorder this case to reflect the physical order of block ids on disk?

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:5115
@@ +5114,3 @@
+  // need to understand them all.
+  while (1) {
+    if (Stream.AtEndOfStream()) {
----------------
It is more readable to have a helper function to do 'seekToModuleStart' which includes the loop, and move the parseModule outside of it.

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:5379
@@ +5378,3 @@
+// If ParseFuncSummary is true, parse the entire function summary into
+// the index, otherwise skip the function summary section.
+ErrorOr<std::unique_ptr<ThinLTOFunctionSummaryIndex>>
----------------
Suggested comment:

Otherwise skip the function summary section, but only create an index object with a map from function name to function summary offset. The index is used to perform lazy function summary reading later.

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:5434
@@ +5433,3 @@
+  // so the below will need to be a loop.
+  size_t FunctionOffset = 0; /* TODO: fill this in. */
+
----------------
FunctionSummaryOffset?


http://reviews.llvm.org/D11722