[PATCH] Add framework for iterative compilation to llvm

[Radovan Obradovic]

================
Comment at: include/llvm/Analysis/DecisionTree.h:17
@@ +16,3 @@
+#define LLVM_DECISIONTREE_H
+
+#include <vector>
----------------
grosser wrote:
> Maybe LLVM_ANALYSIS_DECISIONTREE_H?
Done

================
Comment at: include/llvm/Analysis/DecisionTree.h:44
@@ +43,3 @@
+  DecisionTreeNode *CurrentNode;
+  std::vector<bool> Path;
+  int FunctionFitness;
----------------
etherzhhb wrote:
> You could use llvm::Bitvector instead of std::vector<bool>.
Done.

================
Comment at: include/llvm/Analysis/DecisionTree.h:104
@@ +103,3 @@
+public:
+  enum IterativeCompilationPhase {
+    FunctionPhase, ///< Function level optimizations.
----------------
etherzhhb wrote:
> Please briefly explain all these Phases.
Done

================
Comment at: include/llvm/Analysis/DecisionTree.h:158
@@ +157,3 @@
+
+class ModuleDecisionTreeProxies : public ImmutablePass {
+public:
----------------
etherzhhb wrote:
> Maybe you could clarify your code by adding comments like:
> "This ImmutablePass provides a persistent place to hold the decision trees (for each functions) during the iterative compilation process ..."
Thanks! Done

================
Comment at: include/llvm/Analysis/DecisionTree.h:186
@@ +185,3 @@
+  std::map<std::string, DecisionTreeProxy *> DecisionTreeProxies;
+  DecisionTreeProxy *CurrFnDecisionTreeProxy;
+  ModuleDecisionTrees::IterativeCompilationPhase ICPhase;
----------------
etherzhhb wrote:
> Now you have "nameFunction" for the name of the current function and "CurrFnDecisionTreeProxy" for the DecisionTree of the current function. How about add a pointer to Function to  DecisionTreeProxy so that you can always access the current function (as well as its name) when you have a pointer to the current DecisionTree.
> 
> Then you can remobe "nameFunction".
> 
> You my also need a hook to notify the DecisionTree when the function inlining pass inline and delete a function.
"nameFunction" is removed. We are now using ModuleDecisionTreeProxies::setCurrentFunctionAndPhase to find and set decision tree proxy for given function name and compilation phase. So function name and
pointer to the Function do not have to be stored.

================
Comment at: lib/Analysis/DecisionTree.cpp:37
@@ +36,3 @@
+  FunctionFitness = 0;
+  BestFunctionFitness = 100000;
+  CurrentNode = DecisionTreeRoot;
----------------
etherzhhb wrote:
> Maybe you could use a constant (like MaxDtPriority) here instead of a magic number.
Now INT_MAX is used.

================
Comment at: lib/Analysis/DecisionTree.cpp:43
@@ +42,3 @@
+  CurrentNode->Priority = 0;
+  CurrentNode->Fitness = 100000;
+  CurrentNode->Depth = 0;
----------------
grosser wrote:
> How has this value be choosen? Does it always need to be in sync with BestFunctionFitness?
Now INT_MAX is used.

================
Comment at: lib/Analysis/DecisionTree.cpp:90
@@ +89,3 @@
+    CurrentNode->AlternativeDecision = newNode;
+    CurrentNode = newNode;
+    return true;
----------------
etherzhhb wrote:
> Looks like this line of code appears in both branches, you could move it above the "if" statement.
Done

================
Comment at: lib/Analysis/DecisionTree.cpp:103
@@ +102,3 @@
+
+std::vector<bool> DecisionTree::findPath(DecisionTreeNode *node) {
+  std::vector<bool> path;
----------------
etherzhhb wrote:
> In this function you are creating a vector in a function and return it at the end, such an approach need to copy the vector when the function return.
> 
> You could ask the caller of this function to pass in an empty vector by reference then fill the vector, such that the signature of this function become:
> void DecisionTree::findPath(DecisionTreeNode *node, std::vector<bool> &path) {
> ...
> }
> 
Done

================
Comment at: lib/Analysis/DecisionTree.cpp:156
@@ +155,3 @@
+  else {
+    Path = BestPath;
+    return true;
----------------
grosser wrote:
> Write:
> 
> } else {
Done

================
Comment at: lib/Analysis/DecisionTree.cpp:206
@@ +205,3 @@
+
+void DecisionTree::applayResults(std::vector<int> &results) {
+  unsigned i;
----------------
grosser wrote:
> Typo: applyResults, no?
Fixed

================
Comment at: lib/Analysis/DecisionTree.cpp:254
@@ +253,3 @@
+                               const std::string &data) {
+  std::ofstream outf(fileName.c_str());
+  if (!outf)
----------------
etherzhhb wrote:
> How about use LLVM's streams instead of ofstream?
> 
> Maybe you could write your data out in form of JSON, such that you could write python utilities to analyze/debug/render your data. And it is much more human readable.
Now using llvm::MemoryBuffer for reading and raw_ostream for writing. Temp files are now saved in JSON format and parsed by llmv YAML parser.

================
Comment at: lib/Analysis/DecisionTree.cpp:378
@@ +377,3 @@
+  std::map<std::string, DecisionTree *>::iterator it, e;
+  for (it = DecisionTrees.begin(), e = DecisionTrees.end(); it != e; it++) {
+    DecisionTree *dt = it->second;
----------------
grosser wrote:
> C++11 range loops maybe?
Done

================
Comment at: lib/Target/Mips/MipsFunctionFitness.cpp:46
@@ +45,3 @@
+      getAnalysis<ModuleDecisionTreeProxies>();
+    //DecisionTreeProxy &proxy = proxies.getDecisionTreeProxy(n);
+    proxies.setCurrentFunctionAndPhase(fnName, ModuleDecisionTrees::CodeGenPhase);
----------------
grosser wrote:
> Why is this commented out? (The same for the lines above)
Commented out lines deleted.

================
Comment at: lib/Target/Mips/MipsFunctionFitness.cpp:72
@@ +71,3 @@
+runOnMachineBasicBlock(MachineBasicBlock &MBB) {
+  unsigned count = 0;
+
----------------
grosser wrote:
> We commonly start variable names with upper-case letters.
OK. Done for some variables.

================
Comment at: lib/Transforms/IPO/Inliner.cpp:456
@@ -443,1 +455,3 @@
+  MDTP = &getAnalysis<ModuleDecisionTreeProxies>();
+  MDTP->setCurrentFunctionAndPhase("", ModuleDecisionTrees::ModulePhase);
   DataLayoutPass *DLP = getAnalysisIfAvailable<DataLayoutPass>();
----------------
etherzhhb wrote:
> This "" looks confusing.
Now function ModuleDecisionTreeProxies::setModuleCompilationPhase() is called to set decision tree for the ModulePhase.

http://reviews.llvm.org/D4723