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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
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]);
----------------
tejohnson wrote:
> Do we know if it is distinct or not?
The "IsDistinct" used to decide for placeholder is referring to the parent of the metadata we're about to load. NamedMD are always "distinct" AFAIK. 
The issue however is that placeholder works only with MDNode which NamedMD are not. So while conceptually possible it requires some deeper change (I'll update the comment, I wrote it before figuring out the limitation)


================
Comment at: llvm/lib/Bitcode/Reader/MetadataLoader.cpp:771
+    auto *N = dyn_cast_or_null<MDNode>(MD);
+    if (!N || !N->isTemporary())
+      return;
----------------
tejohnson wrote:
> 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?
That was the intent but I don't think it is useful, it is supposed to be already checked at call sites. So I replaced it with an assertion.


================
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");
----------------
tejohnson wrote:
> 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?
I removed the "ModuleLevel" parameter from `parseOneMetadata`, it was more useful originally but no longer.


https://reviews.llvm.org/D28113