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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

[Suggestion] Add a regression test where generated LLVM-IR is tested.



================
Comment at: include/polly/ScopInfo.h:840-842
+  /// Get the original access function as read from IR.
+  __isl_give isl_map *getOriginalAccessRelation() const;
+
----------------
[Style] Why was this moved?


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:47
+  // The set of not expandable SAI.
+  std::set<const ScopArrayInfo *> NotExpandables;
+
----------------
[Style] Instead of a set of not expandable arrays, why not using a set of expandable arrays? It feels safer because if an array is just missing in the set for whatever reason, it would not default to expand it.

[Style] `std::set` is rarely used in LLVM. There are alternitives: See http://llvm.org/docs/ProgrammersManual.html#llvm-adt-smallset-h .


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:51
+  // The parameter @p Dependences is the RAW dependences of the SCoP @p S.
+  void checkExpendability(Scop &S, isl::union_map &Dependences);
+
----------------
[Typo] checkExp**a**ndability


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:59-61
+  // Expand the read memory access @p MAP belonging to the SCoP @p S.
+  // The parameter @p Writes is all the write memory accesses of the SCoP @p S.
+  // The parameter @p Dependences is the RAW dependences of the SCoP @p S.
----------------
[Style] Please make them doxygen comments (`///`)

The doxygen style for parameters we usually use is.
```
@param Dependences The RAW dependences of the SCoP @p S.
```


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:77
+    int NumberWrites = 0;
+    bool NotExpandable = false;
+    for (ScopStmt &Stmt : S) {
----------------
[Style] Negation in variable name and double negation. Why not `bool Expandable = true`?


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:124-129
+          unsigned NumberElementMap = 0;
+          CurrentReadWriteDependences.foreach_map(
+              [=, &NumberElementMap](isl::map Map) -> isl::stat {
+                NumberElementMap++;
+                return isl::stat::ok;
+              });
----------------
[Style] `NumerElementMap = isl_union_map_n_map(CurrentReadWriteDependences.get())`


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:143-144
+          if (!StmtDomain.is_subset(DepsDomainSet)) {
+            errs() << "MSE ERROR : " << SAI->getName()
+                   << " expansion leads to a partial read access. \n";
+            NotExpandable = true;
----------------
[Serious] In normal operation, do not print anything. Users expect only clang warnings and errors.

Alternatives are:
1. `DEBUG(dbgs() << "")` (discurraged for regression tests, this is meant for debugging)
2. Print remarks using `-pass-remarks-missed`
3. Print information at `-analysis`
4. `STATISTIC`


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:214-218
+  unsigned in_dimensions = CurrentAccessMap.dim(isl::dim::in);
+  unsigned out_dimensions = CurrentAccessMap.dim(isl::dim::out);
+  if (in_dimensions == out_dimensions) {
+    return;
+  }
----------------
[Serious] This is not a sufficient condition for full expansion. E.g. one dimension can be a static `0`.
There is also more than one possibility for full expansion (e.g. one starting at 0, another at 1). The reads must access the correct element. So if you want to implement this as a heuristic that expansion is not worth it in this case, implement it in `checkExpandability` such that reads are also not modified.


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:249-250
+    auto Loop = ScopStmt->getLoopForDimension(i);
+    auto SCEV = S.getSE()->getMaxBackedgeTakenCount(
+        Loop); // +1 but this will change later to the ISL version
+    SCEVSizes.push_back(SCEV);
----------------
[Serious] I think `getMaxBackedgeTakenCount` can fail in cases where Polly is still able to detect affine loops. Please add at least an `assert` that SCEV is not `SCEVCountNotCompute`.


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:255
+  // Get the ElementType of the current SAI
+  auto ElementType = MA->getOriginalScopArrayInfo()->getElementType();
+
----------------
[Suggestion] Should this be `getLatestScopArrayInfo()` ?


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



================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:302-304
+      if (!isExpandable(SAI)) {
+        continue;
+      }
----------------
[Style] We usually do not use parenthesis around single statements:
```
if (isExpandable(SAI))
  continue;
```


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:313
+
+  // Expand all expandable read MemoryAccesses
+  for (ScopStmt &Stmt : S) {
----------------
[Style] In Polly's coding style, all sentences end with a dot (but I personally don't care).


https://reviews.llvm.org/D34982