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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: lib/Support/RegisterPasses.cpp:319-320
 
+  if (FullyIndexedStaticExpansion)
+    PM.add(polly::createMaximalStaticExpansionPass());
+
----------------
Suggestion: Put it behind DeadCodeElimination? It uses DependenceInfo and should have the same results even before MSE.


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:1
+//===-  MaximalStaticExpansion.cpp - Expand the memory access -------------===//
+//
----------------
Remove `- Expand the memory access`. No other header does this.


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:12
+// dependencies.
+//===----------------------------------------------------------------------===//
+
----------------
To be consistent with the other files, add an empty line before `//===------`


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:40
+
+  /// Register all analyses and transformation required.
+  void getAnalysisUsage(AnalysisUsage &AU) const override;
----------------
Typo: transformation**s**


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:85
+    auto Loop = ScopStmt->getLoopForDimension(i);
+    auto SCEV = S.getSE()->getMaxBackedgeTakenCount(Loop);
+    SCEVSizes.push_back(SCEV);
----------------
Check for whther this returns `nullptr`. At least an assertion.

Suggestion: Get the domain sizes from the writing statement's domain. 


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:105-108
+  for (unsigned i = 0; i < isl_map_dim(CurrentAccessMap, isl_dim_param); i++) {
+    isl_id *Id = isl_map_get_dim_id(CurrentAccessMap, isl_dim_param, i);
+    NewAccessMap = isl_map_set_dim_id(NewAccessMap, isl_dim_param, i, Id);
+  }
----------------
`isl_dim_in` and `isl_dim_out` dimensions have no dim_id, so this is not necessary.


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:111
+  // Set the new access relation map
+  MA->setNewAccessRelation(NewAccessMap);
+
----------------
At this point, in the regression test, we have
```
{ Stmt_for_cond1_preheader[i0] -> MemRef_tmp_11__phi1_expanded[o0] }
```
(i0 and o0 are unconnected)

That is, every statement instance accesses all array elements. It should be something like:
```
{ Stmt_for_cond1_preheader[i0] -> MemRef_tmp_11__phi1_expanded[i0] }
```


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:135
+      if (MA->isWrite()) {
+        CorrectWrite = expandWrite(S, MA);
+        if (!CorrectWrite) {
----------------
Use inline declarations:
```
bool CorrectWrite = expandWrite(S, MA);
```


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:137
+        if (!CorrectWrite) {
+          return false;
+        }
----------------
We cannot return early with transformations only partially applied. The SCoP representation will be inconsistent and at best passes after this will crash, at worst we miscompile.

Please check in advance if a transformation can be applied before trying to apply it.

In this case, you may want to check to each `ScopArrayInfo` whether it is applicable. The class `ScalarDefUseChains` in DeLICM.cpp might be helpful to get the accesses of a SAI. I think we will sooner-or-later integrate it into ScopInfo.


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:147
+  }
+  return true;
+}
----------------
`runOnScop` returns true if and only if the IR has been modified. We only modify the SCoP representation, therefore we only return `false`.


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:151
+void MaximalStaticExpander::printScop(raw_ostream &OS, Scop &S) const {
+  S.dump();
+}
----------------
In `printScop`, you must print to `OS`. It will print to `stdout` instead to `stderr`. This should also make the regression test simpler.


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:155
+void MaximalStaticExpander::getAnalysisUsage(AnalysisUsage &AU) const {
+  ScopPass::getAnalysisUsage(AU);
+}
----------------
You will need to add `AU.addRequired<DependenceInfo>();` here at some point.


================
Comment at: test/MaximalStaticExpansion/mse___%for.cond1.preheader---%for.end8.jscop:1
+{
+   "arrays" : [
----------------
What is this file needed for?


https://reviews.llvm.org/D34982