[llvm] r286297 - [ThinLTO] Prevent exporting of locals used/defined in module level asm
David Jones via llvm-commits
llvm-commits at lists.llvm.org
Tue Nov 8 17:46:25 PST 2016
It looks like this adds a new layering violation:
Analysis/ModuleSummaryAnalysis.cpp ->
Object/IRObjectFile ->
Bitcode ->
Analysis
In particular, the long pole appears to be
IRObjectFile::CollectAsmUndefinedRefs. It is almost independent of Object,
except for BasicSymbolRef::Flags.
Ben and I looked at it for a bit but didn't see any obvious moves. Teresa,
Mehdi, any ideas?
On Tue, Nov 8, 2016 at 1:53 PM, Teresa Johnson via llvm-commits <
llvm-commits at lists.llvm.org> wrote:
> Author: tejohnson
> Date: Tue Nov 8 15:53:35 2016
> New Revision: 286297
>
> URL: http://llvm.org/viewvc/llvm-project?rev=286297&view=rev
> Log:
> [ThinLTO] Prevent exporting of locals used/defined in module level asm
>
> Summary:
> This patch uses the same approach added for inline asm in r285513 to
> similarly prevent promotion/renaming of locals used or defined in module
> level asm.
>
> All static global values defined in normal IR and used in module level asm
> should be included on either the llvm.used or llvm.compiler.used global.
> The former were already being flagged as NoRename in the summary, and
> I've simply added llvm.compiler.used values to this handling.
>
> Module level asm may also contain defs of values. We need to prevent
> export of any refs to local values defined in module level asm (e.g. a
> ref in normal IR), since that also requires renaming/promotion of the
> local. To do that, the summary index builder looks at all values in the
> module level asm string that are not marked Weak or Global, which is
> exactly the set of locals that are defined. A summary is created for
> each of these local defs and flagged as NoRename.
>
> This required adding handling to the BitcodeWriter to look at GV
> declarations to see if they have a summary (rather than skipping them
> all).
>
> Finally, added an assert to IRObjectFile::CollectAsmUndefinedRefs to
> ensure that an MCAsmParser is available, otherwise the module asm parse
> would silently fail. Initialized the asm parser in the opt tool for use
> in testing this fix.
>
> Fixes PR30610.
>
> Reviewers: mehdi_amini
>
> Subscribers: johanengelen, krasin, llvm-commits
>
> Differential Revision: https://reviews.llvm.org/D26146
>
> Added:
> llvm/trunk/test/ThinLTO/X86/Inputs/module_asm2.ll
> llvm/trunk/test/ThinLTO/X86/module_asm2.ll
> Modified:
> llvm/trunk/include/llvm/Support/TargetRegistry.h
> llvm/trunk/lib/Analysis/LLVMBuild.txt
> llvm/trunk/lib/Analysis/ModuleSummaryAnalysis.cpp
> llvm/trunk/lib/Bitcode/Writer/BitcodeWriter.cpp
> llvm/trunk/lib/Object/IRObjectFile.cpp
> llvm/trunk/test/LTO/X86/current-section.ll
> llvm/trunk/tools/opt/opt.cpp
>
> Modified: llvm/trunk/include/llvm/Support/TargetRegistry.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/
> llvm/Support/TargetRegistry.h?rev=286297&r1=286296&r2=286297&view=diff
> ============================================================
> ==================
> --- llvm/trunk/include/llvm/Support/TargetRegistry.h (original)
> +++ llvm/trunk/include/llvm/Support/TargetRegistry.h Tue Nov 8 15:53:35
> 2016
> @@ -280,6 +280,9 @@ public:
> /// hasMCAsmBackend - Check if this target supports .o generation.
> bool hasMCAsmBackend() const { return MCAsmBackendCtorFn != nullptr; }
>
> + /// hasMCAsmParser - Check if this target supports assembly parsing.
> + bool hasMCAsmParser() const { return MCAsmParserCtorFn != nullptr; }
> +
> /// @}
> /// @name Feature Constructors
> /// @{
>
> Modified: llvm/trunk/lib/Analysis/LLVMBuild.txt
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/
> Analysis/LLVMBuild.txt?rev=286297&r1=286296&r2=286297&view=diff
> ============================================================
> ==================
> --- llvm/trunk/lib/Analysis/LLVMBuild.txt (original)
> +++ llvm/trunk/lib/Analysis/LLVMBuild.txt Tue Nov 8 15:53:35 2016
> @@ -19,4 +19,4 @@
> type = Library
> name = Analysis
> parent = Libraries
> -required_libraries = Core Support ProfileData
> +required_libraries = Core Support ProfileData Object
>
> Modified: llvm/trunk/lib/Analysis/ModuleSummaryAnalysis.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/
> ModuleSummaryAnalysis.cpp?rev=286297&r1=286296&r2=286297&view=diff
> ============================================================
> ==================
> --- llvm/trunk/lib/Analysis/ModuleSummaryAnalysis.cpp (original)
> +++ llvm/trunk/lib/Analysis/ModuleSummaryAnalysis.cpp Tue Nov 8 15:53:35
> 2016
> @@ -13,6 +13,7 @@
> //===-------------------------------------------------------
> ---------------===//
>
> #include "llvm/Analysis/ModuleSummaryAnalysis.h"
> +#include "llvm/ADT/Triple.h"
> #include "llvm/Analysis/BlockFrequencyInfo.h"
> #include "llvm/Analysis/BlockFrequencyInfoImpl.h"
> #include "llvm/Analysis/BranchProbabilityInfo.h"
> @@ -24,6 +25,7 @@
> #include "llvm/IR/InstIterator.h"
> #include "llvm/IR/IntrinsicInst.h"
> #include "llvm/IR/ValueSymbolTable.h"
> +#include "llvm/Object/IRObjectFile.h"
> #include "llvm/Pass.h"
> using namespace llvm;
>
> @@ -194,12 +196,22 @@ ModuleSummaryIndex llvm::buildModuleSumm
> ProfileSummaryInfo *PSI) {
> ModuleSummaryIndex Index;
>
> - // Identify the local values in the llvm.used set, which should not be
> - // exported as they would then require renaming and promotion, but we
> - // may have opaque uses e.g. in inline asm.
> + // Identify the local values in the llvm.used and llvm.compiler.used
> sets,
> + // which should not be exported as they would then require renaming and
> + // promotion, but we may have opaque uses e.g. in inline asm. We
> collect them
> + // here because we use this information to mark functions containing
> inline
> + // assembly calls as not importable.
> + SmallPtrSet<GlobalValue *, 8> LocalsUsed;
> SmallPtrSet<GlobalValue *, 8> Used;
> + // First collect those in the llvm.used set.
> collectUsedGlobalVariables(M, Used, /*CompilerUsed*/ false);
> - SmallPtrSet<GlobalValue *, 8> LocalsUsed;
> + for (auto *V : Used) {
> + if (V->hasLocalLinkage())
> + LocalsUsed.insert(V);
> + }
> + Used.clear();
> + // Next collect those in the llvm.compiler.used set.
> + collectUsedGlobalVariables(M, Used, /*CompilerUsed*/ true);
> for (auto *V : Used) {
> if (V->hasLocalLinkage())
> LocalsUsed.insert(V);
> @@ -244,6 +256,47 @@ ModuleSummaryIndex llvm::buildModuleSumm
> Summary->setNoRename();
> }
>
> + if (!M.getModuleInlineAsm().empty()) {
> + // Collect the local values defined by module level asm, and set up
> + // summaries for these symbols so that they can be marked as NoRename,
> + // to prevent export of any use of them in regular IR that would
> require
> + // renaming within the module level asm. Note we don't need to create
> a
> + // summary for weak or global defs, as they don't need to be flagged
> as
> + // NoRename, and defs in module level asm can't be imported anyway.
> + // Also, any values used but not defined within module level asm
> should
> + // be listed on the llvm.used or llvm.compiler.used global and marked
> as
> + // referenced from there.
> + // FIXME: Rename CollectAsmUndefinedRefs to something more general,
> as we
> + // are also using it to find the file-scope locals defined in module
> asm.
> + object::IRObjectFile::CollectAsmUndefinedRefs(
> + Triple(M.getTargetTriple()), M.getModuleInlineAsm(),
> + [&M, &Index](StringRef Name, object::BasicSymbolRef::Flags Flags)
> {
> + // Symbols not marked as Weak or Global are local definitions.
> + if (Flags & (object::BasicSymbolRef::SF_Weak ||
> + object::BasicSymbolRef::SF_Global))
> + return;
> + GlobalValue *GV = M.getNamedValue(Name);
> + if (!GV)
> + return;
> + assert(GV->isDeclaration() && "Def in module asm already has
> definition");
> + GlobalValueSummary::GVFlags GVFlags(GlobalValue::
> InternalLinkage,
> + /* NoRename */ true,
> + /*IsNotViableToInline */
> true);
> + // Create the appropriate summary type.
> + if (isa<Function>(GV)) {
> + std::unique_ptr<FunctionSummary> Summary =
> + llvm::make_unique<FunctionSummary>(GVFlags, 0);
> + Summary->setNoRename();
> + Index.addGlobalValueSummary(Name, std::move(Summary));
> + } else {
> + std::unique_ptr<GlobalVarSummary> Summary =
> + llvm::make_unique<GlobalVarSummary>(GVFlags);
> + Summary->setNoRename();
> + Index.addGlobalValueSummary(Name, std::move(Summary));
> + }
> + });
> + }
> +
> return Index;
> }
>
>
> Modified: llvm/trunk/lib/Bitcode/Writer/BitcodeWriter.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/
> Bitcode/Writer/BitcodeWriter.cpp?rev=286297&r1=286296&r2=286297&view=diff
> ============================================================
> ==================
> --- llvm/trunk/lib/Bitcode/Writer/BitcodeWriter.cpp (original)
> +++ llvm/trunk/lib/Bitcode/Writer/BitcodeWriter.cpp Tue Nov 8 15:53:35
> 2016
> @@ -3327,11 +3327,16 @@ void ModuleBitcodeWriter::writePerModule
> void ModuleBitcodeWriter::writeModuleLevelReferences(
> const GlobalVariable &V, SmallVector<uint64_t, 64> &NameVals,
> unsigned FSModRefsAbbrev) {
> - // Only interested in recording variable defs in the summary.
> - if (V.isDeclaration())
> + auto Summaries =
> + Index->findGlobalValueSummaryList(GlobalValue::getGUID(V.
> getName()));
> + if (Summaries == Index->end()) {
> + // Only declarations should not have a summary (a declaration might
> however
> + // have a summary if the def was in module level asm).
> + assert(V.isDeclaration());
> return;
> + }
> + auto *Summary = Summaries->second.front().get();
> NameVals.push_back(VE.getValueID(&V));
> - auto *Summary = Index->getGlobalValueSummary(V);
> GlobalVarSummary *VS = cast<GlobalVarSummary>(Summary);
> NameVals.push_back(getEncodedGVSummaryFlags(VS->flags()));
>
> @@ -3409,14 +3414,20 @@ void ModuleBitcodeWriter::writePerModule
> // Iterate over the list of functions instead of the Index to
> // ensure the ordering is stable.
> for (const Function &F : M) {
> - if (F.isDeclaration())
> - continue;
> // Summary emission does not support anonymous functions, they have to
> // renamed using the anonymous function renaming pass.
> if (!F.hasName())
> report_fatal_error("Unexpected anonymous function when writing
> summary");
>
> - auto *Summary = Index->getGlobalValueSummary(F);
> + auto Summaries =
> + Index->findGlobalValueSummaryList(GlobalValue::getGUID(F.
> getName()));
> + if (Summaries == Index->end()) {
> + // Only declarations should not have a summary (a declaration might
> + // however have a summary if the def was in module level asm).
> + assert(F.isDeclaration());
> + continue;
> + }
> + auto *Summary = Summaries->second.front().get();
> writePerModuleFunctionSummaryRecord(NameVals, Summary,
> VE.getValueID(&F),
> FSCallsAbbrev,
> FSCallsProfileAbbrev, F);
> }
>
> Modified: llvm/trunk/lib/Object/IRObjectFile.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Object/
> IRObjectFile.cpp?rev=286297&r1=286296&r2=286297&view=diff
> ============================================================
> ==================
> --- llvm/trunk/lib/Object/IRObjectFile.cpp (original)
> +++ llvm/trunk/lib/Object/IRObjectFile.cpp Tue Nov 8 15:53:35 2016
> @@ -54,8 +54,7 @@ void IRObjectFile::CollectAsmUndefinedRe
>
> std::string Err;
> const Target *T = TargetRegistry::lookupTarget(TT.str(), Err);
> - if (!T)
> - return;
> + assert(T && T->hasMCAsmParser());
>
> std::unique_ptr<MCRegisterInfo> MRI(T->createMCRegInfo(TT.str()));
> if (!MRI)
>
> Modified: llvm/trunk/test/LTO/X86/current-section.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/LTO/
> X86/current-section.ll?rev=286297&r1=286296&r2=286297&view=diff
> ============================================================
> ==================
> --- llvm/trunk/test/LTO/X86/current-section.ll (original)
> +++ llvm/trunk/test/LTO/X86/current-section.ll Tue Nov 8 15:53:35 2016
> @@ -2,4 +2,7 @@
> ; RUN: llvm-lto -o %t2 %t1
> ; REQUIRES: default_triple
>
> +target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
> +target triple = "x86_64-unknown-linux-gnu"
> +
> module asm ".align 4"
>
> Added: llvm/trunk/test/ThinLTO/X86/Inputs/module_asm2.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/
> ThinLTO/X86/Inputs/module_asm2.ll?rev=286297&view=auto
> ============================================================
> ==================
> --- llvm/trunk/test/ThinLTO/X86/Inputs/module_asm2.ll (added)
> +++ llvm/trunk/test/ThinLTO/X86/Inputs/module_asm2.ll Tue Nov 8 15:53:35
> 2016
> @@ -0,0 +1,12 @@
> +target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
> +target triple = "x86_64-unknown-linux-gnu"
> +
> +define i32 @main({ i64, { i64, i8* }* } %unnamed) #0 {
> + %1 = call i32 @func1() #1
> + %2 = call i32 @func2() #1
> + %3 = call i32 @func3() #1
> + ret i32 %1
> +}
> +declare i32 @func1() #1
> +declare i32 @func2() #1
> +declare i32 @func3() #1
>
> Added: llvm/trunk/test/ThinLTO/X86/module_asm2.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/
> ThinLTO/X86/module_asm2.ll?rev=286297&view=auto
> ============================================================
> ==================
> --- llvm/trunk/test/ThinLTO/X86/module_asm2.ll (added)
> +++ llvm/trunk/test/ThinLTO/X86/module_asm2.ll Tue Nov 8 15:53:35 2016
> @@ -0,0 +1,84 @@
> +; Test to ensure that uses and defs in module level asm are handled
> +; appropriately. Specifically, we should conservatively block importing
> +; of any references to these values, as they can't be renamed.
> +; RUN: opt -module-summary %s -o %t1.bc
> +; RUN: opt -module-summary %p/Inputs/module_asm2.ll -o %t2.bc
> +
> +; RUN: llvm-lto -thinlto-action=run -exported-symbol=main
> -exported-symbol=func1 -exported-symbol=func2 -exported-symbol=func3 %t1.bc
> %t2.bc
> +; RUN: llvm-nm %t1.bc.thinlto.o | FileCheck %s --check-prefix=NM0
> +; RUN: llvm-nm %t2.bc.thinlto.o | FileCheck %s --check-prefix=NM1
> +
> +; RUN: llvm-lto2 %t1.bc %t2.bc -o %t.o -save-temps \
> +; RUN: -r=%t1.bc,foo,plx \
> +; RUN: -r=%t1.bc,b,pl \
> +; RUN: -r=%t1.bc,x,pl \
> +; RUN: -r=%t1.bc,func1,pl \
> +; RUN: -r=%t1.bc,func2,pl \
> +; RUN: -r=%t1.bc,func3,pl \
> +; RUN: -r=%t2.bc,main,plx \
> +; RUN: -r=%t2.bc,func1,l \
> +; RUN: -r=%t2.bc,func2,l \
> +; RUN: -r=%t2.bc,func3,l
> +; RUN: llvm-nm %t.o.0 | FileCheck %s --check-prefix=NM0
> +; RUN: llvm-nm %t.o.1 | FileCheck %s --check-prefix=NM1
> +
> +; Check that local values b and x, which are referenced on
> +; llvm.used and llvm.compiler.used, respectively, are not promoted.
> +; Similarly, foo which is defined in module level asm should not be
> +; promoted.
> +; NM0-DAG: d b
> +; NM0-DAG: d x
> +; NM0-DAG: t foo
> +; NM0-DAG: T func1
> +; NM0-DAG: T func2
> +; NM0-DAG: T func3
> +
> +; Ensure that foo, b and x are likewise not exported (imported as refs
> +; into the other module), since they can't be promoted. Additionally,
> +; referencing functions func1, func2 and func3 should not have been
> +; imported.
> +; NM1-NOT: foo
> +; NM1-NOT: b
> +; NM1-NOT: x
> +; NM1-DAG: U func1
> +; NM1-DAG: U func2
> +; NM1-DAG: U func3
> +; NM1-DAG: T main
> +; NM1-NOT: foo
> +; NM1-NOT: b
> +; NM1-NOT: x
> +
> +target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
> +target triple = "x86_64-unknown-linux-gnu"
> +
> + at b = internal global i32 1, align 4
> + at x = internal global i32 1, align 4
> +
> + at llvm.compiler.used = appending global [1 x i8*] [i8* bitcast (i32* @b to
> i8*)], section "llvm.metadata"
> + at llvm.used = appending global [1 x i8*] [i8* bitcast (i32* @x to i8*)],
> section "llvm.metadata"
> +
> +module asm "\09.text"
> +module asm "\09.type\09foo, at function"
> +module asm "foo:"
> +module asm "\09movl b, %eax"
> +module asm "\09movl x, %edx"
> +module asm "\09ret "
> +module asm "\09.size\09foo, .-foo"
> +module asm ""
> +
> +declare i16 @foo() #0
> +
> +define i32 @func1() #1 {
> + call i16 @foo()
> + ret i32 1
> +}
> +
> +define i32 @func2() #1 {
> + %1 = load i32, i32* @b, align 4
> + ret i32 %1
> +}
> +
> +define i32 @func3() #1 {
> + %1 = load i32, i32* @x, align 4
> + ret i32 %1
> +}
>
> Modified: llvm/trunk/tools/opt/opt.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/opt/
> opt.cpp?rev=286297&r1=286296&r2=286297&view=diff
> ============================================================
> ==================
> --- llvm/trunk/tools/opt/opt.cpp (original)
> +++ llvm/trunk/tools/opt/opt.cpp Tue Nov 8 15:53:35 2016
> @@ -364,6 +364,7 @@ int main(int argc, char **argv) {
> InitializeAllTargets();
> InitializeAllTargetMCs();
> InitializeAllAsmPrinters();
> + InitializeAllAsmParsers();
>
> // Initialize passes
> PassRegistry &Registry = *PassRegistry::getPassRegistry();
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161108/110fdb04/attachment.html>
More information about the llvm-commits
mailing list