[PATCH] Use auto profiler to set branch weights

[Chandler Carruth]

================
Comment at: include/llvm/Transforms/Scalar.h:365
@@ +364,3 @@
+FunctionPass *createSampleProfileLoaderPass();
+FunctionPass *createSampleProfileLoaderPass(std::string Name);
+
----------------
Use a StringRef?

================
Comment at: lib/Transforms/Scalar/SampleProfile.cpp:485-486
@@ +484,4 @@
+FunctionPass *llvm::createSampleProfileLoaderPass(std::string name) {
+  SampleProfileFilename = name;
+  return new SampleProfileLoader();
+}
----------------
Writing to the flag is thread hostile.

Instead, the constructor should accept the name as an argument. The default constructor can still use the flag for 'opt' command usage.

================
Comment at: lib/Transforms/Scalar/SampleProfile.cpp:52
@@ +51,3 @@
+static cl::opt<std::string>
+SampleProfileFilename("sample-profile-file", cl::init("samples.prof"),
+                      cl::value_desc("filename"),
----------------
Just for simplicity I would suggest naming the flag and the variable the same way. (So FooBarBaz and -foo-bar-baz for example...)

================
Comment at: lib/Transforms/Scalar/SampleProfile.cpp:52
@@ +51,3 @@
+static cl::opt<std::string>
+SampleProfileFilename("sample-profile-file", cl::init("samples.prof"),
+                      cl::value_desc("filename"),
----------------
Chandler Carruth wrote:
> Just for simplicity I would suggest naming the flag and the variable the same way. (So FooBarBaz and -foo-bar-baz for example...)
Also, I wouldn't use an actual filename as the default, and instead use the empty string. Then you can report a fatal error if this pass is run without a filename being provided.

================
Comment at: lib/Transforms/Scalar/SampleProfile.cpp:118-122
@@ +117,7 @@
+
+    /// \brief Map basic blocks to their computed weights.
+    ///
+    /// The weight of a basic block is defined to be the maximum
+    /// of all the instruction weights in that block.
+    BlockWeightMap BlockWeights;
+  };
----------------
I don't think you want this to persist this way.

This profile struct should contain the invariant data read from the file. Basic blocks might go away at any point. You'll want a single weight map in your pass and you'll want to clear it on each run of the pass.

================
Comment at: lib/Transforms/Scalar/SampleProfile.cpp:125
@@ +124,3 @@
+
+  typedef StringMap<FunctionProfile> FunctionProfileMap;
+
----------------
I don't think this typedef is buying anything. Just write the type down?

================
Comment at: lib/Transforms/Scalar/SampleProfile.cpp:471-474
@@ +470,6 @@
+
+bool SampleProfileLoader::loadProfile() {
+  Profiler = new SampleProfile(SampleProfileFilename);
+  return Profiler->loadText();
+}
+
----------------
I would inline this into its only caller -- doInitialization.

================
Comment at: lib/Transforms/Scalar/SampleProfile.cpp:472
@@ +471,3 @@
+bool SampleProfileLoader::loadProfile() {
+  Profiler = new SampleProfile(SampleProfileFilename);
+  return Profiler->loadText();
----------------
So, if we run over 2 modules, we leak the old profiler?

I think you want an OwningPtr here and to reset.


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