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

[Michael Kruse via llvm-commits]

Meinersbur added a comment.

I think I already mentioned in a conversation that I could make two (larger) commits from this, depending on whether it makes sense:

- Introduction of the scalar access maps ScalarLoads/ScalarWrites and uniqueing the loads per stmt.
- Uniqueing of PHI writes for non-affine subregion statements

A drawback of this patch is that the getAccessInstructin() might be nullptr (because an access can be causes by multiple instructions) in some cases. I started the thread "Accessing the same element multiple times within one ScopStmt" on the polly-dev mailing list to discuss this.

Required for DeLICM was the collapse of PHI writes in non-affine regions because such write may only occur after exiting a stmt, but an exiting basic block can have an edge back into the non-affine region. Another way to implement this to split edges from the exiting block to the exit block (ie. insert a basic block) and create the PHI WRITE accesses there.

DeLICM naturally uniques writes because it determines for each (BB,mappable array element) what the content of the array element at the end of the BB should be.


================
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,
----------------
grosser wrote:
> Is this change unrelated? Assuming it is, you can probably commit this as obvious.
In some intermediate state, I made it return a PHINode instead of just an llvm::Value. I may have noticed that it never returns nullptr, but forgot to change the comment back.

I'll commit this separately include the type change, which IMHO makes sense.

================
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);
 
----------------
grosser wrote:
> This is independent, no? It it is, please commit it ahead of time.
This was added with D13676. The explicit was added to contrast the usage of the functions introduced below

================
Comment at: include/polly/ScopInfo.h:1483
@@ -1414,3 +1482,3 @@
   }
   //@}
 
----------------
grosser wrote:
> This seems to be unused after the patch.
This is a general accessor. Removing means that AccFuncMap is not accessible anymore from other code. 

================
Comment at: lib/Analysis/ScopInfo.cpp:753
@@ -752,1 +752,3 @@
+  if (AccessRelation)
+    OS.indent(16) << getOriginalAccessRelationStr() << ";\n";
   if (hasNewAccessRelation())
----------------
grosser wrote:
> 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).
I'll commit separately

================
Comment at: lib/Analysis/ScopInfo.cpp:891
@@ -889,2 +890,3 @@
+    else if (Access->isScalar())
       Ty = ScopArrayInfo::KIND_SCALAR;
     else
----------------
grosser wrote:
> This seems good, but unrelated. Can you commit this ahead of time.
is Scalar is introduced in D13676. It maybe should belong to that 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;
----------------
grosser wrote:
> This looks good, but it seems independently useful? Could you commit this by itself?
getStmtForRegionNode was introduced in this patch

================
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;
 
----------------
grosser wrote:
> 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?
It has become necessary with this patch. The terminator can be a legitimate user of a value, such as the condition. Omitting the terminator before was fine before because the terminator does not define a value itself.

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


http://reviews.llvm.org/D13762