[PATCH] D14825: [ThinLTO] Add MODULE_CODE_METADATA_VALUES record

[Mehdi AMINI via llvm-commits]

joker.eph added a comment.

Looks good overall, I need to do a second pass later, probably this afternoon.


================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:157
@@ -156,1 +156,3 @@
   std::string ProducerIdentification;
+  unsigned NumModuleMDs = 0;
+  // Support older bitcode without this record.
----------------
One line comment for `NumModuleMDs`?

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:158
@@ +157,3 @@
+  unsigned NumModuleMDs = 0;
+  // Support older bitcode without this record.
+  bool SeenModuleValuesRecord = false;
----------------
I suggest to substitute "this" with "the MODULE_CODE_METADATA_VALUES"

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:1922
@@ -1916,1 +1921,3 @@
   unsigned NextMDValueNo = MDValueList.size();
+  if (ModuleLevel && SeenModuleValuesRecord) {
+    // Now that we are parsing the module level metadata, we want to restart
----------------
Is it possible to have `NextMDValueNo != 0` and `SeenModuleValuesRecord`? It there some redundancy here?

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:3292
@@ -3269,3 +3291,3 @@
         break;
-      case bitc::METADATA_BLOCK_ID:
+      case bitc::METADATA_BLOCK_ID: {
         if (ShouldLazyLoadMetadata && !IsMetadataMaterialized) {
----------------
This change does not seem necessary here

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:5351
@@ -5301,2 +5350,3 @@
   }
+
   // At this point, if there are any function bodies, parse the rest of
----------------
remove


http://reviews.llvm.org/D14825