From 8b036feacf91477815020f6ff37e4323eccc68fd Mon Sep 17 00:00:00 2001 From: Nikita Popov Date: Wed, 28 Nov 2018 15:26:38 +0100 Subject: [PATCH 1/2] Revert "[DebugInfo][Dexter] Unreachable line stepped onto after SimplifyCFG." This reverts commit 8381d3e30329e6bc0c3014b3edd3d5db1e8f3a4b. --- include/llvm/IR/Instruction.h | 8 -- lib/IR/Instruction.cpp | 7 -- lib/Transforms/Utils/SimplifyCFG.cpp | 16 ---- test/CodeGen/X86/pr39187-g.ll | 121 --------------------------- 4 files changed, 152 deletions(-) delete mode 100644 test/CodeGen/X86/pr39187-g.ll diff --git a/include/llvm/IR/Instruction.h b/include/llvm/IR/Instruction.h index e1a1faedf11f..2a75801c649e 100644 --- a/include/llvm/IR/Instruction.h +++ b/include/llvm/IR/Instruction.h @@ -586,14 +586,6 @@ class Instruction : public User, static_cast(this)->getNextNonDebugInstruction()); } - /// Return a pointer to the previous non-debug instruction in the same basic - /// block as 'this', or nullptr if no such instruction exists. - const Instruction *getPrevNonDebugInstruction() const; - Instruction *getPrevNonDebugInstruction() { - return const_cast( - static_cast(this)->getPrevNonDebugInstruction()); - } - /// Create a copy of 'this' instruction that is identical in all ways except /// the following: /// * The instruction has no parent diff --git a/lib/IR/Instruction.cpp b/lib/IR/Instruction.cpp index 1e326370318e..75e7413a47d0 100644 --- a/lib/IR/Instruction.cpp +++ b/lib/IR/Instruction.cpp @@ -602,13 +602,6 @@ const Instruction *Instruction::getNextNonDebugInstruction() const { return nullptr; } -const Instruction *Instruction::getPrevNonDebugInstruction() const { - for (const Instruction *I = getPrevNode(); I; I = I->getPrevNode()) - if (!isa(I)) - return I; - return nullptr; -} - bool Instruction::isAssociative() const { unsigned Opcode = getOpcode(); if (isAssociative(Opcode)) diff --git a/lib/Transforms/Utils/SimplifyCFG.cpp b/lib/Transforms/Utils/SimplifyCFG.cpp index af9d2eaf2535..849f9ee1d19d 100644 --- a/lib/Transforms/Utils/SimplifyCFG.cpp +++ b/lib/Transforms/Utils/SimplifyCFG.cpp @@ -1372,14 +1372,6 @@ static bool HoistThenElseCodeToIf(BranchInst *BI, } } - // As the parent basic block terminator is a branch instruction which is - // removed at the end of the current transformation, use its previous - // non-debug instruction, as the reference insertion point, which will - // provide the debug location for the instruction being hoisted. For BBs - // with only debug instructions, use an empty debug location. - Instruction *InsertPt = - BIParent->getTerminator()->getPrevNonDebugInstruction(); - // Okay, it is safe to hoist the terminator. Instruction *NT = I1->clone(); BIParent->getInstList().insert(BI->getIterator(), NT); @@ -1389,14 +1381,6 @@ static bool HoistThenElseCodeToIf(BranchInst *BI, NT->takeName(I1); } - // The instruction NT being hoisted, is the terminator for the true branch, - // with debug location (DILocation) within that branch. We can't retain - // its original debug location value, otherwise 'select' instructions that - // are created from any PHI nodes, will take its debug location, giving - // the impression that those 'select' instructions are in the true branch, - // causing incorrect stepping, affecting the debug experience. - NT->setDebugLoc(InsertPt ? InsertPt->getDebugLoc() : DebugLoc()); - IRBuilder Builder(NT); // Hoisting one of the terminators from our successor is a great thing. // Unfortunately, the successors of the if/else blocks may have PHI nodes in diff --git a/test/CodeGen/X86/pr39187-g.ll b/test/CodeGen/X86/pr39187-g.ll deleted file mode 100644 index 46da64111224..000000000000 --- a/test/CodeGen/X86/pr39187-g.ll +++ /dev/null @@ -1,121 +0,0 @@ -; RUN: opt < %s -S -simplifycfg | FileCheck %s - -; SimplifyCFG can hoist any common code in the 'then' and 'else' blocks to -; the 'if' basic block. -; -; For the special case, when hoisting the terminator instruction, its debug -; location keep references to its basic block, causing the debug information -; to become ambiguous. It causes the debugger to display unreached lines. - -; Change the debug location associated with the hoisted instruction, to -; the debug location from the insertion point in the 'if' block. - -; The insertion point is the previous non-debug instruction before the -; terminator in the parent basic block of the hoisted instruction. - -; IR with '-g': -; -; [...] -; %frombool = zext i1 %cmp to i8, !dbg !26 -; call void @llvm.dbg.value(metadata i8 %frombool, metadata !15, metadata !DIExpression()), !dbg !26 -; call void @llvm.dbg.value(metadata i32 0, metadata !17, metadata !DIExpression()), !dbg !27 -; br i1 %cmp, label %if.then, label %if.else -; [...] -; -; Insertion point is: %frombool = zext i1 %cmp to i8, !dbg !26 - -; IR generated with: -; clang -S -g -gno-column-info -O2 -emit-llvm pr39187.cpp -o pr39187-g.ll -mllvm -opt-bisect-limit=10 - -; // pr39187.cpp -; int main() { -; volatile int foo = 0; -; -; int beards = 0; -; bool cond = foo == 4; -; int bar = 0; -; if (cond) -; beards = 8; -; else -; beards = 4; -; -; volatile bool face = cond; -; -; return face ? beards : 0; -; } - -; CHECK-LABEL: entry -; CHECK: %foo = alloca i32, align 4 -; CHECK: %face = alloca i8, align 1 -; CHECK: %foo.0..sroa_cast = bitcast i32* %foo to i8* -; CHECK: store volatile i32 0, i32* %foo, align 4 -; CHECK: %foo.0. = load volatile i32, i32* %foo, align 4, !dbg !16 -; CHECK: %cmp = icmp eq i32 %foo.0., 4, !dbg !16 -; CHECK: %frombool = zext i1 %cmp to i8, !dbg !16 -; CHECK: call void @llvm.dbg.value(metadata i8 %frombool, metadata !13, metadata !DIExpression()), !dbg !16 -; CHECK: call void @llvm.dbg.value(metadata i32 0, metadata !15, metadata !DIExpression()), !dbg !17 -; CHECK: %. = select i1 %cmp, i32 8, i32 4, !dbg !16 - -; ModuleID = 'pr39187.cpp' -source_filename = "pr39187.cpp" -target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128" -target triple = "x86_64-pc-linux-gnu" - -; Function Attrs: norecurse nounwind uwtable -define dso_local i32 @main() local_unnamed_addr #0 !dbg !7 { -entry: - %foo = alloca i32, align 4 - %face = alloca i8, align 1 - %foo.0..sroa_cast = bitcast i32* %foo to i8* - store volatile i32 0, i32* %foo, align 4 - %foo.0. = load volatile i32, i32* %foo, align 4, !dbg !26 - %cmp = icmp eq i32 %foo.0., 4, !dbg !26 - %frombool = zext i1 %cmp to i8, !dbg !26 - call void @llvm.dbg.value(metadata i8 %frombool, metadata !15, metadata !DIExpression()), !dbg !26 - call void @llvm.dbg.value(metadata i32 0, metadata !17, metadata !DIExpression()), !dbg !27 - br i1 %cmp, label %if.then, label %if.else - -if.then: ; preds = %entry - call void @llvm.dbg.value(metadata i32 8, metadata !14, metadata !DIExpression()), !dbg !25 - br label %if.end - -if.else: ; preds = %entry - call void @llvm.dbg.value(metadata i32 4, metadata !14, metadata !DIExpression()), !dbg !25 - br label %if.end - -if.end: ; preds = %if.else, %if.then - %beards.0 = phi i32 [ 8, %if.then ], [ 4, %if.else ] - store volatile i8 %frombool, i8* %face, align 1 - %face.0. = load volatile i8, i8* %face, align 1 - %0 = and i8 %face.0., 1 - %tobool3 = icmp eq i8 %0, 0 - %cond4 = select i1 %tobool3, i32 0, i32 %beards.0 - ret i32 %cond4 -} - -; Function Attrs: nounwind readnone speculatable -declare void @llvm.dbg.value(metadata, metadata, metadata) #2 - -!llvm.dbg.cu = !{!0} -!llvm.module.flags = !{!3, !4, !5} -!llvm.ident = !{!6} - -!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang version 8.0.0 (trunk 346301)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, nameTableKind: None) -!1 = !DIFile(filename: "pr39187.cpp", directory: ".") -!2 = !{} -!3 = !{i32 2, !"Dwarf Version", i32 4} -!4 = !{i32 2, !"Debug Info Version", i32 3} -!5 = !{i32 1, !"wchar_size", i32 4} -!6 = !{!"clang version 8.0.0 (trunk 346301)"} -!7 = distinct !DISubprogram(name: "main", scope: !1, file: !1, line: 1, type: !8, isLocal: false, isDefinition: true, scopeLine: 1, flags: DIFlagPrototyped, isOptimized: true, unit: !0, retainedNodes: !11) -!8 = !DISubroutineType(types: !9) -!9 = !{!10} -!10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed) -!11 = !{!14, !15, !17} -!14 = !DILocalVariable(name: "beards", scope: !7, file: !1, line: 4, type: !10) -!15 = !DILocalVariable(name: "cond", scope: !7, file: !1, line: 5, type: !16) -!16 = !DIBasicType(name: "bool", size: 8, encoding: DW_ATE_boolean) -!17 = !DILocalVariable(name: "bar", scope: !7, file: !1, line: 6, type: !10) -!25 = !DILocation(line: 4, scope: !7) -!26 = !DILocation(line: 5, scope: !7) -!27 = !DILocation(line: 6, scope: !7) From 63e77c80da91b42c25eeaa845e9358b3caf42043 Mon Sep 17 00:00:00 2001 From: Nikita Popov Date: Wed, 21 Nov 2018 19:37:19 +0000 Subject: [PATCH 2/2] [MergeFuncs] Generate alias instead of thunk if possible The MergeFunctions pass was originally intended to emit aliases instead of thunks where possible (unnamed_addr). However, for a long time this functionality was behind a flag hardcoded to false, bitrotted and was eventually removed in r309313. Originally the functionality was first disabled in r108417 due to lack of support for aliases in Mach-O. I believe that this is no longer the case nowadays, but not really familiar with this area. In the interest of being conservative, this patch reintroduces the aliasing functionality behind a default disabled -mergefunc-use-aliases flag. Differential Revision: https://reviews.llvm.org/D53285 git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@347407 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/Transforms/IPO/MergeFunctions.cpp | 87 +++++++++++++++---- test/Transforms/MergeFunc/alias.ll | 116 ++++++++++++++++++++++++++ 2 files changed, 189 insertions(+), 14 deletions(-) create mode 100644 test/Transforms/MergeFunc/alias.ll diff --git a/lib/Transforms/IPO/MergeFunctions.cpp b/lib/Transforms/IPO/MergeFunctions.cpp index 4c51cd131a10..e84de09c7e02 100644 --- a/lib/Transforms/IPO/MergeFunctions.cpp +++ b/lib/Transforms/IPO/MergeFunctions.cpp @@ -136,6 +136,7 @@ using namespace llvm; STATISTIC(NumFunctionsMerged, "Number of functions merged"); STATISTIC(NumThunksWritten, "Number of thunks generated"); +STATISTIC(NumAliasesWritten, "Number of aliases generated"); STATISTIC(NumDoubleWeak, "Number of new functions created"); static cl::opt NumFunctionsForSanityCheck( @@ -165,6 +166,11 @@ static cl::opt cl::desc("Preserve debug info in thunk when mergefunc " "transformations are made.")); +static cl::opt + MergeFunctionsAliases("mergefunc-use-aliases", cl::Hidden, + cl::init(false), + cl::desc("Allow mergefunc to create aliases")); + namespace { class FunctionNode { @@ -272,6 +278,13 @@ class MergeFunctions : public ModulePass { /// delete G. void writeThunk(Function *F, Function *G); + // Replace G with an alias to F (deleting function G) + void writeAlias(Function *F, Function *G); + + // Replace G with an alias to F if possible, or a thunk to F if + // profitable. Returns false if neither is the case. + bool writeThunkOrAlias(Function *F, Function *G); + /// Replace function F with function G in the function tree. void replaceFunctionInTree(const FunctionNode &FN, Function *G); @@ -735,27 +748,76 @@ void MergeFunctions::writeThunk(Function *F, Function *G) { ++NumThunksWritten; } +// Whether this function may be replaced by an alias +static bool canCreateAliasFor(Function *F) { + if (!MergeFunctionsAliases || !F->hasGlobalUnnamedAddr()) + return false; + + // We should only see linkages supported by aliases here + assert(F->hasLocalLinkage() || F->hasExternalLinkage() + || F->hasWeakLinkage() || F->hasLinkOnceLinkage()); + return true; +} + +// Replace G with an alias to F (deleting function G) +void MergeFunctions::writeAlias(Function *F, Function *G) { + Constant *BitcastF = ConstantExpr::getBitCast(F, G->getType()); + PointerType *PtrType = G->getType(); + auto *GA = GlobalAlias::create( + PtrType->getElementType(), PtrType->getAddressSpace(), + G->getLinkage(), "", BitcastF, G->getParent()); + + F->setAlignment(std::max(F->getAlignment(), G->getAlignment())); + GA->takeName(G); + GA->setVisibility(G->getVisibility()); + GA->setUnnamedAddr(GlobalValue::UnnamedAddr::Global); + + removeUsers(G); + G->replaceAllUsesWith(GA); + G->eraseFromParent(); + + LLVM_DEBUG(dbgs() << "writeAlias: " << GA->getName() << '\n'); + ++NumAliasesWritten; +} + +// Replace G with an alias to F if possible, or a thunk to F if +// profitable. Returns false if neither is the case. +bool MergeFunctions::writeThunkOrAlias(Function *F, Function *G) { + if (canCreateAliasFor(G)) { + writeAlias(F, G); + return true; + } + if (isThunkProfitable(F)) { + writeThunk(F, G); + return true; + } + return false; +} + // Merge two equivalent functions. Upon completion, Function G is deleted. void MergeFunctions::mergeTwoFunctions(Function *F, Function *G) { if (F->isInterposable()) { assert(G->isInterposable()); - if (!isThunkProfitable(F)) { + // Both writeThunkOrAlias() calls below must succeed, either because we can + // create aliases for G and NewF, or because a thunk for F is profitable. + // F here has the same signature as NewF below, so that's what we check. + if (!isThunkProfitable(F) && (!canCreateAliasFor(F) || !canCreateAliasFor(G))) { return; } // Make them both thunks to the same internal function. - Function *H = Function::Create(F->getFunctionType(), F->getLinkage(), "", - F->getParent()); - H->copyAttributesFrom(F); - H->takeName(F); + Function *NewF = Function::Create(F->getFunctionType(), F->getLinkage(), "", + F->getParent()); + NewF->copyAttributesFrom(F); + NewF->takeName(F); removeUsers(F); - F->replaceAllUsesWith(H); + F->replaceAllUsesWith(NewF); - unsigned MaxAlignment = std::max(G->getAlignment(), H->getAlignment()); + unsigned MaxAlignment = std::max(G->getAlignment(), NewF->getAlignment()); - writeThunk(F, G); - writeThunk(F, H); + writeThunkOrAlias(F, G); + writeThunkOrAlias(F, NewF); F->setAlignment(MaxAlignment); F->setLinkage(GlobalValue::PrivateLinkage); @@ -789,12 +851,9 @@ void MergeFunctions::mergeTwoFunctions(Function *F, Function *G) { return; } - if (!isThunkProfitable(F)) { - return; + if (writeThunkOrAlias(F, G)) { + ++NumFunctionsMerged; } - - writeThunk(F, G); - ++NumFunctionsMerged; } } diff --git a/test/Transforms/MergeFunc/alias.ll b/test/Transforms/MergeFunc/alias.ll new file mode 100644 index 000000000000..ee1c7af5e817 --- /dev/null +++ b/test/Transforms/MergeFunc/alias.ll @@ -0,0 +1,116 @@ +; RUN: opt -S -mergefunc -mergefunc-use-aliases < %s | FileCheck %s + +; Aliases should always be created for the weak functions, and +; for external functions if there is no local function + +; CHECK: @external_external_2 = unnamed_addr alias void (float*), bitcast (void (i32*)* @external_external_1 to void (float*)*) +; CHECK: @weak_weak_2 = weak unnamed_addr alias void (float*), bitcast (void (i32*)* @0 to void (float*)*) +; CHECK: @weak_weak_1 = weak unnamed_addr alias void (i32*), void (i32*)* @0 +; CHECK: @weak_external_1 = weak unnamed_addr alias void (i32*), bitcast (void (float*)* @weak_external_2 to void (i32*)*) +; CHECK: @external_weak_2 = weak unnamed_addr alias void (float*), bitcast (void (i32*)* @external_weak_1 to void (float*)*) +; CHECK: @weak_internal_1 = weak unnamed_addr alias void (i32*), bitcast (void (float*)* @weak_internal_2 to void (i32*)*) +; CHECK: @internal_weak_2 = weak unnamed_addr alias void (float*), bitcast (void (i32*)* @internal_weak_1 to void (float*)*) + +; A strong backing function had to be created for the weak-weak pair + +; CHECK: define private void @0(i32* %a) unnamed_addr +; CHECK_NEXT: call void @dummy4() + +; These internal functions are dropped in favor of the external ones + +; CHECK-NOT: define internal void @external_internal_2(float *%a) unnamed_addr +; CHECK-NOT: define internal void @internal_external_1(i32 *%a) unnamed_addr +; CHECK-NOT: define internal void @internal_external_1(i32 *%a) unnamed_addr +; CHECK-NOT: define internal void @internal_external_2(float *%a) unnamed_addr + +; Only used to mark which functions should be merged. +declare void @dummy1() +declare void @dummy2() +declare void @dummy3() +declare void @dummy4() +declare void @dummy5() +declare void @dummy6() +declare void @dummy7() +declare void @dummy8() +declare void @dummy9() + +define void @external_external_1(i32 *%a) unnamed_addr { + call void @dummy1() + ret void +} +define void @external_external_2(float *%a) unnamed_addr { + call void @dummy1() + ret void +} + +define void @external_internal_1(i32 *%a) unnamed_addr { + call void @dummy2() + ret void +} +define internal void @external_internal_2(float *%a) unnamed_addr { + call void @dummy2() + ret void +} + +define internal void @internal_external_1(i32 *%a) unnamed_addr { + call void @dummy3() + ret void +} +define void @internal_external_2(float *%a) unnamed_addr { + call void @dummy3() + ret void +} + +define weak void @weak_weak_1(i32 *%a) unnamed_addr { + call void @dummy4() + ret void +} +define weak void @weak_weak_2(float *%a) unnamed_addr { + call void @dummy4() + ret void +} + +define weak void @weak_external_1(i32 *%a) unnamed_addr { + call void @dummy5() + ret void +} +define external void @weak_external_2(float *%a) unnamed_addr { + call void @dummy5() + ret void +} + +define external void @external_weak_1(i32 *%a) unnamed_addr { + call void @dummy6() + ret void +} +define weak void @external_weak_2(float *%a) unnamed_addr { + call void @dummy6() + ret void +} + +define weak void @weak_internal_1(i32 *%a) unnamed_addr { + call void @dummy7() + ret void +} +define internal void @weak_internal_2(float *%a) unnamed_addr { + call void @dummy7() + ret void +} + +define internal void @internal_weak_1(i32 *%a) unnamed_addr { + call void @dummy8() + ret void +} +define weak void @internal_weak_2(float *%a) unnamed_addr { + call void @dummy8() + ret void +} + +define internal void @internal_internal_1(i32 *%a) unnamed_addr { + call void @dummy9() + ret void +} +define internal void @internal_internal_2(float *%a) unnamed_addr { + call void @dummy9() + ret void +}