[PATCH] D28113: Use lazy-loading of Metadata in MetadataLoader when importing is enabled (NFC)

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

I need to read it through again, but a few comments/questions based on my first pass.

Nit: since "data" is plural I also tend to think of "metadata" as being plural (as well as singular as "data" is commonly used as a singular today). So personally I prefer metadata over metadatas. I noticed a lot of places in the patch where "metadatas" is used.



================
Comment at: llvm/lib/Bitcode/Reader/MetadataLoader.cpp:95
+STATISTIC(NumMDNodeTemporary, "Number of MDNode::Temporary created");
+STATISTIC(NumMDRecordLoaded, "Number of Metadata records loaded");
+
----------------
Can there be a test added based on these strings and the option added below to verify e.g. that the number of records loaded is lower in the lazy-loading case?


================
Comment at: llvm/lib/Bitcode/Reader/MetadataLoader.cpp:414
 
+  /// Cursor associated to the lazy-loading of Metadatas. This is the easy way
+  /// to keep around the right "context" (Abbrev list) to be able to jump in
----------------
s/associated to/associated with/


================
Comment at: llvm/lib/Bitcode/Reader/MetadataLoader.cpp:624
+        // FIXME: we need to do this early because we don't materialize global
+        // value expliticitly.
+        IndexCursor.JumpToBit(CurrentPos);
----------------
s/expliticitly/explicitly/


================
Comment at: llvm/lib/Bitcode/Reader/MetadataLoader.cpp:625
+        // value expliticitly.
+        IndexCursor.JumpToBit(CurrentPos);
+        Record.clear();
----------------
Should NumMDRecordLoaded be incremented in this case too? Maybe it would be better to CSE the increment of that down into a single place at the end of the BitstreamEntry::Record block (i.e. after the switch on Code, so after any case that doesn't cause an early return)?


================
Comment at: llvm/lib/Bitcode/Reader/MetadataLoader.cpp:671
+      case bitc::METADATA_GLOBAL_VAR_EXPR:
+        // We don't expect to see any of these, if we see one, give on
+        // lazy-loading and fallback.
----------------
s/give on/give up on/


================
Comment at: llvm/lib/Bitcode/Reader/MetadataLoader.cpp:1501
+
+Error MetadataLoader::MetadataLoaderImpl::indexMetadataStrings(
+    ArrayRef<uint64_t> Record, StringRef Blob) {
----------------
This is largely the same as the code in parseMetadataStrings. Could it be merged with a callback to do the right thing with each resulting StringRef?

Also, since we are essentially always parsing all the strings, what is the cost of always adding them to the MetadataList as MDStrings? Is this a memory savings?


================
Comment at: llvm/lib/Transforms/IPO/FunctionImport.cpp:700
 
+    // Upgrade debug info after we're done materialize all the globals and we
+    // have loaded all the metadatas!
----------------
s/materialize/materializing/ and maybe "all the required globals"?


================
Comment at: llvm/lib/Transforms/IPO/FunctionImport.cpp:701
+    // Upgrade debug info after we're done materialize all the globals and we
+    // have loaded all the metadatas!
+    UpgradeDebugInfo(*SrcModule);
----------------
Maybe "all the required metadata" (since not all metadata are loaded now)?


https://reviews.llvm.org/D28113