[PATCH] D28423: Fix assertions on lazy-loading of Metadata TBAA attachments

[Teresa Johnson via Phabricator via llvm-commits]

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

LGTM as a bug fix. But I have a few general questions below.



================
Comment at: llvm/lib/Bitcode/Reader/MetadataLoader.cpp:704
       !DisableLazyLoading) {
     auto SuccessOrErr = lazyLoadModuleMetadataBlock(Placeholders);
     if (!SuccessOrErr)
----------------
I just noticed that Placeholders is never used in lazyLoadModuleMetadataBlock, so it should probably be removed (in a follow up patch). It does create forward refs so I guess resolveForwardRefsAndPlaceholders below still needs to be invoked, but should it be invoked from within lazyLoadModuleMetadataBlock instead (and passed a dummy PlaceholdersQueue)?


================
Comment at: llvm/lib/Bitcode/Reader/MetadataLoader.cpp:1613
           lazyLoadOneMetadata(Idx, Placeholders);
+          resolveForwardRefsAndPlaceholders(Placeholders);
+        }
----------------
I suppose there is no loss in doing this more frequently since we know we need to resolve these anyway? Is there any disadvantage to doing the resolveForwardRefsAndPlaceholders more frequently? If so, should it only be done for TBAA attachments? If not, then should we be doing it more frequently elsewhere?


https://reviews.llvm.org/D28423