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

Teresa Johnson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Dec 30 10:25:06 PST 2016


tejohnson added a comment.

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

> 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...


Thinking through the individual flags, I am somewhat questioning whether NoRename should be split out. Here's what this patch folds into NotEligibleToImport, along with some thoughts on whether we could eventually import them:

1. NoRename: can't import a def or a ref if local because that currently requires promotion/renaming. But what if in the future we would be able to import as a local copy? In that case it would be useful to split this out as a separate flag. But right now this is set when it is a local in a section - is it legal to create a copy in that case? If not, we would need to create a different flag for any non-renamable copyable cases that pop up in the future anyway.

2. HasInlineAsmMaybeReferencingInternal: The only way we can ever safely import something that has opaque references (via inline asm) is if we also imported all locals in the llvm.*used sets as local copies, just to be safe. Not sure we would ever want to do that - I suppose we could eventually encounter a function that contains inline asm and locals in the used set that is profitable to import/inline, but do we want to carry around an extra bit in the summary before we find such a case?

3. IsNotViableToInline: This one is a hint, not correctness, and currently only set when the function is variadic. If the inliner is ever able to inline variadic functions, we could just remove this flag (or stop setting the new NotEligibleToImport combined flag in that case). So I don't think it is currently worthwhile to split this out into a separate flag anyway. If we want to use inliner information for more complex importing decisions in the thin link I think this would have to be made into a richer set of flags anyway. Or would we ever want to import something that isn't inlineable? The only case I can think of is if it is also a local function that is non-renamable/promoteable, we want to import a reference to it, and we have the ability to import such non-renamable/promoteable local functions as local copies. In that case though, I would argue that we could simply stop marking these functions as NotEligibleToImport - it is just a small compile time/memory improvement anyway.

4. Anything that references a non-renamable/promoteable local (what I am moving from the thin link to the summary generation with this patch). If we can import these referenced locals as a copy (see #1 above), then we will simply need to stop setting the NotEligibleToImport flag on references to those copyable non-renamable locals. Then once we decide to import any function, we'll need to walk through its references/calls and mark any such references for import (presumably the import logic will ensure that any local function marked for import that is also marked as non-renamable will be imported as a local copy). So I don't think we need to split this one out into a separate flag.

Thoughts?



================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:125
+
+    GVFlags(const GlobalValue &GV, bool NonRenamableLocal)
+        : Linkage(GV.getLinkage()) {
----------------
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.


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:127
+        : Linkage(GV.getLinkage()) {
+      NotEligibleToImport = NonRenamableLocal;
       if (const auto *F = dyn_cast<Function>(&GV))
----------------
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)


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


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


================
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'
----------------
mehdi_amini wrote:
> mehdi_amini wrote:
> > mehdi_amini wrote:
> > > 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.
> > r290736
> (and r290740)
Great thanks, forgot about that functionality.


================
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
----------------
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).


https://reviews.llvm.org/D28169





More information about the llvm-commits mailing list