[PATCH] D13762: [Polly] Ensure unique implicit reads/writes at beginning/end of ScopStmts

[Tobias Grosser via llvm-commits]

grosser added a comment.

Hi Michael,

thank you for submitting this patch. I just went though it a first time and the overall direction seems good. Uniqueing implicit read/write accesses is likely to make sense by itself and as it as it is useful for DeLICM (either pre-or-post polyhedrization), this change seems well motivated.

I did not yet manage to do a full review, but already posted a couple of comments. My first impression is that the patch is mostly non-controversial, but rather large (600 LoC). To make the individual changes easier to understand (and easier to bisect in case of bugs), it seems worth to split out the independent parts and commit them one-by-one (some of them are trivial). I still need to have a more in-depth look at the actual unification, which seems mostly straightforward but includes some moved code which - in this larger patch - is not easy
to compare. Also, It seems we now keep track of the same MemoryAccess in multiple lists. I still need to understand this better as well.


================
Comment at: include/polly/CodeGen/BlockGenerators.h:798
@@ -797,3 +797,3 @@
   ///
-  /// @returns The copied instruction or nullptr if no copy was made.
+  /// @returns The copied PHI.
   virtual Value *copyPHIInstruction(ScopStmt &Stmt, PHINode *Inst,
----------------
Is this change unrelated? Assuming it is, you can probably commit this as obvious.

================
Comment at: include/polly/ScopInfo.h:326
@@ -327,1 +325,3 @@
+  /// #AccessInst is either the llvm::Value for WRITEs or nullptr for reads.
+  /// The #BaseAddr is represented by the value's definition (i.e. the
   /// llvm::Value itself) as no such alloca yet exists before CodeGeneration.
----------------
This is because a scalar may be used by multiple instructions in a ScopStmt and we only want to store one memory access for all of them. This makes sense (but could probably be mentioned in this comment).

================
Comment at: include/polly/ScopInfo.h:361
@@ -360,1 +360,3 @@
+  /// multiple exiting edges). The #BaseAddr is represented through the PHINode
+  /// because there
   /// also such alloca in the analyzed code. The #AccessValue is represented by
----------------
OK, I see that you move from multiple  to just a single PHI write access for non-affine regions. I wonder if this could be a separate patch?

================
Comment at: include/polly/ScopInfo.h:836
@@ -817,1 +835,3 @@
 
+  /// @brief The set of values defined elsewehere required in this ScopStmt and
+  ///        their SCALAR READ MemoryAccesses.
----------------
elsewhere

================
Comment at: include/polly/ScopInfo.h:841
@@ +840,3 @@
+  /// @brief The set of values defined in this ScopStmt that are required
+  ///        elsewehere and their SCALAR WRITE MemoryAccesses.
+  DenseMap<Instruction *, MemoryAccess *> ScalarWrites;
----------------
elsewhere

================
Comment at: include/polly/ScopInfo.h:848
@@ +847,3 @@
+  /// Non-affine subregions can
+  /// have multiple exiting blocks that are incoming blocks of the PHI nodes.
+  /// This map ensures that there is only one write operation for the complete
----------------
Refold lines?

================
Comment at: include/polly/ScopInfo.h:1046
@@ -990,2 +1045,3 @@
+  /// @brief Add @p Access to this statement's list of explicit accesses.
   void addExplicitAccess(MemoryAccess *Access);
 
----------------
This is independent, no? It it is, please commit it ahead of time.

================
Comment at: include/polly/ScopInfo.h:1483
@@ -1414,3 +1482,3 @@
   }
   //@}
 
----------------
This seems to be unused after the patch.

================
Comment at: lib/Analysis/ScopInfo.cpp:753
@@ -752,1 +752,3 @@
+  if (AccessRelation)
+    OS.indent(16) << getOriginalAccessRelationStr() << ";\n";
   if (hasNewAccessRelation())
----------------
I can remove this change without test case failure. It seems unrelated. Can you drop this one (or commit it independently if you need this for debugging).

================
Comment at: lib/Analysis/ScopInfo.cpp:891
@@ -889,2 +890,3 @@
+    else if (Access->isScalar())
       Ty = ScopArrayInfo::KIND_SCALAR;
     else
----------------
This seems good, but unrelated. Can you commit this ahead of time.

================
Comment at: lib/Analysis/ScopInfo.cpp:2010
@@ +2009,3 @@
+                              : Stmt->getBasicBlock();
+}
+
----------------
This function is unused. Please remove from patch.

================
Comment at: lib/Analysis/ScopInfo.cpp:3395
@@ -3343,9 +3394,3 @@
   // Check if there are accesses contained.
-  bool ContainsAccesses = false;
-  if (!RN->isSubRegion())
-    ContainsAccesses = getAccessFunctions(BB);
-  else
-    for (BasicBlock *RBB : RN->getNodeAs<Region>()->blocks())
-      ContainsAccesses |= (getAccessFunctions(RBB) != nullptr);
-  if (!ContainsAccesses)
+  if (Stmt->isEmpty())
     return true;
----------------
This looks good, but it seems independently useful? Could you commit this by itself?

================
Comment at: lib/Analysis/ScopInfo.cpp:3589
@@ +3588,3 @@
+  // region, s.t. a PHI demotion is required as well.
+  // Should be equivalent to getStmtForBasicBlock() == get
+  if (NonAffineSubRegion && PHI->getParent() != NonAffineSubRegion->getEntry())
----------------
This sentence is incomplete.

================
Comment at: lib/Analysis/ScopInfo.cpp:3618
@@ +3617,3 @@
+  // Check for uses of this instruction outside the scop. Because we do not
+  // iterate over such instructions and therefore do not "ensured" the existence
+  // of a write, we must determine such use here.
----------------
did not "ensure"

================
Comment at: lib/Analysis/ScopInfo.cpp:3628
@@ -3673,9 +3627,3 @@
 
-      if (Instruction *OpInst = dyn_cast<Instruction>(Op))
-        if (R->contains(OpInst))
-          continue;
-
-      if (isa<Constant>(Op))
-        continue;
-
-      addScalarReadAccess(Op, Inst);
+    // TODO: Explain condition
+    if (!R->contains(UseParent) ||
----------------
This seems unaddressed.

================
Comment at: lib/Analysis/ScopInfo.cpp:3851
@@ -3904,5 +3850,3 @@
 
-  bool isApproximated =
-      Stmt->isRegionStmt() && (Stmt->getRegion()->getEntry() != BB);
-  if (isApproximated && Type == MemoryAccess::MUST_WRITE)
-    Type = MemoryAccess::MAY_WRITE;
+  // The execution of an explicit store is not guranteed if not in the entry
+  // block of a subregion. By contrast, implicit writes must occur in
----------------
guaranteed

================
Comment at: lib/Analysis/ScopInfo.cpp:3855
@@ -3854,3 +3806,1 @@
-  for (BasicBlock::iterator I = BB.begin(), E = --BB.end(); I != E; ++I) {
-    Instruction *Inst = &*I;
 
----------------
Earlier we skipped the last iteration, now we do not do this any more. Why is this safe?

Also, I wonder if some of this move to the range iterator could be committed ahead of time?

================
Comment at: lib/Analysis/ScopInfo.cpp:3859
@@ -3909,1 +3858,3 @@
+      Type = MemoryAccess::MAY_WRITE;
+  }
 
----------------
This change seems independent. Can this be separated out?


http://reviews.llvm.org/D13762