[PATCH] D28169: [ThinLTO] Subsume all importing checks into a single flag

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D28169#634946, @mehdi_amini wrote:

> In https://reviews.llvm.org/D28169#634938, @tejohnson wrote:
>
> > Ah ok, that's more than what we do currently. Do you think I should add that to this patch?
>
>
> Not necessarily: it'd be nice to have such validation of the expected invariant, but I have no idea how much work it is, so if you think you can do it in ~30min, I'd say "yes please", otherwise I would't require it for us to move forward. Sounds reasonable?


I went ahead and added this in the new patch I am uploading.



================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:125
+
+    GVFlags(const GlobalValue &GV, bool NonRenamableLocal)
+        : Linkage(GV.getLinkage()) {
----------------
tejohnson wrote:
> mehdi_amini wrote:
> > Seems to me that it is `NotEligibleToImport`.
> Here I specifically meant that the parameter indicates it is a local that can't be renamed/promoted - it is only one of the things that goes into determining NotEligibleToImport. Rethinking - I will move the variadic check into the caller (it seems more appropriate to have this logic where we compute the summary index anyway), then I will just nuke this constructor and have it invoke the above one.
Removed this constructor, moved variadic check to module summary builder.


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:127
+        : Linkage(GV.getLinkage()) {
+      NotEligibleToImport = NonRenamableLocal;
       if (const auto *F = dyn_cast<Function>(&GV))
----------------
tejohnson wrote:
> mehdi_amini wrote:
> > Delegate to the other Ctor: 
> > 
> > ```
> > GVFlags(const GlobalValue &GV, bool NotEligibleToImport)
> >     : GVFlags(GV.getLinkage(), NotEligibleToImport) {
> > ...
> > ```
> Going to remove this one (see above comment)
ditto


================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:191
   if (HasInlineAsmMaybeReferencingInternal)
-    FuncSummary->setHasInlineAsmMaybeReferencingInternal();
+    FuncSummary->setNotEligibleToImport();
+  if (NonRenamableLocal)
----------------
mehdi_amini wrote:
> tejohnson wrote:
> > mehdi_amini wrote:
> > > This can be moved a few lines above when creating `Flags` IIUC.
> > I assume you mean the "if (NotRenamableLocal) ... " handling (in phab it looks like it's on the lines above). Yes, I can move this.
> I meant ` FuncSummary->setNotEligibleToImport();`.
> 
> IIUC it could be:
> 
> ```
> bool NonRenamableLocal = isNonRenamableLocal(F);
> bool IsEligibleForImport = NonRenamableLocal || HasInlineAsmMaybeReferencingInternal
> GlobalValueSummary::GVFlags Flags(F, IsEligibleForImport);
> ```
> 
Done (but with variable being NotEligibleToImport not IsEligibleToImport)


================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:244
   collectUsedGlobalVariables(M, Used, /*CompilerUsed*/ true);
+  SmallPtrSet<GlobalValue::GUID, 8> NonExternallyReferenceable;
   for (auto *V : Used) {
----------------
tejohnson wrote:
> mehdi_amini wrote:
> > I'm not sure if `NonExternallyReferenceable` is the best name, I feel it would be more accurate with `CantBePromoted` or something like that.
> > (Any local symbols is "not externally referenceable" in the absolute).
> Ok, I can change the name. I was trying to find a name that indicates it is 1) a local (currently) and 2) can't be renamed/promoted. I think CantBePromoted is sufficiently descriptive.
done


https://reviews.llvm.org/D28169