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

[Teresa Johnson via llvm-commits]

tejohnson marked 11 inline comments as done.
tejohnson added a comment.

Responses and some other comments below. New patch being uploaded shortly.


================
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,
----------------
davidxl wrote:
> Should it really be 'readThinLTOFunctionSummary'?
Yes, changed.

================
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);
----------------
davidxl wrote:
> parseBitcodeInto sounds too generic. Perhaps parseSummaryIndex?
Since it mirrors BitcodeReader::parseBitcodeInto, I will change this to parseSummaryIndexInto.

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:427
@@ +426,3 @@
+                                   ThinLTOFunctionSummaryIndex *I);
+  std::error_code parseFunction(std::unique_ptr<DataStreamer> Streamer,
+                                ThinLTOFunctionSummaryIndex *I,
----------------
davidxl wrote:
> Is it better to call it  'parseFunctionSummary'? Also move this interface decl after parseThinLTOSummary() below to show their order in the parsing hierarchy.
Changed the name. Can't move after parseThinLTOSummary since this one needs to be public and I am making the other one private (see below). Instead added a comment about it being used to read a function summary entry lazily.

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:431
@@ +430,3 @@
+
+  std::error_code parseThinLTOBlock();
+  std::error_code parseThinLTOSymtab();
----------------
davidxl wrote:
> Do these parsing interfaces need to be public, other than the top level parseBitcodeInto and the lazy reading parseFunction?
Moved all of these parseThinLTO* routines into the private section.

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:433
@@ +432,3 @@
+  std::error_code parseThinLTOSymtab();
+  std::error_code parseThinLTOSummary();
+  std::error_code parseThinLTOModuleStringTable();
----------------
davidxl wrote:
> Is it clearer to call it
> 
> parseThinLTOSummaries or parseThinLTOSummaryBlock?
Changed to parseEntireThinLTOSummary.

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:437
@@ +436,3 @@
+private:
+  std::error_code parseModule();
+  std::error_code parseValueSymbolTable();
----------------
davidxl wrote:
> 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.
I prefer to keep the current division as it mirrors how parseBitcodeInto/parseModule are structured in the normal BilcodeReader.

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

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:5354
@@ +5353,3 @@
+static ErrorOr<std::unique_ptr<ThinLTOFunctionSummaryIndex>>
+getThinLTOIndexImpl(std::unique_ptr<MemoryBuffer> &&Buffer,
+                    LLVMContext &Context,
----------------
Inlined into getThinLTOIndex since that was the only caller.

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:5389
@@ +5388,3 @@
+
+static bool hasThinLTOIndexImpl(std::unique_ptr<MemoryBuffer> &&Buffer,
+                                LLVMContext &Context,
----------------
Inlined into hasThinLTOIndex since that was the only caller.

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:5415
@@ +5414,3 @@
+
+static std::error_code readThinLTOFunctionInfoImpl(
+    std::unique_ptr<MemoryBuffer> &&Buffer, LLVMContext &Context,
----------------
Inlined into readThinLTOFunctionInfo since that was the only caller.

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:5428
@@ +5427,3 @@
+
+  // TODO: If we have a FunctionName, then we have the bitcode offset
+  // of it's summary within the Buffer saved in the ThinLTOFunctionSummary
----------------
I have replaced this TODO with the code.


http://reviews.llvm.org/D11722