[PATCH] D36791: [Polly][Bug fix] Wrong dependences filtering during Fully Indexed 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.

LGTM.



================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:91
+  /// @param MA The memory access that need to be expanded.
+  isl::union_map filterDependences(Scop &S, isl::union_map &MapDependences,
+                                   MemoryAccess *MA);
----------------
[Suggestion] If the isl::union_map is not modfied in the function, either pass values (which just 'costs' a reference counter increase+decrease) of by a const reference. With just a reference I'd assume that the function is meant to modify/return the argument.


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:166
+
+        // Filter out Statement to Statement dependences.
+        if (!Map.can_curry())
----------------
Why? How can this happen?


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:173
+        auto Id = SAI->getBasePtrId();
+        auto MARangeId = MA->getLatestAccessRelation().range().get_tuple_id();
+
----------------
[Suggestion] `MA->getLatestAccessRelation().get_tuple_id(isl::dim::out)` is cheaper because it does not require projecting-out the domain dimensions.

[Suggestion] `MA` does not change during each iteration, you can hoist this before the `foreach_map` invocation.


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:175-176
+
+        auto TmpMap = Map.curry().range().wrapped_domain_map().curry();
+        auto TmpMapDomainId = TmpMap.domain().get_tuple_id();
+
----------------
[Suggestion] Do this on `Map.get_space()`, so the projections don't need to be computed.


https://reviews.llvm.org/D36791