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

Michael Kruse via llvm-commits llvm-commits at lists.llvm.org
Sun Apr 3 06:42:06 PDT 2016 

Meinersbur added a comment.

Thank you for the awseome patch. AFAIU the algorithm is "follow each generated MK_PHIKind through to the end of the generated CFG while adding PHIs to make them usable", ignoring whether the defs are used or how the PHIs where in the original code. Could you explain this somewhere, preferable not only in the commit message?

Is it correct that this gives us O(n*m) generated PHIs (n=number of MK_PHIKind, m=number of statements)?

I still don't really understand the "ScalarMap". What is the difference to BBMaps? Do we need both? It uses original defs as well merge PHIs used as key. I see the test cases motivating this, but could this be avoided by e.g. adding the incoming values in a separate pass instead on-the-fly?

I had 17 LNT test fails with -polly-position=before-vectorizer (none without it). Interestingly I also got 4 unit test fails under Windows but not Linux. Is there maybe some indeterministic ordering? The failing tests are:

  Polly :: Isl/CodeGen/phi_loop_carried_float.ll
  Polly :: Isl/CodeGen/phi_loop_carried_float_4.ll
  Polly :: Isl/CodeGen/phi_loop_carried_float_escape.ll
  Polly :: Isl/CodeGen/phi_scalar_simple_1.ll

If you rebase and the problem is still there, I'd debug this for you under Windows.


================
Comment at: include/polly/CodeGen/BlockGenerators.h:125
@@ -298,1 +124,3 @@
+  /// @brief The loop depth of the currently copied block in the new schedule.
+  int &LoopDepth;
 
----------------
I generally don't like the idea of a private/protected field being sneakingly modified from outside the object (here: IslNodeBuilder)

================
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
----------------
jdoerfert wrote:
> sebpop wrote:
> > What is a loop carried value?
> Any scalar value that is defined and used in different iterations of the same loop. Prior to the transformation this means it is used by a PHI, however after the transformation it might also refere to a scalar that is used in a loop textually before it was defined.
> 
> An example would be the folloing original code:
> 
> for (i = 0...N)
>   a = A[i];
>   A[i+1] = a;
>   
> with the following "optimized" version:
> 
> for (i = 0...N+1)
>   if (i > 0)
>     A[i] = a;
>   if (i < N + 1)
>     a = A[i];
> 
> Here the former non-loop carried scalar a is now loop-carried (thus needs a PHI).
Referring to "(possibly)": What is the function supposed to do if the value is not loop-carried (in what loop)?

================
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);
+
----------------
jdoerfert wrote:
> sebpop wrote:
> > 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.
> > 
> Sure.
Idea: Maybe define in some comment what exactly a "loop-carried phi" is? Eg. a PHI in a loop header.

================
Comment at: include/polly/CodeGen/BlockGenerators.h:574
@@ -768,2 +573,3 @@
   DenseMap<BasicBlock *, ValueMapT> RegionMaps;
+  DenseMap<BasicBlock *, ValueMapT> ScalarMaps;
 
----------------
ScalarMaps should have its comment. AFAIK Doxygen treats this as uncommented field.

================
Comment at: include/polly/CodeGen/IslNodeBuilder.h:69
@@ -74,1 +68,3 @@
+  /// @brief Map from loop carried PHI nodes to "floating" copies.
+  ValueMapT LCPHIs;
 
----------------
Can you explain "floating"?

As Sebastian also noted, on seeing "LCPHI" I was first thinking about the PHIs for LCSSA.

================
Comment at: include/polly/CodeGen/IslNodeBuilder.h:71
@@ -74,3 +70,3 @@
 
-  /// @brief See BlockGenerator::PhiOpMap.
-  BlockGenerator::ScalarAllocaMapTy PHIOpMap;
+  /// @brief Currently valid scalar mappings.
+  ValueMapT ScalarMap;
----------------
Could you explain a bit more about "ScalarMaps"? What is it mapping from/to? How/where is it used? What are valid keys?

