From fdbb91ad22bdf26c03592dcd5c588f7d831520d0 Mon Sep 17 00:00:00 2001 From: Johannes Doerfert Date: Thu, 28 May 2020 09:41:01 -0500 Subject: [PATCH] Reuse OMPIRBuilder `struct ident_t` handling in Clang Replace the `ident_t` handling in Clang with the methods offered by the OMPIRBuilder. This cuts down on the clang code as well as the differences between the two, making further transitions easier. Tests have changed but there should not be a real functional change. The most interesting difference is probably that we stop generating local ident_t allocations for now and just use globals. Given that this happens only with debug info, the location part of the `ident_t` is probably bigger than the test anyway. As the location part is already a global, we can avoid the allocation, memcpy, and store in favor of a constant global that is slightly bigger. This can be revisited if there are complications. Reviewed By: ABataev Differential Revision: https://reviews.llvm.org/D80735 --- include/llvm/Frontend/OpenMP/OMPIRBuilder.h | 11 +++- include/llvm/IR/IRBuilder.h | 14 +++-- lib/Frontend/OpenMP/OMPIRBuilder.cpp | 60 +++++++++++++-------- lib/IR/IRBuilder.cpp | 13 ++--- test/Transforms/OpenMP/deduplication.ll | 14 ++--- 5 files changed, 73 insertions(+), 39 deletions(-) diff --git a/include/llvm/Frontend/OpenMP/OMPIRBuilder.h b/include/llvm/Frontend/OpenMP/OMPIRBuilder.h index 95eed59f1b3..a2a440d65fd 100644 --- a/include/llvm/Frontend/OpenMP/OMPIRBuilder.h +++ b/include/llvm/Frontend/OpenMP/OMPIRBuilder.h @@ -210,12 +210,19 @@ public: /// Return the (LLVM-IR) string describing the default source location. Constant *getOrCreateDefaultSrcLocStr(); + /// Return the (LLVM-IR) string describing the source location identified by + /// the arguments. + Constant *getOrCreateSrcLocStr(StringRef FunctionName, StringRef FileName, + unsigned Line, unsigned Column); + /// Return the (LLVM-IR) string describing the source location \p Loc. Constant *getOrCreateSrcLocStr(const LocationDescription &Loc); /// Return an ident_t* encoding the source location \p SrcLocStr and \p Flags. + /// TODO: Create a enum class for the Reserve2Flags Value *getOrCreateIdent(Constant *SrcLocStr, - omp::IdentFlag Flags = omp::IdentFlag(0)); + omp::IdentFlag Flags = omp::IdentFlag(0), + unsigned Reserve2Flags = 0); /// Generate control flow and cleanup for cancellation. /// @@ -280,7 +287,7 @@ public: StringMap SrcLocStrMap; /// Map to remember existing ident_t*. - DenseMap, GlobalVariable *> IdentMap; + DenseMap, Value *> IdentMap; /// Helper that contains information about regions we need to outline /// during finalization. diff --git a/include/llvm/IR/IRBuilder.h b/include/llvm/IR/IRBuilder.h index ffec4ff64ca..b90480ebc59 100644 --- a/include/llvm/IR/IRBuilder.h +++ b/include/llvm/IR/IRBuilder.h @@ -386,8 +386,12 @@ public: /// filled in with the null terminated string value specified. The new global /// variable will be marked mergable with any others of the same contents. If /// Name is specified, it is the name of the global variable created. + /// + /// If no module is given via \p M, it is take from the insertion point basic + /// block. GlobalVariable *CreateGlobalString(StringRef Str, const Twine &Name = "", - unsigned AddressSpace = 0); + unsigned AddressSpace = 0, + Module *M = nullptr); /// Get a constant value representing either true or false. ConstantInt *getInt1(bool V) { @@ -1934,9 +1938,13 @@ public: /// Same as CreateGlobalString, but return a pointer with "i8*" type /// instead of a pointer to array of i8. + /// + /// If no module is given via \p M, it is take from the insertion point basic + /// block. Constant *CreateGlobalStringPtr(StringRef Str, const Twine &Name = "", - unsigned AddressSpace = 0) { - GlobalVariable *GV = CreateGlobalString(Str, Name, AddressSpace); + unsigned AddressSpace = 0, + Module *M = nullptr) { + GlobalVariable *GV = CreateGlobalString(Str, Name, AddressSpace, M); Constant *Zero = ConstantInt::get(Type::getInt32Ty(Context), 0); Constant *Indices[] = {Zero, Zero}; return ConstantExpr::getInBoundsGetElementPtr(GV->getValueType(), GV, diff --git a/lib/Frontend/OpenMP/OMPIRBuilder.cpp b/lib/Frontend/OpenMP/OMPIRBuilder.cpp index 9468a3aa3c8..6c72cd01ce6 100644 --- a/lib/Frontend/OpenMP/OMPIRBuilder.cpp +++ b/lib/Frontend/OpenMP/OMPIRBuilder.cpp @@ -185,16 +185,18 @@ void OpenMPIRBuilder::finalize() { } Value *OpenMPIRBuilder::getOrCreateIdent(Constant *SrcLocStr, - IdentFlag LocFlags) { + IdentFlag LocFlags, + unsigned Reserve2Flags) { // Enable "C-mode". LocFlags |= OMP_IDENT_FLAG_KMPC; - GlobalVariable *&DefaultIdent = IdentMap[{SrcLocStr, uint64_t(LocFlags)}]; - if (!DefaultIdent) { + Value *&Ident = + IdentMap[{SrcLocStr, uint64_t(LocFlags) << 31 | Reserve2Flags}]; + if (!Ident) { Constant *I32Null = ConstantInt::getNullValue(Int32); - Constant *IdentData[] = {I32Null, - ConstantInt::get(Int32, uint64_t(LocFlags)), - I32Null, I32Null, SrcLocStr}; + Constant *IdentData[] = { + I32Null, ConstantInt::get(Int32, uint32_t(LocFlags)), + ConstantInt::get(Int32, Reserve2Flags), I32Null, SrcLocStr}; Constant *Initializer = ConstantStruct::get( cast(IdentPtr->getPointerElementType()), IdentData); @@ -203,15 +205,16 @@ Value *OpenMPIRBuilder::getOrCreateIdent(Constant *SrcLocStr, for (GlobalVariable &GV : M.getGlobalList()) if (GV.getType() == IdentPtr && GV.hasInitializer()) if (GV.getInitializer() == Initializer) - return DefaultIdent = &GV; + return Ident = &GV; - DefaultIdent = new GlobalVariable(M, IdentPtr->getPointerElementType(), - /* isConstant = */ false, - GlobalValue::PrivateLinkage, Initializer); - DefaultIdent->setUnnamedAddr(GlobalValue::UnnamedAddr::Global); - DefaultIdent->setAlignment(Align(8)); + auto *GV = new GlobalVariable(M, IdentPtr->getPointerElementType(), + /* isConstant = */ true, + GlobalValue::PrivateLinkage, Initializer); + GV->setUnnamedAddr(GlobalValue::UnnamedAddr::Global); + GV->setAlignment(Align(8)); + Ident = GV; } - return DefaultIdent; + return Ident; } Constant *OpenMPIRBuilder::getOrCreateSrcLocStr(StringRef LocStr) { @@ -227,11 +230,30 @@ Constant *OpenMPIRBuilder::getOrCreateSrcLocStr(StringRef LocStr) { GV.getInitializer() == Initializer) return SrcLocStr = ConstantExpr::getPointerCast(&GV, Int8Ptr); - SrcLocStr = Builder.CreateGlobalStringPtr(LocStr); + SrcLocStr = Builder.CreateGlobalStringPtr(LocStr, /* Name */ "", + /* AddressSpace */ 0, &M); } return SrcLocStr; } +Constant *OpenMPIRBuilder::getOrCreateSrcLocStr(StringRef FunctionName, + StringRef FileName, + unsigned Line, + unsigned Column) { + SmallString<128> Buffer; + Buffer.push_back(';'); + Buffer.append(FileName); + Buffer.push_back(';'); + Buffer.append(FunctionName); + Buffer.push_back(';'); + Buffer.append(std::to_string(Line)); + Buffer.push_back(';'); + Buffer.append(std::to_string(Column)); + Buffer.push_back(';'); + Buffer.push_back(';'); + return getOrCreateSrcLocStr(Buffer.str()); +} + Constant *OpenMPIRBuilder::getOrCreateDefaultSrcLocStr() { return getOrCreateSrcLocStr(";unknown;unknown;0;0;;"); } @@ -241,17 +263,13 @@ OpenMPIRBuilder::getOrCreateSrcLocStr(const LocationDescription &Loc) { DILocation *DIL = Loc.DL.get(); if (!DIL) return getOrCreateDefaultSrcLocStr(); - StringRef Filename = + StringRef FileName = !DIL->getFilename().empty() ? DIL->getFilename() : M.getName(); StringRef Function = DIL->getScope()->getSubprogram()->getName(); Function = !Function.empty() ? Function : Loc.IP.getBlock()->getParent()->getName(); - std::string LineStr = std::to_string(DIL->getLine()); - std::string ColumnStr = std::to_string(DIL->getColumn()); - std::stringstream SrcLocStr; - SrcLocStr << ";" << Filename.data() << ";" << Function.data() << ";" - << LineStr << ";" << ColumnStr << ";;"; - return getOrCreateSrcLocStr(SrcLocStr.str()); + return getOrCreateSrcLocStr(Function, FileName, DIL->getLine(), + DIL->getColumn()); } Value *OpenMPIRBuilder::getOrCreateThreadID(Value *Ident) { diff --git a/lib/IR/IRBuilder.cpp b/lib/IR/IRBuilder.cpp index 1fffce015f7..a82f1589578 100644 --- a/lib/IR/IRBuilder.cpp +++ b/lib/IR/IRBuilder.cpp @@ -42,13 +42,14 @@ using namespace llvm; /// created. GlobalVariable *IRBuilderBase::CreateGlobalString(StringRef Str, const Twine &Name, - unsigned AddressSpace) { + unsigned AddressSpace, + Module *M) { Constant *StrConstant = ConstantDataArray::getString(Context, Str); - Module &M = *BB->getParent()->getParent(); - auto *GV = new GlobalVariable(M, StrConstant->getType(), true, - GlobalValue::PrivateLinkage, StrConstant, Name, - nullptr, GlobalVariable::NotThreadLocal, - AddressSpace); + if (!M) + M = BB->getParent()->getParent(); + auto *GV = new GlobalVariable( + *M, StrConstant->getType(), true, GlobalValue::PrivateLinkage, + StrConstant, Name, nullptr, GlobalVariable::NotThreadLocal, AddressSpace); GV->setUnnamedAddr(GlobalValue::UnnamedAddr::Global); GV->setAlignment(Align(1)); return GV; diff --git a/test/Transforms/OpenMP/deduplication.ll b/test/Transforms/OpenMP/deduplication.ll index a25d980b180..9074b948cc3 100644 --- a/test/Transforms/OpenMP/deduplication.ll +++ b/test/Transforms/OpenMP/deduplication.ll @@ -5,21 +5,21 @@ target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16 %struct.ident_t = type { i32, i32, i32, i32, i8* } -@0 = private unnamed_addr global %struct.ident_t { i32 0, i32 34, i32 0, i32 0, i8* getelementptr inbounds ([23 x i8], [23 x i8]* @.str0, i32 0, i32 0) }, align 8 -@1 = private unnamed_addr global %struct.ident_t { i32 0, i32 2, i32 0, i32 0, i8* getelementptr inbounds ([23 x i8], [23 x i8]* @.str1, i32 0, i32 0) }, align 8 -@2 = private unnamed_addr global %struct.ident_t { i32 0, i32 2, i32 0, i32 0, i8* getelementptr inbounds ([23 x i8], [23 x i8]* @.str2, i32 0, i32 0) }, align 8 +@0 = private unnamed_addr constant %struct.ident_t { i32 0, i32 34, i32 0, i32 0, i8* getelementptr inbounds ([23 x i8], [23 x i8]* @.str0, i32 0, i32 0) }, align 8 +@1 = private unnamed_addr constant %struct.ident_t { i32 0, i32 2, i32 0, i32 0, i8* getelementptr inbounds ([23 x i8], [23 x i8]* @.str1, i32 0, i32 0) }, align 8 +@2 = private unnamed_addr constant %struct.ident_t { i32 0, i32 2, i32 0, i32 0, i8* getelementptr inbounds ([23 x i8], [23 x i8]* @.str2, i32 0, i32 0) }, align 8 @.str0 = private unnamed_addr constant [23 x i8] c";unknown;unknown;0;0;;\00", align 1 @.str1 = private unnamed_addr constant [23 x i8] c";file001;loc0001;0;0;;\00", align 1 @.str2 = private unnamed_addr constant [23 x i8] c";file002;loc0002;0;0;;\00", align 1 ; UTC_ARGS: --disable -; CHECK-DAG: @0 = private unnamed_addr global %struct.ident_t { i32 0, i32 34, i32 0, i32 0, i8* getelementptr inbounds ([23 x i8], [23 x i8]* @.str0, i32 0, i32 0) }, align 8 -; CHECK-DAG: @1 = private unnamed_addr global %struct.ident_t { i32 0, i32 2, i32 0, i32 0, i8* getelementptr inbounds ([23 x i8], [23 x i8]* @.str1, i32 0, i32 0) }, align 8 -; CHECK-DAG: @2 = private unnamed_addr global %struct.ident_t { i32 0, i32 2, i32 0, i32 0, i8* getelementptr inbounds ([23 x i8], [23 x i8]* @.str2, i32 0, i32 0) }, align 8 +; CHECK-DAG: @0 = private unnamed_addr constant %struct.ident_t { i32 0, i32 34, i32 0, i32 0, i8* getelementptr inbounds ([23 x i8], [23 x i8]* @.str0, i32 0, i32 0) }, align 8 +; CHECK-DAG: @1 = private unnamed_addr constant %struct.ident_t { i32 0, i32 2, i32 0, i32 0, i8* getelementptr inbounds ([23 x i8], [23 x i8]* @.str1, i32 0, i32 0) }, align 8 +; CHECK-DAG: @2 = private unnamed_addr constant %struct.ident_t { i32 0, i32 2, i32 0, i32 0, i8* getelementptr inbounds ([23 x i8], [23 x i8]* @.str2, i32 0, i32 0) }, align 8 ; CHECK-DAG: @.str0 = private unnamed_addr constant [23 x i8] c";unknown;unknown;0;0;;\00", align 1 ; CHECK-DAG: @.str1 = private unnamed_addr constant [23 x i8] c";file001;loc0001;0;0;;\00", align 1 ; CHECK-DAG: @.str2 = private unnamed_addr constant [23 x i8] c";file002;loc0002;0;0;;\00", align 1 -; CHECK-DAG: @3 = private unnamed_addr global %struct.ident_t { i32 0, i32 2, i32 0, i32 0, i8* getelementptr inbounds ([23 x i8], [23 x i8]* @.str0, i32 0, i32 0) }, align 8 +; CHECK-DAG: @3 = private unnamed_addr constant %struct.ident_t { i32 0, i32 2, i32 0, i32 0, i8* getelementptr inbounds ([23 x i8], [23 x i8]* @.str0, i32 0, i32 0) }, align 8 ; UTC_ARGS: --enable