[PATCH] D33362: [Polly][WIP]JSCoP Importer: support for multi-valued schedules

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

Did you add files with `git add -N`? I cannot apply the patch. Please make the diff against `origin/master` (or use arcanist).

In https://reviews.llvm.org/D33362#834967, @Mike_Jongen wrote:

> Fixed the issue of the memory accesses by using -polly-codegen-generate-expressions
>  I would like to include the functionality of this option to the patch, but I am not yet sure how to do this.
>
> It now does generate the correct working code, as long as -polly-codegen-generate-expressions is used when generating code.


Codegen decides based on `hasNewAccessRelation()` whether to generate new index expressions, otherwise it tries to reuse the old one (by copying the instruction from the original code).
If it is necessary for this situation to always generate new expression, try calling `(setNewAccessRelation(getLatestAccessRelation()))`.

However, it would be preferable if reusing old instructions would be possible, since it is the only way to handle accesses with non-affine indices.

I will have a look when I can apply the patch.



================
Comment at: lib/Exchange/JSONExporter.cpp:329
 
+// finds the ScopStmt corresponding to Map.
+ScopStmt *findStmtFromMap(Scop &S, isl::map Map) {
----------------
[Style] Can you use doxygen-style comments (triple slash `///`)?


================
Comment at: lib/Exchange/JSONExporter.cpp:330
+// finds the ScopStmt corresponding to Map.
+ScopStmt *findStmtFromMap(Scop &S, isl::map Map) {
+  isl::map Universe;
----------------
[Style] Can you make the function (and the following if possible) `static`?


================
Comment at: lib/Exchange/JSONExporter.cpp:349
+
+isl::stat makeStmtSingleValued(Scop &S, isl::union_map *NewSchedule,
+                               isl::map MapToAdd) {
----------------
[Suggestion] Could you add a doxygen-style describing the function?


================
Comment at: lib/Exchange/JSONExporter.cpp:353
+
+  assert((LexMin = MapToAdd.lexmin()) && "Map is not lower-bounded!");
+  if (!MapToAdd.is_equal(LexMin)) {
----------------
[Serious] Asserts are removed from `NDEBUG` builds. Try
```
isl::map LexMin= MapToAdd.lexmin();
assert(LexMin && "Map is not lower-bounded!");
```


================
Comment at: lib/Exchange/JSONExporter.cpp:378
+    while (!MapToAdd.is_empty()) {
+      assert((LexMin = MapToAdd.lexmin()) && "Map is not lower-bounded!");
+      MapToAdd = MapToAdd.subtract(LexMin);
----------------
[Serious] See above.


https://reviews.llvm.org/D33362