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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added a comment.

So we're losing some information for the ThinLink, which is somehow scary, I hope we won't have to backtrack later.
Originally we had these separate flags because we expected to propagate more information across modules. But I don't see any use-case that would miss these flags right now, so...



================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:125
+
+    GVFlags(const GlobalValue &GV, bool NonRenamableLocal)
+        : Linkage(GV.getLinkage()) {
----------------
Seems to me that it is `NotEligibleToImport`.


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:127
+        : Linkage(GV.getLinkage()) {
+      NotEligibleToImport = NonRenamableLocal;
       if (const auto *F = dyn_cast<Function>(&GV))
----------------
Delegate to the other Ctor: 

```
GVFlags(const GlobalValue &GV, bool NotEligibleToImport)
    : GVFlags(GV.getLinkage(), NotEligibleToImport) {
...
```


================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:191
   if (HasInlineAsmMaybeReferencingInternal)
-    FuncSummary->setHasInlineAsmMaybeReferencingInternal();
+    FuncSummary->setNotEligibleToImport();
+  if (NonRenamableLocal)
----------------
This can be moved a few lines above when creating `Flags` IIUC.


================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:244
   collectUsedGlobalVariables(M, Used, /*CompilerUsed*/ true);
+  SmallPtrSet<GlobalValue::GUID, 8> NonExternallyReferenceable;
   for (auto *V : Used) {
----------------
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).


================
Comment at: test/ThinLTO/X86/autoupgrade.ll:11
-; CHECK-NOT: 'llvm.invariant.start'
-; CHECK: record string = 'llvm.invariant.start.p0i8'
 ; CHECK-NOT: 'llvm.invariant.start'
----------------
This was an annoying an subtle bug, I'm concerned about losing coverage for this.
If there is no other way, we should regenerate the bitcode file with the new summary.
I'm wondering now if this test can't be triggered with llvm-link instead of llvm-lto. I'll try and report.


================
Comment at: test/ThinLTO/X86/drop-debug-info.ll:5
-; The imported module has out-of-date debug information, let's make sure we can
-; drop them without crashing when materializing later.
 ; RUN: llvm-lto -thinlto-action=import %t.bc -thinlto-index=%t.index.bc -o - | llvm-dis -o - | FileCheck %s
----------------
I'd be OK to drop this test


https://reviews.llvm.org/D28169