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

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

Thanks for the comments, will address as noted below before submitting.


================
Comment at: tools/gold/gold-plugin.cpp:1018-1019
@@ +1017,4 @@
+    if (options::TheOutputType == options::OT_SAVE_TEMPS) {
+      // Always expect a provided filename in the ThinLTO backend.
+      assert(!SaveTempsFilename.empty());
+      SmallString<128> OptFilename(SaveTempsFilename);
----------------
Prazek wrote:
> I think it would be good to put this comment in an assert.
Ok, will do when I add this later in a separate patch.

================
Comment at: tools/gold/gold-plugin.cpp:1023
@@ -1021,1 +1022,3 @@
+      saveBCFile(OptFilename, *M);
+    }
   }
----------------
mehdi_amini wrote:
> Is this related to the rest of this patch? How was it handled before? 
It is related in the sense that the new test uses the .import.bc file to do some checking (before the optimization pipeline kicks in and does inlining, e.g.). But actually, I just changed the test to check the .opt.bc saved temps files and it still works - looks like the linkonce_odr removal doesn't happen until code gen. I will remove this new block (which is also useful for debugging, but not then related to this patch, I can submit separately) and change the test.

================
Comment at: tools/gold/gold-plugin.cpp:1364
@@ -1360,2 +1363,3 @@
       if (Resolution != LDPR_PREVAILING_DEF_IRONLY)
         Preserve.insert(GlobalValue::getGUID(Sym.name));
+
----------------
mehdi_amini wrote:
> 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".
Right, I was trying to include both scenarios where we need to preserve this in one comment. I.e. if is is not simply referenced by a single IR modue (referenced from other IR modules or from a non-IR file) then it will not have LDPR_PREVAILING_DEF_IRONLY somewhere and will end up in the Preserve set. I will clarify this in the comment.

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


https://reviews.llvm.org/D22302