================
Comment at: include/polly/CodeGen/IslNodeBuilder.h:82
@@ +81,3 @@
+
+  IslExprBuilder ExprBuilder;
+
----------------
Why moving this definition?

================
Comment at: include/polly/Support/ScopHelper.h:170
@@ +169,3 @@
+
+/// @brief Use @p Builder to create merge PHIs.
+///
----------------
This would be an excellent place to explain how a "merge PHI" is different from other PHIs.

================
Comment at: include/polly/Support/ScopHelper.h:176
@@ +175,3 @@
+///
+/// @returns The merged mappings in @p MergeMap.
+void createMergePHIs(PollyIRBuilder &Builder,
----------------
This function returns void

================
Comment at: lib/Analysis/ScopDetection.cpp:90
@@ -89,3 +89,3 @@
         "Process scops that are unlikely to benefit from Polly optimizations."),
-    cl::location(PollyProcessUnprofitable), cl::init(false), cl::ZeroOrMore,
+    cl::location(PollyProcessUnprofitable), cl::init(true), cl::ZeroOrMore,
     cl::cat(PollyCategory));
----------------
I assume this is a leftover from debugging.

================
Comment at: lib/CodeGen/BlockGenerators.cpp:232
@@ -208,3 +231,3 @@
                                           isl_id_to_ast_expr *NewAccesses) {
-  if (Value *PreloadLoad = GlobalMap.lookup(Load))
+  if (Value *PreloadLoad = BBMap.lookup(Load))
     return PreloadLoad;
----------------
This change is surprising to me. Aren't the hoisted loads stored in GlobalMap anymore?

================
Comment at: lib/CodeGen/BlockGenerators.cpp:265
@@ +264,3 @@
+Value *BlockGenerator::createLoopCarriedPHI(Value *V, ValueMapT &BBMap) {
+  auto *PHI = PHINode::Create(V->getType(), 2, V->getName() + ".polly.lc");
+
----------------
What is the reason to not insert the PHI into the generated BB at this point (but to keep them "floating")?

================
Comment at: lib/CodeGen/BlockGenerators.cpp:277-281
@@ +276,7 @@
+                                          LoopToScevMapT &LTS) {
+  if (!LI.isLoopHeader(PHI->getParent()))
+    return nullptr;
+
+  if (LoopDepth == 0 || canSyntheziseInStmt(Stmt, PHI))
+    return nullptr;
+
----------------
Could you add comments to explain these cases?

================
Comment at: lib/CodeGen/BlockGenerators.cpp:339
@@ -289,2 +338,3 @@
 
-  ValueMapT BBMap;
+  // Initialize the block intern mappings with all mapping that hold when this
+  // block is entered.
----------------
block-intern_al_?
mapping_s_

================
Comment at: lib/CodeGen/BlockGenerators.cpp:442
@@ +441,3 @@
+    // In the second case we map both, the PHICopy as well as the BaseAddr to
+    // the NewValue to allow uses textually before as well as after the scalar
+    // definition.
----------------
"textually" doesn't seem the right word; we are not doing text processing here. Should be something about processing order.

================
Comment at: lib/CodeGen/BlockGenerators.cpp:1165
@@ -1278,3 +1164,3 @@
   // Once the block will be copied the incoming value will be added.
-  BasicBlock *BBCopy = BlockMap[IncomingBB];
+  BasicBlock *BBCopy = BlockMap.lookup(IncomingBB);
   if (!BBCopy) {
----------------
Unrelated?

================
Comment at: lib/CodeGen/IslNodeBuilder.cpp:61
@@ +60,3 @@
+    auto &Array = Pair.second;
+    if (Array->getNumberOfDimensions() != 0 || !Array->isPHIKind())
+      continue;
----------------
`Array->getNumberOfDimensions()!=0` seems redundant. MK_PHI always has 0 dimensions.


================
Comment at: lib/CodeGen/IslNodeBuilder.cpp:522
@@ -471,1 +521,3 @@
 
+  auto PreMap = ScalarMap;
+  auto PreLCPHIs = LCPHIs;
----------------
Naive question: Is there some way we could have a nested instance of IslNodeBuilder/function call with fresh values instead of temporarily storing `LCPHIs`, `LoopDepth`, `PreMap` away and restoring it afterwards?

================
Comment at: lib/CodeGen/IslNodeBuilder.cpp:523-524
@@ +522,4 @@
+  auto PreMap = ScalarMap;
+  auto PreLCPHIs = LCPHIs;
+  LCPHIs.clear();
+
----------------
std::move

================
Comment at: lib/CodeGen/IslNodeBuilder.cpp:549
@@ +548,3 @@
+      if (!PreVal)
+        PreVal = UndefValue::get(V->getType());
+
----------------
jdoerfert wrote:
> sebpop wrote:
> > 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.
> Undef values are already "used" in the current code generation but simply hidden well enough. When a scalar is used before it is initialized we basically get an undef. Currently this means a load from an uninitialized alloca is basically an undef. As we promoted these allocas, loads and stores now we see the undefs in the IR, however they should only exist where the alloca content was undefined before. An example:
> 
> int x;
> for i = 0...N {
>   if (i > 0)
>     A[i-1] = x;
>    x = A[i];
> }
> 
> Here the initial value of x is undefined and therefor the PHI in the loop header as well as the PHI after the loop will have one undef as operand.
> 
> In the following example there were no undefs prior to Polly in the IR but right after code generation there are.
> 
> if (a) {
> S:  x = A[i];
>   /* split BB */
> P:  A[i] = x;  
> }
> 
> Now we split the conditional for whatever reason and Polly will generate a CFG that kinda looks like this:
> 
> if (a)
> S:  x1 = A[i];
> x = phi(undef, x1)
> 
> if (a)
> P:  A[i] = x;
> 
> We need the undef for the path that does not define x (or x1). Does that make sense? 
> 
Could you explain this in the code as a comment?

================
Comment at: lib/CodeGen/IslNodeBuilder.cpp:798
@@ -694,1 +797,3 @@
 
+  auto ElseMap = ScalarMap;
+  auto *ElseExitBB = Builder.GetInsertBlock();
----------------
std::move

================
Comment at: lib/Support/RegisterPasses.cpp:45
@@ -44,3 +44,3 @@
     PollyEnabled("polly", cl::desc("Enable the polly optimizer (only at -O3)"),
-                 cl::init(false), cl::ZeroOrMore, cl::cat(PollyCategory));
+                 cl::init(true), cl::ZeroOrMore, cl::cat(PollyCategory));
 
----------------
Assuming debugging leftover

================
Comment at: lib/Support/ScopHelper.cpp:468
@@ +467,3 @@
+    if (!IncomingVal)
+      IncomingVal = UndefValue::get(Val->getType());
+
----------------
If the incoming value is not found in ScalarMap, I can think of two reasons:
1) It's defined before the Scop
2) We forgot to put in there
3) It was defined only in one incoming branch.
In none of these cases "undef" would make sense.  In 3) it cannot be used without an explicit PHI in the original code. What other case is there?

================
Comment at: lib/Support/ScopHelper.cpp:476
@@ +475,3 @@
+
+    MergePHI->setName(IncomingVal->getName() + ".merge");
+  }
----------------
Wouldn't it be better to name the PHI after the original value instead one of the incoming values?

================
Comment at: lib/Support/ScopHelper.cpp:479
@@ +478,3 @@
+
+  if (Values.size() == 1) {
+    MergePHI->eraseFromParent();
----------------
Because Value.size() will be equal to BBs.size(), this condition can be tested before creating an not used MergePHI.


http://reviews.llvm.org/D15722

More information about the llvm-commits mailing list