Hi, These patches (for Clang and LLVM) together fix PR9614.
The Clang patch causes it to mark self calls to available_externally functions via an asm label as noinline, and the LLVM patch inhibits certain optimizations, including tail call elimination, for noinline calls to available_externally functions, forcing a call to the external version of the function. Any thoughts on this approach? Is this OK to commit? Thanks, -- Peter
>From 48d1282f911c8d73ddac6e9e46fd92f12b8fce1f Mon Sep 17 00:00:00 2001 From: Peter Collingbourne <[email protected]> Date: Fri, 7 Oct 2011 19:33:57 +0100 Subject: [PATCH] Modify the SCCP, TRE and function attribute passes and the call graph and inline cost analyses to treat noinline calls to available_externally functions as calls to an external version of the function with unknown semantics. LLVM side of fix for PR9614. --- lib/Analysis/IPA/CallGraph.cpp | 5 +++- lib/Analysis/InlineCost.cpp | 7 +++- lib/Transforms/IPO/FunctionAttrs.cpp | 8 ++++- lib/Transforms/Scalar/SCCP.cpp | 3 +- lib/Transforms/Scalar/TailRecursionElimination.cpp | 6 ++++ test/Transforms/FunctionAttrs/self-call.ll | 28 ++++++++++++++++++++ test/Transforms/SCCP/retvalue-undef.ll | 10 +++++++ test/Transforms/TailCallElim/inf-recursion.ll | 12 ++++++++ 8 files changed, 73 insertions(+), 6 deletions(-) create mode 100644 test/Transforms/FunctionAttrs/self-call.ll diff --git a/lib/Analysis/IPA/CallGraph.cpp b/lib/Analysis/IPA/CallGraph.cpp index 2e79eab..3fc326a 100644 --- a/lib/Analysis/IPA/CallGraph.cpp +++ b/lib/Analysis/IPA/CallGraph.cpp @@ -150,7 +150,10 @@ private: CallSite CS(cast<Value>(II)); if (CS && !isa<IntrinsicInst>(II)) { const Function *Callee = CS.getCalledFunction(); - if (Callee) + // noinline calls to available_externally functions are calls to the + // external function. + if (Callee && + !(CS.isNoInline() && Callee->hasAvailableExternallyLinkage())) Node->addCalledFunction(CS, getOrInsertFunction(Callee)); else Node->addCalledFunction(CS, CallsExternalNode); diff --git a/lib/Analysis/InlineCost.cpp b/lib/Analysis/InlineCost.cpp index e12e322..dcf8c52 100644 --- a/lib/Analysis/InlineCost.cpp +++ b/lib/Analysis/InlineCost.cpp @@ -78,8 +78,11 @@ void CodeMetrics::analyzeBasicBlock(const BasicBlock *BB, // If this call is to function itself, then the function is recursive. // Inlining it into other functions is a bad idea, because this is // basically just a form of loop peeling, and our metrics aren't useful - // for that case. - if (F == BB->getParent()) + // for that case. However, a noinline self-call in an + // available_externally function is a call to the external version of + // the function, not a recursive call. + if (F == BB->getParent() && + !(CS.isNoInline() && F->hasAvailableExternallyLinkage())) isRecursive = true; } diff --git a/lib/Transforms/IPO/FunctionAttrs.cpp b/lib/Transforms/IPO/FunctionAttrs.cpp index 0edf342..c4187dc 100644 --- a/lib/Transforms/IPO/FunctionAttrs.cpp +++ b/lib/Transforms/IPO/FunctionAttrs.cpp @@ -125,8 +125,12 @@ bool FunctionAttrs::AddReadAttrs(const CallGraphSCC &SCC) { // Detect these now, skipping to the next instruction if one is found. CallSite CS(cast<Value>(I)); if (CS) { - // Ignore calls to functions in the same SCC. - if (CS.getCalledFunction() && SCCNodes.count(CS.getCalledFunction())) + // Ignore calls to functions in the same SCC. A noinline call to + // an available_externally function is a call to the external function, + // which is never in the SCC. + llvm::Function *F = CS.getCalledFunction(); + if (F && !(CS.isNoInline() && F->hasAvailableExternallyLinkage()) && + SCCNodes.count(CS.getCalledFunction())) continue; AliasAnalysis::ModRefBehavior MRB = AA->getModRefBehavior(CS); // If the call doesn't access arbitrary memory, we may be able to diff --git a/lib/Transforms/Scalar/SCCP.cpp b/lib/Transforms/Scalar/SCCP.cpp index 196a847..b00b859 100644 --- a/lib/Transforms/Scalar/SCCP.cpp +++ b/lib/Transforms/Scalar/SCCP.cpp @@ -1266,7 +1266,8 @@ void SCCPSolver::visitCallSite(CallSite CS) { // The common case is that we aren't tracking the callee, either because we // are not doing interprocedural analysis or the callee is indirect, or is // external. Handle these cases first. - if (F == 0 || F->isDeclaration()) { + if (F == 0 || F->isDeclaration() || + (CS.isNoInline() && F->hasAvailableExternallyLinkage())) { CallOverdefined: // Void return and not tracking callee, just bail. if (I->getType()->isVoidTy()) return; diff --git a/lib/Transforms/Scalar/TailRecursionElimination.cpp b/lib/Transforms/Scalar/TailRecursionElimination.cpp index e21eb9d..bda7b23 100644 --- a/lib/Transforms/Scalar/TailRecursionElimination.cpp +++ b/lib/Transforms/Scalar/TailRecursionElimination.cpp @@ -384,6 +384,12 @@ TailCallElim::FindTRECandidate(Instruction *TI, if (CI->isTailCall() && CannotTailCallElimCallsMarkedTail) return 0; + // If this is a noinline call to an available_externally function, we cannot + // perform this optimization, because this is a call to the external version + // of the function, not a recursive call. + if (CI->isNoInline() && F->hasAvailableExternallyLinkage()) + return 0; + // As a special case, detect code like this: // double fabs(double f) { return __builtin_fabs(f); } // a 'fabs' call // and disable this xform in this case, because the code generator will diff --git a/test/Transforms/FunctionAttrs/self-call.ll b/test/Transforms/FunctionAttrs/self-call.ll new file mode 100644 index 0000000..7f1de8d --- /dev/null +++ b/test/Transforms/FunctionAttrs/self-call.ll @@ -0,0 +1,28 @@ +; RUN: opt < %s -functionattrs -S | FileCheck %s + +; The readonly or readnone attributes should not be added to an +; available_externally function containing only a self call. + +; CHECK-NOT: readnone +; CHECK-NOT: readonly +; CHECK: } +define available_externally void @a() { + call void @a() noinline + ret void +} + +; But readnone should be added to an ordinary self call in an +; available_externally function, or a noinline self call in a function with +; other linkage. + +; CHECK: @b{{.*}}readnone +define available_externally void @b() { + call void @b() + ret void +} + +; CHECK: @c{{.*}}readnone +define void @c() { + call void @c() noinline + ret void +} diff --git a/test/Transforms/SCCP/retvalue-undef.ll b/test/Transforms/SCCP/retvalue-undef.ll index 389561f..b671669 100644 --- a/test/Transforms/SCCP/retvalue-undef.ll +++ b/test/Transforms/SCCP/retvalue-undef.ll @@ -30,3 +30,13 @@ declare void @register_outer_mod(void ()*) define i32 @main() { ret i32 0 } + +;PR9614 + +; CHECK: define available_externally i32 @btowc +; CHECK: ret i32 %call +define available_externally i32 @btowc(i32 %__c) { +entry: + %call = tail call i32 @btowc(i32 %__c) noinline + ret i32 %call +} diff --git a/test/Transforms/TailCallElim/inf-recursion.ll b/test/Transforms/TailCallElim/inf-recursion.ll index c427869..a9b2f8d 100644 --- a/test/Transforms/TailCallElim/inf-recursion.ll +++ b/test/Transforms/TailCallElim/inf-recursion.ll @@ -12,6 +12,18 @@ entry: ret double %tmp2 } +; Don't turn this into an infinite loop either, the call is to the external +; version of btowc. +; CHECK: @btowc(i32 %i) +; CHECK: call +; CHECK: ret + +define available_externally i32 @btowc(i32 %i) { +entry: + %tmp2 = call i32 @btowc(i32 %i) noinline + ret i32 %tmp2 +} + ; Do turn other calls into infinite loops though. ; CHECK: define double @foo -- 1.7.5.3
>From cf21fa99850d21bfa941921b0f69ec88fc9faa15 Mon Sep 17 00:00:00 2001 From: Peter Collingbourne <[email protected]> Date: Sun, 9 Oct 2011 04:45:57 +0100 Subject: [PATCH] Mark self calls to available_externally functions via an asm label as noinline, which forces an external call and inhibits certain optimizations, including tail call elimination. Clang side of fix for PR9614. --- lib/CodeGen/CGCall.cpp | 12 ++++++++++++ test/CodeGen/asm-label-self-call.c | 12 ++++++++++++ 2 files changed, 24 insertions(+), 0 deletions(-) create mode 100644 test/CodeGen/asm-label-self-call.c diff --git a/lib/CodeGen/CGCall.cpp b/lib/CodeGen/CGCall.cpp index f52abef..46df8b4 100644 --- a/lib/CodeGen/CGCall.cpp +++ b/lib/CodeGen/CGCall.cpp @@ -1767,6 +1767,18 @@ RValue CodeGenFunction::EmitCall(const CGFunctionInfo &CallInfo, CS.setAttributes(Attrs); CS.setCallingConv(static_cast<llvm::CallingConv::ID>(CallingConv)); + // Some extern inline functions defined in glibc headers call themselves + // via an asm labelled function, in an attempt to force a call to the + // external version of the function. Since asm labels are resolved earlier + // in LLVM than in gcc, we see through this trick, and emit a self call + // which the optimizer would otherwise translate into an infinite loop or + // undefined behavior. Emulate the gcc behavior by marking self calls + // to available_externally functions via an asm label as noinline, which + // forces an external call and inhibits certain optimizations. + if (Callee == CurFn && CurFn->hasAvailableExternallyLinkage() && + TargetDecl->hasAttr<AsmLabelAttr>()) + CS.setIsNoInline(true); + // If the call doesn't return, finish the basic block and clear the // insertion point; this allows the rest of IRgen to discard // unreachable code. diff --git a/test/CodeGen/asm-label-self-call.c b/test/CodeGen/asm-label-self-call.c new file mode 100644 index 0000000..0eb2434 --- /dev/null +++ b/test/CodeGen/asm-label-self-call.c @@ -0,0 +1,12 @@ +// RUN: %clang_cc1 -triple=i686-pc-linux-gnu -emit-llvm -O3 %s -o - | FileCheck %s + +typedef __typeof(+L'a') wint_t; +extern wint_t __btowc_alias (int __c) __asm ("btowc"); +__attribute((nothrow)) inline wint_t btowc (int __c) { + return __btowc_alias (__c); +} + +// CHECK: define i32 @f +// CHECK-NEXT: : +// CHECK-NEXT: call i32 @btowc{{.*}} noinline +int f(int x) { return btowc(x); } -- 1.7.5.3
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
