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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur accepted this revision.
Meinersbur added a comment.
This revision is now accepted and ready to land.

To have any effect, we need to clear the DependenceInfo, otherwise `-polly-opt-isl` will still use the unexpanded dependence info and the mse was useless.

DependenceInfo has no facility yet to reset a previous analysis. You might want want to add one into this patch or a follow-up one.



================
Comment at: lib/Support/RegisterPasses.cpp:149
+static cl::opt<bool> FullyIndexedStaticExpansion(
+    "polly-mse",
+    cl::desc("Fully expand the memory accesses of the detected Scops"),
----------------
[Suggestion] To be consistent with other switches that add passes, name this `-polly-enable-mse` and the pass itself `-polly-mse` (instead of `-polly-opt-mse`)?


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:67
+  bool isExpandable(const ScopArrayInfo *SAI,
+                    SmallPtrSet<MemoryAccess *, 4> &Writes,
+                    SmallPtrSet<MemoryAccess *, 4> &Reads, Scop &S,
----------------
[Style] Use `SmallPtrSetImpl<MemoryAccess *>` to not require the small size in the parameter.


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:187
+
+      // Check if it is possible to extand this read.
+      if (MA->isRead()) {
----------------
[Typo] extand -> expand


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:218
+          emitRemark(SAI->getName() +
+                         " expansion leads to a partial read access.",
+                     MA->getAccessInstruction());
----------------
The consequence would not be a partial read access, but it would need to read the original value the element had before entering the SCoP. That's a special case similar to having more than one write.


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:307
+           "The upper bound is not a positive integer.");
+    Sizes.push_back(UpperBound.get_num_si() + 1);
+  }
----------------
[Nit] The UpperBound could overflow a long. Add an assertion for that?


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:328-331
+    auto Constraints = isl::constraint::alloc_equality(ls);
+    Constraints = Constraints.set_coefficient_si(isl::dim::out, dim, 1);
+    Constraints = Constraints.set_coefficient_si(isl::dim::in, dim, -1);
+    NewAccessMap = NewAccessMap.add_constraint(Constraints);
----------------
[Style] This could be simpler using
```
NewAccessMap = NewAccessMap->equate(isl::dim::in, dim. isl::dim::out, dim);
```
or, even, better, use `basic_map::equal`.


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:347
+
+  // Get the ORE from Optimizationremarkemitterwrapperpass.
+  ORE = &(getAnalysis<OptimizationRemarkEmitterWrapperPass>().getORE());
----------------
[Nit] `OptimizationRemarkEmitterWrapperPass`


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:357-359
+  for (auto &SAI : S.arrays()) {
+    CurrentSAI.insert(SAI);
+  }
----------------
[Style] No braces around single statements.

Also possible:
```
SmallPtrSet<ScopArrayInfo *, 4> CurrentSAI(S.array_begin(), array_end());
```


================
Comment at: test/MaximalStaticExpansion/partial_access.ll:1
+; RUN: opt -polly-canonicalize %loadPolly -analyze -polly-opt-mse -pass-remarks-analysis="polly-mse" < %s 2>&1 | FileCheck %s
+;
----------------
The interleaving of stdout (`-analyze`) and stderr (`-pass-remarks-analysis`) is undefined. It is better to have two separate RUN lines, one checking `analyze`, the other `-pass-remarks-analysis`.


https://reviews.llvm.org/D34982