[PATCH] D15722: [WIP][Polly] SSA Codegen

[Sebastian Pop via llvm-commits]

sebpop added a comment.

In general I like the change, though I do not understand the use of undef values in several places in the patch.


================
Comment at: include/polly/CodeGen/BlockGenerators.h:308
@@ +307,3 @@
+  ///
+  /// @param V     A (possibly) loop carried value.
+  /// @param BBMap A mapping from old values to their new values
----------------
What is a loop carried value?

================
Comment at: include/polly/CodeGen/BlockGenerators.h:313
@@ +312,3 @@
+  /// @returns The new loop carried PHI for @p V.
+  Value *createLoopCarriedPHI(Value *V, ValueMapT &BBMap);
+
----------------
Could we call these "loop phi" nodes instead of "loop carried phi" nodes?

There is possibly confusion in reading the abbreviated LCPHI: the existing convention is to read it Loop Closed Phi, as in LCSSA.


================
Comment at: lib/CodeGen/BlockGenerators.cpp:136
@@ -128,3 +135,3 @@
 
-  // A scop-constant value defined by an instruction executed outside the scop.
-  if (const Instruction *Inst = dyn_cast<Instruction>(Old))
+  // An instruction defined inside the scop.
+  if (const Instruction *Inst = dyn_cast<Instruction>(Old)) {
----------------
This comment is confusing: either remove it, or move it one line down and fix it to say "outside the scop".

================
Comment at: lib/CodeGen/BlockGenerators.cpp:425
@@ +424,3 @@
+          canSyntheziseInStmt(Stmt, AccessValueInst))
+        copyInstruction(Stmt, AccessValueInst, BBMap, LTS, nullptr,
+                        /* Force */ true);
----------------
Why are we forcing copy of instructions that can be synthesized?
Aren't these synthesizable instructions discarded in the first place by not being added to the scalar memory accesses of the stmt?

================
Comment at: lib/CodeGen/BlockGenerators.cpp:1055
@@ -1134,1 +1054,3 @@
 
+    // TODO
+    generateScalarMappings(Stmt, BB, ScalarMap, RegionMap, LTS);
----------------
Unfinished comment?

================
Comment at: lib/CodeGen/IslNodeBuilder.cpp:549
@@ +548,3 @@
+      if (!PreVal)
+        PreVal = UndefValue::get(V->getType());
+
----------------
I don't understand how these undef values are supposed to work.
Are you cleaning them up in a later pass?
I see in some of the testcases that they are still left in the IR after Polly.


http://reviews.llvm.org/D15722