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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

Thank you for this mostly working version. I hope my comments are not too daunting.



================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:90
+                 << " is a Scalar access. \n";
+          NotExpandable = true;
+        }
----------------
[Suggestion] One could skip the check for the current array once it is known to be unexpandable and continue with the next one.


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:188-199
+  for (auto Write : Writes) {
+    auto WriteAccessRelation = isl::give(Write->getOriginalAccessRelation());
+    auto WriteOutId = WriteAccessRelation.get_tuple_id(isl::dim::out);
+    auto WriteInId = WriteAccessRelation.get_tuple_id(isl::dim::in);
+
+    if (ReadOutId.keep() == WriteOutId.keep() &&
+        NewOutId.keep() == WriteInId.keep()) {
----------------
[Suggestion] Instead of searching for the correct id, you could derive the name as in `expandWrite` and look it up in `ScopArrayNameMap`.

[Serious] What if the `Id` is not found? Please add an assertion for that case.


================
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);
----------------
[Suggestion] Or use `ScalarEvolution::getAddExpr(SCEV, ScalarEvolution::getConstant(1))`

[Serious] I don't see where +1 is added to the ISL version.


================
Comment at: test/MaximalStaticExpansion/working_expansion.ll:24
+;
+; CHECK: double MemRef_B_Stmt_for_body3_expanded[1999][2999]; // Element size 8
+;
----------------
Shouldn't this be
```
MemRef_B_Stmt_for_body3_expanded[2000][3000]
```
?


https://reviews.llvm.org/D34982