[PATCH] D34982: [Polly][WIP] Fully-Indexed static expansion

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: include/polly/ScopInfo.h:840-842
+  /// Get the original access function as read from IR.
+  __isl_give isl_map *getOriginalAccessRelation() const;
+
----------------
niosega wrote:
> Meinersbur wrote:
> > [Style] Why was this moved?
> This was moved from private to public section because I need this method to find the expanded SAI name during read expansion. But if I use the solution you suggest, I will not need it anymore. 
ok.


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:90
+                 << " is a Scalar access. \n";
+          NotExpandable = true;
+        }
----------------
niosega wrote:
> Meinersbur wrote:
> > [Suggestion] One could skip the check for the current array once it is known to be unexpandable and continue with the next one.
> That is what I wanted to do in the beginning. But I didn't find a clean solution to "escape" from the two innermost loops and go to next iteration of the loop that iterate over the SAI
A possible solution is to refactor the inner two loops to a function, which can `return true/false` for a `ScopArrayInfo` at any point.
```
bool isExpandable(SAI) {
 for (ScopStmt &Stmt : S) 
      for (MemoryAccess *MA : Stmt) {
         if (MA->isMayWrite())
           return false;
        
           ...
      }     
}

for (auto &SAI : S.arrays()) {
  if (!isExpandable())
    NotExpandables.insert(SAI);
}

```


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:250
+    auto SCEV = S.getSE()->getMaxBackedgeTakenCount(
+        Loop); // +1 but this will change later to the ISL version
+    SCEVSizes.push_back(SCEV);
----------------
niosega wrote:
> Meinersbur wrote:
> > [Suggestion] Or use `ScalarEvolution::getAddExpr(SCEV, ScalarEvolution::getConstant(1))`
> > 
> > [Serious] I don't see where +1 is added to the ISL version.
> We discuss during the last phone call about an other solution that involves methods that are in FlattenAlgo.cpp to get the boundary of the loop iterations variables. That's what I meant by "ISL version". But for now, I can not access the methods from FlattenAlgo.cpp because they are defined inside an unamed namespace.
You may need to modify them anyway, so don't hesitate to copy them over, especially for a prototype.

If later we find they share a significant amount of code, we can find a common file for them.


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:297-298
+  // Expand all expandable write MemoryAccesses
+  for (ScopStmt &Stmt : S) {
+    for (MemoryAccess *MA : Stmt) {
+
----------------
niosega wrote:
> Meinersbur wrote:
> > [Remark] The structure I had in mind was
> > ```
> > for (array : S.arrays()) {
> >   if (!checkExpendability(array))
> >     continue;
> > 
> >   for (ScopStmt &Stmt : S) 
> >     for (MemoryAccess *MA : Stmt) {
> >        if (MA->isRead())
> >          expandRead(MA);
> >        if (MA->isWrite())
> >          expandRead(MA);
> >   }
> > }
> > ```
> > This does not need a `NotExpandable` set, but has worse asymptotic runtime. So I guess your version has an advantage.
> > 
> If I am not wrong, we must first expand all writes before expanding the reads. Because otherwise problems can happened during trying to find expanded SAI during read expansion. 
Let me refine what I some time ago I had in mind.
```
for (array : S.arrays()) {
  MemoryAccess *TheWrite = nullptr;
  List<MemoryAccess*> *AllReads;
  if (!isExpandable(array, TheWrite, AllReads))
    continue;
  assert(TheWrite);
  assert(AllReads.size() > 0);

  ScopArrayInfo *ExpandedArray =  expandWrite(TheWrite);

  for (MemoryAccess *MA : AllReads) 
    expandRead(MA, ExpandedArray);
}
```


https://reviews.llvm.org/D34982