[PATCH] Scalar/PHI code genration

[Tobias Grosser]

Hi Johannes,

this patch looks great. Thanks for pushing this ahead.

I added a couple of comments. Mostly minor typos. There is one comment on stuff that could be split off into a separate patch (would be nice, but not essential) as well as one possible correctness issue in the linked-list implementation.

Otherwise, this LGTM.

Also, we should try to enable this quickly. I think just having LNT performance numbers for the latest patch that confirm we do not badly regress should be enough to justify this switch.

Best,
Tobias


================
Comment at: include/polly/CodeGen/BlockGenerators.h:297
@@ -183,4 +296,3 @@
 
-  Value *generateScalarStore(ScopStmt &Stmt, const StoreInst *store,
-                             ValueMapT &BBMap, ValueMapT &GlobalMap,
-                             LoopToScevMapT &LTS);
+  Instruction *generateScalarStore(ScopStmt &Stmt, const StoreInst *store,
+                                   ValueMapT &BBMap, ValueMapT &GlobalMap,
----------------
This change is not needed. I would suggest to remove it from the patch to not distract from the core changes.

================
Comment at: include/polly/CodeGen/BlockGenerators.h:342
@@ +341,3 @@
+  ///                    (for values recalculated in the new ScoP, but not
+  ///                    within this basic block).
+  Value *getNewScalarValue(Value *ScalarValue, const Region &R,
----------------
@returns  missing

================
Comment at: include/polly/CodeGen/IslNodeBuilder.h:107
@@ -82,3 +106,3 @@
   /// @param Expr The expression to code generate.
-  llvm::Value *generateSCEV(const SCEV *Expr);
+  Value *generateSCEV(const SCEV *Expr);
 
----------------
This (and below)) seems like an unrelated change. Maybe commit separately.

================
Comment at: lib/Analysis/ScopInfo.cpp:888
@@ -895,1 +887,3 @@
+      MemAccs.back()->setNextMA(MA);
+    MA = MemAccs.back();
   }
----------------
Is this correct? In case we add three memory accesses A,B,C. Would this code not first add A to the map, than add a link from A to B, and then overwrite the link to B with C, which results in us loosing the link to B entirely?

I somehow feel the manually implemented linked list adds unnecessary complexity and also somehow mixes the MemoryAccess definition and
the InstructionToAccess mapping. I wonder if it might not be clearer to make the InstructionToAccess Map a mapping from instructions to a
vector of accesses.

================
Comment at: lib/CodeGen/BlockGenerators.cpp:278
@@ -259,1 +277,3 @@
+    Instruction *NewLoad =
+        generateScalarLoad(Stmt, Load, BBMap, GlobalMap, LTS);
     // Compute NewLoad before its insertion in BBMap to make the insertion
----------------
As this function only returns a Value, there is no need to change this to 'Instruction' (similar comment below). To not distract from the core change, I would suggest to avoid this change (or perform it in a separate patch).

================
Comment at: lib/CodeGen/BlockGenerators.cpp:360
@@ +359,3 @@
+    assert((InstCopy == nullptr || BBMap[&Inst] == InstCopy) &&
+           "Bad copy mapping found");
+  }
----------------
This patch modifies various functions to pass the newly created instruction on.  I somehow believe to remember you earlier passed these on for some other use
case, but the current patch only seems to need these changes for the assert.

If the assert has shown helpful during development, it seems reasonable to commit it (and the associated changes). However, in the optimal case, this would be a separate patch committed ahead of time. If that is to much work, I do not insist on separating this out.

Also, a general phabricator issue is that the updated commit messages are not visible. It would be nice if you could add a comment in the commit message that explains the intention of these set of changes (to help people understand that they are not strictly necessary for the core goal of this patch).

================
Comment at: lib/CodeGen/BlockGenerators.cpp:419
@@ +418,3 @@
+  // it in the EscapeMap for later finalization. However, if the alloca was not
+  // created by a already handled scalar dependence we have to initialize it
+  // also. Lastly, if the instruction was copied multiple times we already did
----------------
an

================
Comment at: lib/CodeGen/BlockGenerators.cpp:483
@@ +482,3 @@
+                                         ValueMapT &GlobalMap) {
+  // If the value we want to store is a instruction we might have demoted it
+  // in order to make it accessible here. In such a case a reload is
----------------
an

================
Comment at: lib/CodeGen/BlockGenerators.cpp:489
@@ +488,3 @@
+  //  (1) The value is no instruction ==> use the value.
+  //  (2) The value is an instruction that was split out the region prior to
+  //      code generation  ==> use the instruction as it dominates the region.
----------------
out of the region

================
Comment at: lib/CodeGen/BlockGenerators.cpp:494
@@ +493,3 @@
+  //          the BBMap ==> use the mapped value.
+  //      (b) The value was defined in a previous block, thus demoted it
+  //          earlier ==> use the reloaded value.
----------------
grammar?

================
Comment at: lib/CodeGen/BlockGenerators.cpp:536
@@ +535,3 @@
+  // Iterate over all accesses in the given statement.
+  for (auto MI = Stmt.begin(), ME = Stmt.end(); MI != ME; MI++) {
+    MemoryAccess *MA = *MI;
----------------
Use a range-based for loop?

================
Comment at: lib/CodeGen/BlockGenerators.cpp:1220
@@ +1219,3 @@
+  // have a definition in the region. Therefore,
+
+  // Iterate over all accesses in the given statement.
----------------
Unfinished sentence.

================
Comment at: lib/CodeGen/IslCodeGeneration.cpp:141
@@ -137,1 +140,3 @@
+    AI->dump();
+    EnteringBB->getParent()->dump();
     return true;
----------------
leftover debugging stuff?

http://reviews.llvm.org/D7513

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/