[PATCH] Refactor bitcode reader to simplify control.

[Jan Voung]

A couple nits for now, but still trying to read through and understand what's happening in the streaming and lazy cases...


================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:235
@@ +234,3 @@
+    AtTopLevel,    // Processing top-level records.
+    InsideModule,  // processing records inside a module block.
+    // All states below here represent cases where input shouldn't be parsed.
----------------
be consistent about capitalization in comments

one line starts with "parsed input" and another line starts with "Parsed input" =)

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:257
@@ -228,3 +256,3 @@
 
-  explicit BitcodeReader(MemoryBuffer *buffer, LLVMContext &C,
+  explicit BitcodeReader(MemoryBuffer *Buffer, LLVMContext &C,
                          DiagnosticHandlerFunction DiagnosticHandler);
----------------
Don't need explicit anymore, though that transition was a while back so not really related to this CL.

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:275
@@ -246,2 +274,3 @@
 
-  /// @brief Main interface to parsing a bitcode buffer.
+  /// @brief Starts an incremental parse for module M. Reads enough to
+  /// define global values. The flags define what should happen before
----------------
http://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments

says "\brief" instead of @brief

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:283
@@ +282,3 @@
+  /// @returns true if an error occurred.
+  std::error_code StartParse(Module *M,
+                             bool MaterializeAll,
----------------
lowercase first letter of function name -- should probably do it for these new functions since you're touching it (but leave existing functions alone?)

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:284
@@ +283,3 @@
+  std::error_code StartParse(Module *M,
+                             bool MaterializeAll,
+                             bool ShouldLazyLoadMetadata = false);
----------------
Variable name is different from comment "MaterializeAll" vs "ShouldMaterializeAll" -- make them the same? I see that in the actual definition you're trying to avoid conflicting with the field name...

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:288
@@ -248,2 +287,3 @@
+  /// @brief Parses bitcode. Materializes based on flags.
   /// @returns true if an error occurred.
   std::error_code ParseBitcodeInto(Module *M,
----------------
\returns

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:397
@@ +396,3 @@
+  // comes first.
+  std::error_code ContinueParse();
+
----------------
lowercase first letter for new function (and I guess update the commit message if you do)

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:401
@@ +400,3 @@
+  // StartParse().
+  std::error_code FinishParse();
+
----------------
same

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:407
@@ +406,3 @@
+  // ContinueParse().
+  std::error_code &updateParseState (BitcodeReaderState NewValue,
+                                     std::error_code &EC) {
----------------
extra space in between updateParseState and (

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:409
@@ +408,3 @@
+                                     std::error_code &EC) {
+    ParseState = EC ? ParseError : NewValue;
+    NextUnreadBit = Stream.GetCurrentBitNo();
----------------
In the review, I've tried to look for where ParseState gets set, and there are various ways to grep for that...

one is ParseState = X... another is updateParseState(Y, ...), or updateParseState(Z);

Is there any way you can make the number of variations smaller?

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:424
@@ +423,3 @@
+
+  std::error_code updateParseState() {
+    NextUnreadBit = Stream.GetCurrentBitNo();
----------------
This is a bit weird to me... you have a bunch of function called "updateParseState" but some variants modify ParseState and some variants don't.

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:2752
@@ -2751,3 +2827,1 @@
-        if (LazyStreamer && SeenValueSymbolTable) {
-          NextUnreadBit = Stream.GetCurrentBitNo();
           return std::error_code();
----------------
NextUnreadBit is no longer set -- wanted to check if that is okay now (and why)?

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:2823
@@ +2822,3 @@
+        // Suspend parsing when we reach a function body, assuming we
+        // have already associated names with global values. NOte: If
+        // the bitcode file is old, the symbol table will be at the
----------------
NOte -> Note

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:3241
@@ +3240,3 @@
+
+  if (ShouldMaterializeAll) {
+    if (std::error_code EC = MaterializeModule(TheModule))
----------------
I don't quite understand how "ShouldMaterializeAll = false" is supposed to work for the streaming case, if this isn't checked until after:

while (ParseState < NoMoreInput) {
    if (std::error_code EC = ContinueParse()) {
      return EC;
    }
  }

How do you delay reading until materialize(GV) for streaming?

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:3245
@@ +3244,3 @@
+  } else {
+    if (std::error_code EC = materializeForwardReferencedFunctions())
+      return EC;
----------------
This used to be early, in "getLazyBitcodeModuleImpl". This looks like it is now happening late in FinishParse after the loop until NoMoreInput.

Why is this okay now? Was the early call to "materializeForwardReferencedFunctions" actually extraneous because of the call in materialize(GV), or what?

Make sure to check the lazy case with blockaddresses for computed gotos, if there isn't already a unittest for that.

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:4473
@@ -4331,3 +4472,3 @@
   // blockaddresses.
-  return materializeForwardReferencedFunctions();
+  return  materializeForwardReferencedFunctions();
 }
----------------
no need for extra space

http://reviews.llvm.org/D8786

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/