[PATCH] D28751: [ThinLTO] Add a recursive step in Metadata lazy-loading

[Teresa Johnson via Phabricator via llvm-commits]

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

Nice improvement - is this related to the regression you saw in your measurements on the effectiveness of lazy loading metadata? A few comments/suggestions below.

In https://reviews.llvm.org/D28751#646918, @mehdi_amini wrote:

> Add a comment, and handles correctly uniquing cycles (even if I didn't encounter any when bootstrapping clang)


Do we have a test case that exposes this issue? If not, it would be good to add



================
Comment at: llvm/lib/Bitcode/Reader/MetadataLoader.cpp:836
+      // If lazy-loading is enabled, we try recursively to load the operand
+      // instead of creating a temporary.
+      if (ID < (MDStringRef.size() + GlobalMetadataBitPosIndex.size())) {
----------------
The comment is a little confusing, since we are in fact creating a temporary just above. Maybe change to something like "instead of keeping the temporary which requires a later RAUW".


https://reviews.llvm.org/D28751