[PATCH] D22302: [ThinLTO/gold] Perform index-based weak/linkonce resolution

[Mehdi AMINI via llvm-commits]

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

LGTM, with some comments.


================
Comment at: tools/gold/gold-plugin.cpp:1023
@@ -1021,1 +1022,3 @@
+      saveBCFile(OptFilename, *M);
+    }
   }
----------------
Is this related to the rest of this patch? How was it handled before? 

================
Comment at: tools/gold/gold-plugin.cpp:1364
@@ -1360,2 +1363,3 @@
       if (Resolution != LDPR_PREVAILING_DEF_IRONLY)
         Preserve.insert(GlobalValue::getGUID(Sym.name));
+
----------------
I'm not sure what is this doing, the comment said "identify symbols referenced by more than a single IR module".

After checking https://github.com/geofft/binutils/blob/master/include/plugin-api.h ; it is Preserving any symbols that is not *exclusively referenced from an IR file* (i.e. the comment above the loop is incomplete: it will "identify symbols that can needs to be preserved outside of LTO, i.e. symbols that LTO can't eliminate under any condition".

================
Comment at: tools/gold/gold-plugin.cpp:1370
@@ -1362,1 +1369,3 @@
+        PrevailingCopy[GlobalValue::getGUID(Sym.name)] =
+            Index->getGlobalValueSummary(GlobalValue::getGUID(Sym.name));
     }
----------------
`GlobalValue::getGUID` isn't free, you may want to CSE on these two lines.


https://reviews.llvm.org/D22302