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

[Bonfante Nicolas via Phabricator via llvm-commits]

niosega 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;
+
----------------
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. 


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:90
+                 << " is a Scalar access. \n";
+          NotExpandable = true;
+        }
----------------
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


================
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);
----------------
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.


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:297-298
+  // Expand all expandable write MemoryAccesses
+  for (ScopStmt &Stmt : S) {
+    for (MemoryAccess *MA : Stmt) {
+
----------------
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. 


https://reviews.llvm.org/D34982