[PATCH] Initial instrumentation based PGO implementation

[John McCall]

> <0001-profile-Rudimentary-suppport-for-PGO-instrumentation.patch><0001-Driver-Accept-fprofile-instr-use-and-fprofile-instr-.patch><0002-CodeGen-Initial-instrumentation-based-PGO-implementa.patch>

+  unsigned Counter = PGO.getRegionCounter(&S);
+  RegionCounter Cnt(PGO, Counter);

There are a number of places where you do this instead of getPGORegionCounter, maybe so that you can use Counter separately.  Just use Cnt.getCounter() downstream instead.  (You should have a getCounter() method on that type.)

+ unsigned LoopCnt

There are a bunch of places you pass counts around as unsigned, and then a bunch of other times that you pass them as uint64_t.  That’s problematic; so is the ease of passing other kinds of data accidentally as unsigned.  The sky will be brighter and the day merrier if you make an opaque wrapper type for this.  It’ll also make your rampant use of signal values much more self-documenting.

The same goes for counter indexes, where the size problems are less but the risk of accidentally using a counter where you meant to use a count is much larger.

+class PGOProfileData {
+private:
+  llvm::OwningPtr<llvm::MemoryBuffer> DataBuffer;

+  llvm::StringMap<unsigned> DataOffsets;

Please document that these are offsets into the buffer.

It’s probably reasonable to assume that offsets fit within an unsigned, but you should at least protect against huge profile files.

+PGOProfileData::PGOProfileData(CodeGenModule &CGM, std::string Path)

Why is this being taken as a std::string, and why is it being taken by value?

+  std::vector<uint64_t> *getFunctionCounts(StringRef MangledName);

If you’re going to heap-allocate this (for some reason?), please at least use an owning pointer.

+  /// Given a region counter for a loop, return the counter for a break
+  /// statement within the loop. The result is undefined for non-loop counters.
+  unsigned getBreakCounter(unsigned BaseCounter) { return BaseCounter + 1; }

Interesting.  You want a single counter associated with all the breaks in a loop?

+  CodeGenPGO PGO;
+

Why are we not following the DebugInfo lead and making this something external that only gets created if PGO is actually enabled and data is present for this function?  If you’re worried about allocation overhead, you can at least make it an Optional.

John.