Use ProfileData/InstrProfData.inc template file to define PGO runtime types

[davidxl]

Summary

In r250574, a template file is introduced to be a centralized place to define core PGO data structures that are shared between runtime, reader, and writer. In this patch, the file is used to replace 'hard' coded definitions so that they can always be in sync. Changes in the future will only (mostly) need to touch one file only (the template file).

A couple of notes:

1. clang FE change to use the the template file (will be in a different patch)
2. find a way to share header files between LLVM compiler and compiler_rt.
3. more portable way to specify packed struct.

[davidxl]

Update patch and ping ..

[vsk]

Thanks for the cleanup. Some inline comments --

include/llvm/ProfileData/InstrProf.h

434 ↗

(On Diff #38973)

In 'InstrProfData.inc', the macro parameters names are a bit more descriptive. Could you use the same names here?

458 ↗

(On Diff #38973)

Same nit as above.

lib/ProfileData/CoverageMappingReader.cpp

369 ↗

(On Diff #38973)

Could you make the l.h.s auto *CFR?

370 ↗

(On Diff #38973)

I notice that the old code used unaligned accesses when unpacking FunBuf.

I think having an assertion here for the assumed alignment would make for good future-proofing. Could we assert that CFR->FuncHash is 8-byte aligned?

[davidxl]

lib/ProfileData/CoverageMappingReader.cpp

370 ↗

(On Diff #38973)

Actually on 32bit target, CFR->FuncHash is not 8 byte aligned. To support reading coverage data produced by 32bit target on a 64bit host, the CovMapFunctionRecord struct (instantiated with 32 bit IntPtrT for 32bit target) needs to be declared with attribute((packed)). The attribute does not affect this struct's native layout.

[davidxl]

Updated patch according to Vedant's comment.

[vsk]

Thanks for clarifying, this lgtm.

llvm/trunk/include/llvm/ProfileData/InstrProf.h

 
 // The definition should match the structure defined in
 // compiler-rt/lib/profile/InstrProfiling.h.
 // It should also match the synthesized type in
 // Transforms/Instrumentation/InstrProfiling.cpp:getOrCreateRegionCounters.
 
 template <class IntPtrT>
 struct ProfileData {
-  const uint32_t NameSize;
-  const uint32_t NumCounters;
-  const uint64_t FuncHash;
-  const IntPtrT NamePtr;
-  const IntPtrT CounterPtr;
+  #define INSTR_PROF_DATA(Type, LLVMType, Name, Init) Type Name;
+  #include "llvm/ProfileData/InstrProfData.inc"
 };
 
 // The definition should match the header referenced in
 // compiler-rt/lib/profile/InstrProfilingFile.c  and
 // InstrProfilingBuffer.c.
 
 struct Header {
   const uint64_t Magic;
   const uint64_t Version;
   const uint64_t DataSize;
   const uint64_t CountersSize;
   const uint64_t NamesSize;
   const uint64_t CountersDelta;
   const uint64_t NamesDelta;
 };
 
 }  // end namespace RawInstrProf
 
+namespace coverage {
+
+LLVM_PACKED_START
+template <class IntPtrT>
+struct CovMapFunctionRecord {
+  #define COVMAP_FUNC_RECORD(Type, LLVMType, Name, Init) Type Name;
+  #include "llvm/ProfileData/InstrProfData.inc"
+};
+LLVM_PACKED_END
+
+}
+
 } // end namespace llvm
 
 namespace std {
 template <>
 struct is_error_code_enum<llvm::instrprof_error> : std::true_type {};
 }
 
 #endif // LLVM_PROFILEDATA_INSTRPROF_H_

llvm/trunk/include/llvm/ProfileData/InstrProfData.inc

 // runtime library, the compiler IR lowering, and profile reader/writer need
 // to use the same template to make sure the same data structure is defined
 // consistently.
 //
 // Examples of how the template is used:
 // 1. To declare a structure:
 // 
 // struct ProfData {
-// #define INSTR_PROF_DATA(Type, LLVMTypeVar, LLVMType, Name, Initializer) \
+// #define INSTR_PROF_DATA(Type, LLVMType, Name, Initializer) \
 //    Type Name;
-// #include "ProfileData/InstrProfData.inc"
+// #include "llvm/ProfileData/InstrProfData.inc"
 // };
 //
-// 2. To define local variables for struct member's LLVM types"
-//
-// #define INSTR_PROF_DATA(Type, LLVMTypeVar, LLVMType, Name, Initializer) \
-//   LLVMTypeVar = LLVMType;
-// #include "ProfileData/InstrProfData.inc"
-//
-// 3. To construct LLVM type arrays for the struct type:
+// 2. To construct LLVM type arrays for the struct type:
 //
 // Type *DataTypes[] = {
-// #define INSTR_PROF_DATA(Type, LLVMTypeVar, LLVMType, Name, Initializer) \
-//   LLVMTypeVar,
-// #include "ProfileData/InstrProfData.inc"
+// #define INSTR_PROF_DATA(Type, LLVMType, Name, Initializer) \
+//   LLVMType,
+// #include "llvm/ProfileData/InstrProfData.inc"
 // };
 //
 // 4. To construct constant array for the initializers:
-// #define INSTR_PROF_DATA(Type, LLVMTypeVar, LLVMType, Name, Initializer) \
+// #define INSTR_PROF_DATA(Type, LLVMType, Name, Initializer) \
 //   Initializer,
 // Constant *ConstantVals[] = {
-// #include "ProfileData/InstrProfData.inc"
+// #include "llvm/ProfileData/InstrProfData.inc"
 // };
 //===----------------------------------------------------------------------===//
 
 #ifndef INSTR_PROF_DATA
-#define INSTR_PROF_DATA(Type, LLVMTypeVar, LLVMType, Name, Initializer)
+#define INSTR_PROF_DATA(Type, LLVMType, Name, Initializer)
 #endif
 
 // INSTR_PROF_DATA_START
-INSTR_PROF_DATA(const uint32_t, Int32Ty, llvm::Type::getInt32Ty(Ctx), NameSize, \
-                ConstantInt::get(Int32Ty, NameSize))
-INSTR_PROF_DATA(const uint32_t, Int32Ty, llvm::Type::getInt32Ty(Ctx), NumCounters, \
-                ConstantInt::get(Int32Ty, NumCounters))
-INSTR_PROF_DATA(const uint64_t, Int64Ty, llvm::Type::getInt64Ty(Ctx), FuncHash, \
-                ConstantInt::get(Int64Ty, FuncHash))
-INSTR_PROF_DATA(const IntPtrT, Int8PtrTy,llvm::Type::getInt8PtrTy(Ctx), NamePtr, \
-                ConstantExpr::getBitCast(Name, Int8PtrTy))
-INSTR_PROF_DATA(const IntPtrT, Int64PtrTy, llvm::Type::getInt64PtrTy(Ctx), CounterPtr, \
-                ConstantExpr::getBitCast(CounterPtr, Int8PtrTy))
+INSTR_PROF_DATA(const uint32_t, llvm::Type::getInt32Ty(Ctx), NameSize, \
+                ConstantInt::get(llvm::Type::getInt32Ty(Ctx), \
+                NamePtr->getType()->getPointerElementType()->getArrayNumElements()))
+INSTR_PROF_DATA(const uint32_t, llvm::Type::getInt32Ty(Ctx), NumCounters, \
+                ConstantInt::get(llvm::Type::getInt32Ty(Ctx), NumCounters))
+INSTR_PROF_DATA(const uint64_t, llvm::Type::getInt64Ty(Ctx), FuncHash, \
+                ConstantInt::get(llvm::Type::getInt64Ty(Ctx), \
+                Inc->getHash()->getZExtValue()))
+INSTR_PROF_DATA(const IntPtrT, llvm::Type::getInt8PtrTy(Ctx), NamePtr, \
+                ConstantExpr::getBitCast(NamePtr, llvm::Type::getInt8PtrTy(Ctx)))
+INSTR_PROF_DATA(const IntPtrT, llvm::Type::getInt64PtrTy(Ctx), CounterPtr, \
+                ConstantExpr::getBitCast(CounterPtr, \
+                llvm::Type::getInt64PtrTy(Ctx)))
 // INSTR_PROF_DATA_END
 
 #ifdef INSTR_PROF_DATA
 #undef INSTR_PROF_DATA
 #endif
 
 
 #ifndef COVMAP_FUNC_RECORD
-#define COVMAP_FUNC_RECORD(Type, LLVMTypeVar, LLVMType, Name, Initializer)
+#define COVMAP_FUNC_RECORD(Type, LLVMType, Name, Initializer)
 #endif
 
 // COVMAP_FUNC_RECORD_START
-COVMAP_FUNC_RECORD(const IntPtrT, Int8PtrTy, llvm::Type::getInt8PtrTy(Ctx), \
-                   NamePtr, llvm::ConstantExpr::getBitCast(NamePtr, Int8PtrTy))
-COVMAP_FUNC_RECORD(const uint32_t, Int32Ty, llvm::Type::getInt32Ty(Ctx), \
-                   NameSize, llvm::ConstantInt::get(Int32Ty, NameSize))
-COVMAP_FUNC_RECORD(const uint32_t, Int32Ty, llvm::Type::getInt32Ty(Ctx), \
-                   DataSize, llvm::ConstantInt::get(Int32Ty, DataSize))
-COVMAP_FUNC_RECORD(const uint64_t, Int64Ty, llvm::Type::getInt64Ty(Ctx), \
-                   FuncHash, llvm::ConstantInt::get(Int64Ty, FuncSize))
+COVMAP_FUNC_RECORD(const IntPtrT, llvm::Type::getInt8PtrTy(Ctx), \
+                   NamePtr, llvm::ConstantExpr::getBitCast(NamePtr, \
+                   llvm::Type::getInt8PtrTy(Ctx))) 
+COVMAP_FUNC_RECORD(const uint32_t, llvm::Type::getInt32Ty(Ctx), NameSize, \
+                   llvm::ConstantInt::get(llvm::Type::getInt32Ty(Ctx), NameSize))
+COVMAP_FUNC_RECORD(const uint32_t, llvm::Type::getInt32Ty(Ctx), DataSize, \
+                   llvm::ConstantInt::get(llvm::getInt32Ty(Ctx), DataSize))
+COVMAP_FUNC_RECORD(const uint64_t, llvm::Type::getInt64Ty(Ctx), FuncHash, \
+                   llvm::ConstantInt::get(llvm::Type::getInt64Ty(Ctx), FuncHash))
 // COVMAP_FUNC_RECORD_END
 
 #ifdef COVMAP_FUNC_RECORD
 #undef COVMAP_FUNC_RECORD
 #endif
 
 

llvm/trunk/lib/ProfileData/CoverageMappingReader.cpp

for (const char *Buf = Data.data(), *End = Buf + Data.size(); Buf < End;) {
     case CoverageMappingVersion1:
       break;
     default:
       return coveragemap_error::unsupported_version;
     }
 
     // Skip past the function records, saving the start and end for later.
     const char *FunBuf = Buf;
-    Buf += NRecords * (sizeof(T) + 2 * sizeof(uint32_t) + sizeof(uint64_t));
+    Buf += NRecords * sizeof(coverage::CovMapFunctionRecord<T>);
     const char *FunEnd = Buf;
 
     // Get the filenames.
     if (Buf + FilenamesSize > End)
       return coveragemap_error::malformed;
     size_t FilenamesBegin = Filenames.size();
     RawCoverageFilenamesReader Reader(StringRef(Buf, FilenamesSize), Filenames);
     if (auto Err = Reader.read())
       return Err;
     Buf += FilenamesSize;
 
     // We'll read the coverage mapping records in the loop below.
     const char *CovBuf = Buf;
     Buf += CoverageSize;
     const char *CovEnd = Buf;
 
     if (Buf > End)
       return coveragemap_error::malformed;
     // Each coverage map has an alignment of 8, so we need to adjust alignment
     // before reading the next map.
     Buf += alignmentAdjustment(Buf, 8);
 
-    while (FunBuf < FunEnd) {
+    auto CFR =
+        reinterpret_cast<const coverage::CovMapFunctionRecord<T> *>(FunBuf);
+    while ((const char *)CFR < FunEnd) {
       // Read the function information
-      T NamePtr = endian::readNext<T, Endian, unaligned>(FunBuf);
-      uint32_t NameSize = endian::readNext<uint32_t, Endian, unaligned>(FunBuf);
-      uint32_t DataSize = endian::readNext<uint32_t, Endian, unaligned>(FunBuf);
-      uint64_t FuncHash = endian::readNext<uint64_t, Endian, unaligned>(FunBuf);
+      T NamePtr = endian::byte_swap<T, Endian>(CFR->NamePtr);
+      uint32_t NameSize = endian::byte_swap<uint32_t, Endian>(CFR->NameSize);
+      uint32_t DataSize = endian::byte_swap<uint32_t, Endian>(CFR->DataSize);
+      uint64_t FuncHash = endian::byte_swap<uint64_t, Endian>(CFR->FuncHash);
+      CFR++;
 
       // Now use that to read the coverage data.
       if (CovBuf + DataSize > CovEnd)
         return coveragemap_error::malformed;
       auto Mapping = StringRef(CovBuf, DataSize);
       CovBuf += DataSize;
 
       // Ignore this record if we already have a record that points to the same

llvm/trunk/lib/Transforms/Instrumentation/InstrProfiling.cpp

 static std::string getVarName(InstrProfIncrementInst *Inc, StringRef Prefix) {
   auto *Arr = cast<ConstantDataArray>(Inc->getName()->getInitializer());
   StringRef Name = Arr->isCString() ? Arr->getAsCString() : Arr->getAsString();
   return (Prefix + Name).str();
 }
 
 GlobalVariable *
 InstrProfiling::getOrCreateRegionCounters(InstrProfIncrementInst *Inc) {
-  GlobalVariable *Name = Inc->getName();
-  auto It = RegionCounters.find(Name);
+  GlobalVariable *NamePtr = Inc->getName();
+  auto It = RegionCounters.find(NamePtr);
   if (It != RegionCounters.end())
     return It->second;
 
   // Move the name variable to the right section. Place them in a COMDAT group
   // if the associated function is a COMDAT. This will make sure that
   // only one copy of counters of the COMDAT function will be emitted after
   // linking.
   Function *Fn = Inc->getParent()->getParent();
   Comdat *ProfileVarsComdat = nullptr;
   if (Fn->hasComdat())
     ProfileVarsComdat = M->getOrInsertComdat(
         StringRef(getVarName(Inc, getInstrProfComdatPrefix())));
-  Name->setSection(getNameSection());
-  Name->setAlignment(1);
-  Name->setComdat(ProfileVarsComdat);
+  NamePtr->setSection(getNameSection());
+  NamePtr->setAlignment(1);
+  NamePtr->setComdat(ProfileVarsComdat);
 
   uint64_t NumCounters = Inc->getNumCounters()->getZExtValue();
   LLVMContext &Ctx = M->getContext();
   ArrayType *CounterTy = ArrayType::get(Type::getInt64Ty(Ctx), NumCounters);
 
   // Create the counters variable.
-  auto *Counters =
-      new GlobalVariable(*M, CounterTy, false, Name->getLinkage(),
+  auto *CounterPtr =
+      new GlobalVariable(*M, CounterTy, false, NamePtr->getLinkage(),
                          Constant::getNullValue(CounterTy),
                          getVarName(Inc, getInstrProfCountersVarPrefix()));
-  Counters->setVisibility(Name->getVisibility());
-  Counters->setSection(getCountersSection());
-  Counters->setAlignment(8);
-  Counters->setComdat(ProfileVarsComdat);
+  CounterPtr->setVisibility(NamePtr->getVisibility());
+  CounterPtr->setSection(getCountersSection());
+  CounterPtr->setAlignment(8);
+  CounterPtr->setComdat(ProfileVarsComdat);
 
-  RegionCounters[Inc->getName()] = Counters;
+  RegionCounters[Inc->getName()] = CounterPtr;
 
   // Create data variable.
-  auto *NameArrayTy = Name->getType()->getPointerElementType();
-  auto *Int32Ty = Type::getInt32Ty(Ctx);
-  auto *Int64Ty = Type::getInt64Ty(Ctx);
-  auto *Int8PtrTy = Type::getInt8PtrTy(Ctx);
-  auto *Int64PtrTy = Type::getInt64PtrTy(Ctx);
 
-  Type *DataTypes[] = {Int32Ty, Int32Ty, Int64Ty, Int8PtrTy, Int64PtrTy};
+  Type *DataTypes[] = {
+    #define INSTR_PROF_DATA(Type, LLVMType, Name, Init) LLVMType,
+    #include "llvm/ProfileData/InstrProfData.inc"
+  };
   auto *DataTy = StructType::get(Ctx, makeArrayRef(DataTypes));
+
   Constant *DataVals[] = {
-      ConstantInt::get(Int32Ty, NameArrayTy->getArrayNumElements()),
-      ConstantInt::get(Int32Ty, NumCounters),
-      ConstantInt::get(Int64Ty, Inc->getHash()->getZExtValue()),
-      ConstantExpr::getBitCast(Name, Int8PtrTy),
-      ConstantExpr::getBitCast(Counters, Int64PtrTy)};
-  auto *Data = new GlobalVariable(*M, DataTy, true, Name->getLinkage(),
+    #define INSTR_PROF_DATA(Type, LLVMType, Name, Init) Init,
+    #include "llvm/ProfileData/InstrProfData.inc"
+  };
+
+  auto *Data = new GlobalVariable(*M, DataTy, true, NamePtr->getLinkage(),
                                   ConstantStruct::get(DataTy, DataVals),
                                   getVarName(Inc, getInstrProfDataVarPrefix()));
-  Data->setVisibility(Name->getVisibility());
+  Data->setVisibility(NamePtr->getVisibility());
   Data->setSection(getDataSection());
   Data->setAlignment(8);
   Data->setComdat(ProfileVarsComdat);
 
   // Mark the data variable as used so that it isn't stripped out.
   UsedVars.push_back(Data);
 
-  return Counters;
+  return CounterPtr;
 }
 
 void InstrProfiling::emitRegistration() {
   // Don't do this for Darwin.  compiler-rt uses linker magic.
   if (Triple(M->getTargetTriple()).isOSDarwin())
     return;
 
   // Use linker script magic to get data/cnts/name start/end.