[PATCH] D87970: [ThinLTO] Avoid temporaries when loading global decl attachment metadata

[Eugene Leviant via Phabricator via llvm-commits]

evgeny777 accepted this revision.
evgeny777 added a comment.
This revision is now accepted and ready to land.

LGTM with nits



================
Comment at: llvm/lib/Bitcode/Reader/MetadataLoader.cpp:916
+      return error("Malformed block");
+    case BitstreamEntry::EndBlock: {
+      // Sanity check that we parsed them all.
----------------
nit: no need for {} here


================
Comment at: llvm/lib/Bitcode/Reader/MetadataLoader.cpp:929
+    unsigned Code = MaybeCode.get();
+    switch (Code) {
+    default:
----------------
nit: this is hard to read. May be change to somthing like this?
```
if (MaybeCode.get() != bitc::METADATA_GLOBAL_DECL_ATTACHMENT) {
      assert(NumGlobalDeclAttachSkipped == NumGlobalDeclAttachParsed);
      return true;
}

// Parse stuff here
```


================
Comment at: llvm/lib/Bitcode/Reader/MetadataLoader.cpp:2115
       return error("Invalid ID");
-    MDNode *MD = MetadataList.getMDNodeFwdRefOrNull(Record[I + 1]);
+    MDNode *MD =
+        dyn_cast_or_null<MDNode>(getMetadataFwdRefOrLoad(Record[I + 1]));
----------------
tejohnson wrote:
> evgeny777 wrote:
> > Why is this change needed?
> Because getMDNodeFwdRefOrNull unconditionally creates a forward reference with temporary when the requested metadata is not yet loaded. Whereas getMetadataFwdRefOrLoad will use the lazy loading index built by lazyLoadModuleMetadataBlock to perform the load rather than create a temporary. With the code before this patch, that could not be done as the global decl attachment metadatas were being parsed also by lazyLoadModuleMetadataBlock, so the index wasn't available. And without making the change to this callsite, the patch doesn't have any measureable effect since we won't use the index even when we have it (which I discovered when I first made the change to restructure into loadGlobalDeclAttachments but didn't change this callsite).
Thanks, now clear


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87970/new/

https://reviews.llvm.org/D87970