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

Mehdi AMINI via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Dec 30 10:58:35 PST 2016


mehdi_amini added inline comments.


================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:191
   if (HasInlineAsmMaybeReferencingInternal)
-    FuncSummary->setHasInlineAsmMaybeReferencingInternal();
+    FuncSummary->setNotEligibleToImport();
+  if (NonRenamableLocal)
----------------
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);
```



================
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
----------------
tejohnson wrote:
> mehdi_amini wrote:
> > I'd be OK to drop this test
> I'll go ahead and give it the same treatment you did to the other test (using llvm-link).
That's not equivalent though: the other test was stressing bitcode loading, it could be done equally with llvm-link.
This test was making sure we correctly "upgrade" the debug info before linking the IR in FunctionImport.cpp (IIRC). Changing to llvm-link won't exercise this code-path and we'll lose this coverage anyway.
Thinking about it again, we may be able to do it with `opt -functionimport` though


https://reviews.llvm.org/D28169





More information about the llvm-commits mailing list