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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: llvm/lib/Bitcode/Reader/MetadataLoader.cpp:468
+
+  // Upgrade old-style CU <-> SP pointers to point from SP to CU.
+  void upgradeCUSubprograms() {
----------------
Use "///" instead for doxygen


================
Comment at: llvm/lib/Bitcode/Reader/MetadataLoader.cpp:615
+        for (unsigned i = 0; i != Size; ++i) {
+          // FIXME: could we use a placeholder here?
+          MDNode *MD = MetadataList.getMDNodeFwdRefOrNull(Record[i]);
----------------
Do we know if it is distinct or not?


================
Comment at: llvm/lib/Bitcode/Reader/MetadataLoader.cpp:771
+    auto *N = dyn_cast_or_null<MDNode>(MD);
+    if (!N || !N->isTemporary())
+      return;
----------------
I had to parse this condition a few times - might be worth a comment. Basically it is looking to see if we have left a temporary, in which case we need to parse the metadata record, otherwise we have a valid node of some kind, right?


================
Comment at: llvm/lib/Bitcode/Reader/MetadataLoader.cpp:782
+  unsigned Code = IndexCursor.readRecord(Entry.ID, Record, &Blob);
+  if (Error Err = parseOneMetadata(Record, Code, Placeholders, Blob, false, ID))
+    report_fatal_error("Can't lazyload MD");
----------------
Maybe add "/* ModuleLevel */" in front of "false" param. But why is this hardcoded to false? Aren't we module level if we are lazy loading?


================
Comment at: llvm/lib/Bitcode/Reader/MetadataLoader.cpp:787
+// Flush forward references
+void MetadataLoader::MetadataLoaderImpl::flush(PlaceholderQueue &Placeholders) {
+  DenseSet<unsigned> Temporaries;
----------------
>From the name and the param, it sounds like this routine is only flushing the placeholders. Suggest renaming to something like resolveForwardRefsAndPlaceholders?


https://reviews.llvm.org/D28113