[PATCH] D35875: ThinLTO: Don't import aliases of any kind (even linkonce_odr)

[David Blaikie via llvm-commits]

On Thu, Jul 27, 2017 at 7:12 AM Teresa Johnson via Phabricator <
reviews at reviews.llvm.org> wrote:

> tejohnson added a comment.
>
> Sorry for the delay. A few comments below. Regarding Mehdi's performance
> question - I don't expect this to have a major impact as we were only
> importing aliases in limited cases anyway. We'll see any performance effect
> in our nightly testing once this goes in, which should surface any surprise
> effects.
>
>
>
> ================
> Comment at: lib/Transforms/IPO/FunctionImport.cpp:135
>            return false;
>          if (auto *AS = dyn_cast<AliasSummary>(GVSummary)) {
> +          // Aliases can't point to "available_externally".
> ----------------
> Braces can be removed
>

I go back and forth on this a bit - some people only omit {} when the body
is a single line (not just a single statement) - given the 3 line comment
it feels a bit better to me to include the braces (even though I'll omit
them on something like "for / if / for / return" nested 4 levels deep when
the first for then does have a 3 line body without braces - for some reason
the extra levels of indentation and their proximity are easier to read than
a long similarly indented body without a brace at the end... beats me)

Removed them here, though (:

Applied the rest of the fixes as well.

- Dave


>
>
> ================
> Comment at: lib/Transforms/IPO/FunctionImport.cpp:138
>            // FIXME: we should import alias as available_externally
> *function*,
>            // the destination module does need to know it is an alias.
> +          return false;
> ----------------
> While you're in here, can you correct the comment: s/does needs to
> know/does not need to know/
>
>
> ================
> Comment at: lib/Transforms/IPO/FunctionImport.cpp:228
>      const FunctionSummary *ResolvedCalleeSummary;
>      if (isa<AliasSummary>(CalleeSummary)) {
>        ResolvedCalleeSummary = cast<FunctionSummary>(
> ----------------
> This handling needs to be updated - can simply assert that CalleeSummary
> not an AliasSummary.
>
>
> ================
> Comment at: lib/Transforms/Utils/FunctionImportUtils.cpp:27
>    // For alias, we tie the definition to the base object. Extract it and
> recurse
> -  if (auto *GA = dyn_cast<GlobalAlias>(SGV)) {
> -    if (GA->isInterposable())
> -      return false;
> -    const GlobalObject *GO = GA->getBaseObject();
> -    if (!GO->hasLinkOnceODRLinkage())
> -      return false;
> -    return FunctionImportGlobalProcessing::doImportAsDefinition(
> -        GO, GlobalsToImport);
> -  }
> +  if (auto *GA = dyn_cast<GlobalAlias>(SGV))
> +    return false;
> ----------------
> GA is unused, and comment is stale. But I also think that this should be
> moved to after the below check of SGV being in GlobalsToImport, and
> converted to an assert (if in GlobalsToImport, better not be an alias).
>
>
> ================
> Comment at: lib/Transforms/Utils/FunctionImportUtils.cpp:133
>    case GlobalValue::ExternalLinkage:
>      // External defnitions are converted to available_externally
>      // definitions upon import, so that they are available for inlining
> ----------------
> "External and linkonce definitions" (can you fix typo in "defnitions"
> while here)?
>
>
> ================
> Comment at: test/ThinLTO/X86/alias_import.ll:48
>
>  ; On the import side now, verify that aliases to a linkonce_odr are
> imported, but the weak/linkonce (we can't inline them)
>  ; IMPORT-DAG:  declare void @linkonceODRfuncWeakAlias
> ----------------
> Comment needs update.
>
>
> https://reviews.llvm.org/D35875
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170727/823a2be9/attachment.html>