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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: llvm/lib/Bitcode/Reader/MetadataLoader.cpp:894
+// their parsing until after the index is created, we can use the index
+// instead of creating temporaries.
+Expected<bool> MetadataLoader::MetadataLoaderImpl::loadGlobalDeclAttachments() {
----------------
evgeny777 wrote:
> So this is not called during ThinLTO import, is it?
It is only called during ThinLTO importing. See the if conditions for the block of code at its callsite (line 989)


================
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]));
----------------
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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87970