[PATCH] Use auto profiler to set branch weights

[Diego Novillo]

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

================
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:
> 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.
Done.

================
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;
+  };
----------------
Chandler Carruth wrote:
> 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.
Done. I now clear the block weights cache on every call to the pass.

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

================
Comment at: lib/Transforms/Scalar/SampleProfile.cpp:472
@@ +471,3 @@
+bool SampleProfileLoader::loadProfile() {
+  Profiler = new SampleProfile(SampleProfileFilename);
+  return Profiler->loadText();
----------------
Chandler Carruth wrote:
> So, if we run over 2 modules, we leak the old profiler?
> 
> I think you want an OwningPtr here and to reset.
Done.

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

================
Comment at: lib/Transforms/Scalar/SampleProfile.cpp:485-486
@@ +484,4 @@
+FunctionPass *llvm::createSampleProfileLoaderPass(std::string name) {
+  SampleProfileFilename = name;
+  return new SampleProfileLoader();
+}
----------------
Chandler Carruth wrote:
> 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.
Done.


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