From eea47794b2e8ab41dc2146663ab3a3d3aea7dcef Mon Sep 17 00:00:00 2001 From: Alexey Bataev Date: Wed, 7 Mar 2018 18:17:06 +0000 Subject: [PATCH] [OPENMP] Fix lifetime of the loop counters. We may emit incorrect lifetime info during codegen for loop counters in OpenMP constructs because of automatic scope cleanup when we needed temporarily locations for private loop counters. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@326922 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/CodeGen/CGStmtOpenMP.cpp | 36 ++++----- lib/CodeGen/CodeGenFunction.h | 142 ++++++++++++++++++++-------------- test/OpenMP/for_codegen.cpp | 14 +++- 3 files changed, 115 insertions(+), 77 deletions(-) diff --git a/lib/CodeGen/CGStmtOpenMP.cpp b/lib/CodeGen/CGStmtOpenMP.cpp index d9b4fa83a9..8d1f9c6f7b 100644 --- a/lib/CodeGen/CGStmtOpenMP.cpp +++ b/lib/CodeGen/CGStmtOpenMP.cpp @@ -120,18 +120,18 @@ public: /// of used expression from loop statement. class OMPLoopScope : public CodeGenFunction::RunCleanupsScope { void emitPreInitStmt(CodeGenFunction &CGF, const OMPLoopDirective &S) { - CodeGenFunction::OMPPrivateScope PreCondScope(CGF); + CodeGenFunction::OMPMapVars PreCondVars; for (auto *E : S.counters()) { const auto *VD = cast(cast(E)->getDecl()); - (void)PreCondScope.addPrivate(VD, [&CGF, VD]() { - return CGF.CreateMemTemp(VD->getType().getNonReferenceType()); - }); + (void)PreCondVars.setVarAddr( + CGF, VD, CGF.CreateMemTemp(VD->getType().getNonReferenceType())); } - (void)PreCondScope.Privatize(); + (void)PreCondVars.apply(CGF); if (auto *PreInits = cast_or_null(S.getPreInits())) { for (const auto *I : PreInits->decls()) CGF.EmitVarDecl(cast(*I)); } + PreCondVars.restore(CGF); } public: @@ -1475,25 +1475,25 @@ void CodeGenFunction::EmitOMPPrivateLoopCounters( for (auto *E : S.counters()) { auto *VD = cast(cast(E)->getDecl()); auto *PrivateVD = cast(cast(*I)->getDecl()); - (void)LoopScope.addPrivate(VD, [&]() -> Address { - // Emit var without initialization. - if (!LocalDeclMap.count(PrivateVD)) { - auto VarEmission = EmitAutoVarAlloca(*PrivateVD); - EmitAutoVarCleanups(VarEmission); - } - DeclRefExpr DRE(const_cast(PrivateVD), - /*RefersToEnclosingVariableOrCapture=*/false, - (*I)->getType(), VK_LValue, (*I)->getExprLoc()); - return EmitLValue(&DRE).getAddress(); + // Emit var without initialization. + auto VarEmission = EmitAutoVarAlloca(*PrivateVD); + EmitAutoVarCleanups(VarEmission); + LocalDeclMap.erase(PrivateVD); + (void)LoopScope.addPrivate(VD, [&VarEmission]() { + return VarEmission.getAllocatedAddress(); }); if (LocalDeclMap.count(VD) || CapturedStmtInfo->lookup(VD) || VD->hasGlobalStorage()) { - (void)LoopScope.addPrivate(PrivateVD, [&]() -> Address { + (void)LoopScope.addPrivate(PrivateVD, [this, VD, E]() { DeclRefExpr DRE(const_cast(VD), LocalDeclMap.count(VD) || CapturedStmtInfo->lookup(VD), E->getType(), VK_LValue, E->getExprLoc()); return EmitLValue(&DRE).getAddress(); }); + } else { + (void)LoopScope.addPrivate(PrivateVD, [&VarEmission]() { + return VarEmission.getAllocatedAddress(); + }); } ++I; } @@ -1611,9 +1611,9 @@ void CodeGenFunction::EmitOMPSimdFinal( } } Address OrigAddr = Address::invalid(); - if (CED) + if (CED) { OrigAddr = EmitLValue(CED->getInit()->IgnoreImpCasts()).getAddress(); - else { + } else { DeclRefExpr DRE(const_cast(PrivateVD), /*RefersToEnclosingVariableOrCapture=*/false, (*IPC)->getType(), VK_LValue, (*IPC)->getExprLoc()); diff --git a/lib/CodeGen/CodeGenFunction.h b/lib/CodeGen/CodeGenFunction.h index ca2a8d9192..19deb9bb14 100644 --- a/lib/CodeGen/CodeGenFunction.h +++ b/lib/CodeGen/CodeGenFunction.h @@ -693,57 +693,107 @@ public: typedef llvm::DenseMap DeclMapTy; - /// \brief The scope used to remap some variables as private in the OpenMP - /// loop body (or other captured region emitted without outlining), and to - /// restore old vars back on exit. - class OMPPrivateScope : public RunCleanupsScope { + /// The class used to assign some variables some temporarily addresses. + class OMPMapVars { DeclMapTy SavedLocals; - DeclMapTy SavedPrivates; - - private: - OMPPrivateScope(const OMPPrivateScope &) = delete; - void operator=(const OMPPrivateScope &) = delete; + DeclMapTy SavedTempAddresses; + OMPMapVars(const OMPMapVars &) = delete; + void operator=(const OMPMapVars &) = delete; public: - /// \brief Enter a new OpenMP private scope. - explicit OMPPrivateScope(CodeGenFunction &CGF) : RunCleanupsScope(CGF) {} - - /// \brief Registers \a LocalVD variable as a private and apply \a - /// PrivateGen function for it to generate corresponding private variable. - /// \a PrivateGen returns an address of the generated private variable. - /// \return true if the variable is registered as private, false if it has - /// been privatized already. - bool - addPrivate(const VarDecl *LocalVD, - llvm::function_ref PrivateGen) { - assert(PerformCleanup && "adding private to dead scope"); + explicit OMPMapVars() = default; + ~OMPMapVars() { + assert(SavedLocals.empty() && "Did not restored original addresses."); + }; + /// Sets the address of the variable \p LocalVD to be \p TempAddr in + /// function \p CGF. + /// \return true if at least one variable was set already, false otherwise. + bool setVarAddr(CodeGenFunction &CGF, const VarDecl *LocalVD, + Address TempAddr) { LocalVD = LocalVD->getCanonicalDecl(); // Only save it once. if (SavedLocals.count(LocalVD)) return false; // Copy the existing local entry to SavedLocals. auto it = CGF.LocalDeclMap.find(LocalVD); - if (it != CGF.LocalDeclMap.end()) { - SavedLocals.insert({LocalVD, it->second}); - } else { - SavedLocals.insert({LocalVD, Address::invalid()}); - } + if (it != CGF.LocalDeclMap.end()) + SavedLocals.try_emplace(LocalVD, it->second); + else + SavedLocals.try_emplace(LocalVD, Address::invalid()); // Generate the private entry. - Address Addr = PrivateGen(); QualType VarTy = LocalVD->getType(); if (VarTy->isReferenceType()) { Address Temp = CGF.CreateMemTemp(VarTy); - CGF.Builder.CreateStore(Addr.getPointer(), Temp); - Addr = Temp; + CGF.Builder.CreateStore(TempAddr.getPointer(), Temp); + TempAddr = Temp; } - SavedPrivates.insert({LocalVD, Addr}); + SavedTempAddresses.try_emplace(LocalVD, TempAddr); return true; } - /// \brief Privatizes local variables previously registered as private. + /// Applies new addresses to the list of the variables. + /// \return true if at least one variable is using new address, false + /// otherwise. + bool apply(CodeGenFunction &CGF) { + copyInto(SavedTempAddresses, CGF.LocalDeclMap); + SavedTempAddresses.clear(); + return !SavedLocals.empty(); + } + + /// Restores original addresses of the variables. + void restore(CodeGenFunction &CGF) { + if (!SavedLocals.empty()) { + copyInto(SavedLocals, CGF.LocalDeclMap); + SavedLocals.clear(); + } + } + + private: + /// Copy all the entries in the source map over the corresponding + /// entries in the destination, which must exist. + static void copyInto(const DeclMapTy &Src, DeclMapTy &Dest) { + for (auto &Pair : Src) { + if (!Pair.second.isValid()) { + Dest.erase(Pair.first); + continue; + } + + auto I = Dest.find(Pair.first); + if (I != Dest.end()) + I->second = Pair.second; + else + Dest.insert(Pair); + } + } + }; + + /// The scope used to remap some variables as private in the OpenMP loop body + /// (or other captured region emitted without outlining), and to restore old + /// vars back on exit. + class OMPPrivateScope : public RunCleanupsScope { + OMPMapVars MappedVars; + OMPPrivateScope(const OMPPrivateScope &) = delete; + void operator=(const OMPPrivateScope &) = delete; + + public: + /// Enter a new OpenMP private scope. + explicit OMPPrivateScope(CodeGenFunction &CGF) : RunCleanupsScope(CGF) {} + + /// Registers \p LocalVD variable as a private and apply \p PrivateGen + /// function for it to generate corresponding private variable. \p + /// PrivateGen returns an address of the generated private variable. + /// \return true if the variable is registered as private, false if it has + /// been privatized already. + bool addPrivate(const VarDecl *LocalVD, + llvm::function_ref PrivateGen) { + assert(PerformCleanup && "adding private to dead scope"); + return MappedVars.setVarAddr(CGF, LocalVD, PrivateGen()); + } + + /// Privatizes local variables previously registered as private. /// Registration is separate from the actual privatization to allow /// initializers use values of the original variables, not the private one. /// This is important, for example, if the private variable is a class @@ -751,19 +801,14 @@ public: /// variables. But at initialization original variables must be used, not /// private copies. /// \return true if at least one variable was privatized, false otherwise. - bool Privatize() { - copyInto(SavedPrivates, CGF.LocalDeclMap); - SavedPrivates.clear(); - return !SavedLocals.empty(); - } + bool Privatize() { return MappedVars.apply(CGF); } void ForceCleanup() { RunCleanupsScope::ForceCleanup(); - copyInto(SavedLocals, CGF.LocalDeclMap); - SavedLocals.clear(); + MappedVars.restore(CGF); } - /// \brief Exit scope - all the mapped variables are restored. + /// Exit scope - all the mapped variables are restored. ~OMPPrivateScope() { if (PerformCleanup) ForceCleanup(); @@ -774,25 +819,6 @@ public: VD = VD->getCanonicalDecl(); return !VD->isLocalVarDeclOrParm() && CGF.LocalDeclMap.count(VD) > 0; } - - private: - /// Copy all the entries in the source map over the corresponding - /// entries in the destination, which must exist. - static void copyInto(const DeclMapTy &src, DeclMapTy &dest) { - for (auto &pair : src) { - if (!pair.second.isValid()) { - dest.erase(pair.first); - continue; - } - - auto it = dest.find(pair.first); - if (it != dest.end()) { - it->second = pair.second; - } else { - dest.insert(pair); - } - } - } }; /// \brief Takes the old cleanup stack size and emits the cleanup blocks diff --git a/test/OpenMP/for_codegen.cpp b/test/OpenMP/for_codegen.cpp index 7c295a0b8f..e26bbaaf47 100644 --- a/test/OpenMP/for_codegen.cpp +++ b/test/OpenMP/for_codegen.cpp @@ -1,4 +1,4 @@ -// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple x86_64-unknown-unknown -emit-llvm %s -fexceptions -fcxx-exceptions -o - | FileCheck %s +// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple x86_64-unknown-unknown -emit-llvm %s -fexceptions -fcxx-exceptions -o - -fsanitize-address-use-after-scope | FileCheck %s --check-prefix=CHECK --check-prefix=LIFETIME // RUN: %clang_cc1 -fopenmp -x c++ -std=c++11 -triple x86_64-unknown-unknown -fexceptions -fcxx-exceptions -emit-pch -o %t %s // RUN: %clang_cc1 -fopenmp -x c++ -triple x86_64-unknown-unknown -fexceptions -fcxx-exceptions -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s // RUN: %clang_cc1 -verify -triple x86_64-apple-darwin10 -fopenmp -fexceptions -fcxx-exceptions -debug-info-kind=line-tables-only -x c++ -emit-llvm %s -o - | FileCheck %s --check-prefix=TERM_DEBUG @@ -25,8 +25,20 @@ // CHECK-LABEL: loop_with_counter_collapse void loop_with_counter_collapse() { + // LIFETIME: call void @llvm.lifetime.end + // LIFETIME: call void @llvm.lifetime.end // CHECK: call void @__kmpc_for_static_init_8(%ident_t* @ // CHECK: call void @__kmpc_for_static_fini(%ident_t* @ + // LIFETIME: call void @llvm.lifetime.end + // LIFETIME: call void @llvm.lifetime.end + // LIFETIME: call void @llvm.lifetime.end + // LIFETIME: call void @llvm.lifetime.end + // LIFETIME: call void @llvm.lifetime.end + // LIFETIME: call void @llvm.lifetime.end + // LIFETIME: call void @llvm.lifetime.end + // LIFETIME: call void @llvm.lifetime.end + // LIFETIME: call void @llvm.lifetime.end + // LIFETIME: call void @llvm.lifetime.end #pragma omp for collapse(2) for (int i = 0; i < 4; i++) { for (int j = i; j < 4; j++) { -- 2.50.1