[PATCH] D26402: [ThinLTO] Make inline assembly handling more efficient in summary

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D26402#590932, @tejohnson wrote:

> > Sort of - we will look at the export lists (actually the linkage as marked in the index) when we decide what to internalize, but not what to promote in renameModuleForThinLTO. This means that we first aggressively promote/rename what we can, then we internalize anything that didn't need to be promoted. It has been on my list of things to fix (see the FIXME in llvm::thinLTOInternalizeModule MustPromoteGV for example). I guess this is a good time to fix it. I think there is a quick fix since we are now getting the summary entry already in doPromoteLocalToGlobal in order to check the NoRename flag, it can just check the linkage type in the entry instead. Will try that out.
>
> See https://reviews.llvm.org/D26467 for the work to get the promotion logic to be selective based on the index for the exporting module.


I started to rebase this patch on top of https://reviews.llvm.org/D26467, but after adjusting the logic in the function importer, I think there is a good reason to keep the separate flags. We have a FIXME in the importer where we check if the current summary is eligible for importing:

  // Can't import a global that needs renaming if has a section for instance.
  // FIXME: we may be able to import it by copying it without promotion.

If we drop the NoRename flag, then we have no idea at thin link time which summaries must be copied in order to be imported. And if we have a single MayReferenceNoRename flag, it will then necessarily require being set in both the case where we have *possible* opaque references to non-renamable values, and where we know for sure it is referencing non-renamable values. We won't be able to distinguish when it is safe to import it unless we copy *all* local values referenced in the module (because we don't know which local values are non-renamable, and we don't know if the MayReferenceNoRename function call/reference set even contains all the necessary edges to those non-renamable locals vs a possible reference from opaque inline assembly).

I.e. let's say we have these three situations:

1. Non-opaque reference to a local variable that has a section

int X __attribute__ ((section ("MYSECTION")));
foo() {

  ... = X;

}

2. Opaque use of local variable in inline asm call

static int Y;
bar() {

  asm("...Y...");

}

3. Non-opaque use of a local defined in module level asm

module asm "Z: ..."  // i.e. module level asm def of a local Z
baz() {

  ... = Z;

}

In cases 1 and 3 we could import the functions (foo and baz, respectively) if we did a copy without promotion/rename of the referenced non-renamable local.

In case 2 we cannot import bar, due to the opaque reference in the inline asm call (well, we could only if we copied in *all local values defined and referenced in the module* - we could potentially refine that set once we load the module during the backend import and see which are referenced from llvm.used/llvm.compiler.used, but we don't know this during the Thin Link).

With the current version of this patch, we can detect this because the foo and baz* summaries have a direct reference to the summaries of those locals, which are flagged as NoRename (and wouldn't have the MayReferenceNoRename flag set). And bar will simply have the MayReferenceNoRename flag set, preventing its import. (*baz will once r286297 is resubmitted)

If I change this to use a single MayReferenceNoRename flag, then when we are in the Thin Link these 3 cases are indistinguishable.

Note that https://reviews.llvm.org/D26467 (selective promotion) is still good and should go in independently.


https://reviews.llvm.org/D26402