[PATCH] Use discriminator information in sample profiles.

[Chandler Carruth]

  Again, mostly nits. This is fine to commit at any point really. I would fix the doxygen format issue before committing I suppose, but the rest is very forward thinking and less to do with the code at hand.


================
Comment at: lib/Transforms/Scalar/SampleProfile.cpp:68
@@ -67,1 +67,3 @@
 namespace {
+/// InstructionLocation - Instruction locations are specified by the
+/// line offset from the beginning of the function (marked by the line
----------------
Don't repeat the name of the class in the doxygen. Instead, use a one-sentence "\brief" bit?

================
Comment at: lib/Transforms/Scalar/SampleProfile.cpp:79-80
@@ +78,4 @@
+      : LineOffset(L), Discriminator(D) {}
+  uint32_t LineOffset;
+  uint32_t Discriminator;
+};
----------------
Are these wrapping? saturating? error if we reach UINT32_MAX (or whatever it is)? I'm not really worried either way, just wondering if it would be good to document it and enforce it with some asserts.

================
Comment at: lib/Transforms/Scalar/SampleProfile.cpp:86
@@ +85,3 @@
+template<> struct DenseMapInfo<InstructionLocation> {
+  typedef DenseMapInfo<uint32_t> Info;
+  static inline InstructionLocation getEmptyKey() {
----------------
Maybe HalfInfo or MemberInfo? Looking for some name that better identifies what it is...

================
Comment at: lib/Transforms/Scalar/SampleProfile.cpp:88-92
@@ +87,7 @@
+  static inline InstructionLocation getEmptyKey() {
+    return InstructionLocation(Info::getEmptyKey(), Info::getEmptyKey());
+  }
+  static inline InstructionLocation getTombstoneKey() {
+    return InstructionLocation(Info::getTombstoneKey(),
+                               Info::getTombstoneKey());
+  }
----------------
Related to my comment above about valid range -- I think this indicates the answer my question. If you can't have these uint32_t values, then it can't be wrapping, and if it is saturating you would need to write some code to saturate outside of these.

We should at least document and place an assert around this?


http://llvm-reviews.chandlerc.com/D2857

BRANCH
  discriminator-profile

ARCANIST PROJECT
  llvm