[PATCH] D36791: [Polly][Bug fix] Wrong dependences filtering during Fully Indexed expansion

[Andreas Simbuerger via Phabricator via llvm-commits]

simbuerg added a comment.

Apart from the Load/Store union_map issue and the tests, just a few minor remarks.



================
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);
----------------
Is there a specific reason you pass MapDependences by-value and not by-reference as you did with expandRead?


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:207
 
+  // Union_map needed to check if there are load after store in same statement.
+  auto Stores = isl::union_map::empty(S.getParamSpace());
----------------
The comment at this position does not really help me in understanding the following code.

suggestion: I would try to stick to the terminology of your current 'domain'.
Therefore, I would call them reads and writes, not loads and stores.



================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:209
+  auto Stores = isl::union_map::empty(S.getParamSpace());
+  auto Loads = isl::union_map::empty(S.getParamSpace());
+
----------------
Maybe I miss something, but shouldn't you reset those for each Scop Statement? Later you are forming the union of all previous accesses with the current access.


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:234
+                                      "element in same statement.",
+                     MA->getAccessInstruction());
+          return false;
----------------
I would word the remark differently, because this doesn't give the user enough information (IMO). Load after Store in the same statement isn't the problem, the problem is that we risk getting the dependences wrong because of that.

Ideally this should be fixed in polly, nothing the user can do about it.


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:271
 
-        unsigned NumberElementMap =
-            isl_union_map_n_map(CurrentReadWriteDependences.get());
+        // Get the dependences related to MA
+        auto MapDependences = filterDependences(S, Dependences, MA);
----------------
// Get the dependences relevant for this MA


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:321
 
-  auto CurrentReadWriteDependences =
-      Dependences.reverse().intersect_domain(WriteDomain);
+  // Get dependences related to MA
+  auto MapDependences = filterDependences(S, Dependences, MA);
----------------
// Get the dependences relevant for this MA


================
Comment at: test/MaximalStaticExpansion/load_after_store_same_statement.ll:2
+; RUN: opt %loadPolly -polly-canonicalize -polly-mse -analyze < %s | FileCheck %s
+; RUN: opt %loadPolly -polly-canonicalize -polly-mse -pass-remarks-analysis="polly-mse" -analyze < %s 2>&1| FileCheck %s --check-prefix=MSE
+;
----------------
Do not use -polli-canonicalize (or any other transformation, other than the one you want to test) in tests. Run the IR file through -polli-canonicalize and use the output as test. Otherwise you risk testing more than your pass.


================
Comment at: test/MaximalStaticExpansion/working_expansion_multiple_dependences_per_statement.ll:1
+; RUN: opt %loadPolly -polly-canonicalize -polly-mse -analyze < %s | FileCheck %s
+;
----------------
See above.


================
Comment at: test/MaximalStaticExpansion/working_expansion_multiple_instruction_per_statement.ll:1
+; RUN: opt %loadPolly -polly-canonicalize -polly-mse -analyze < %s | FileCheck %s
+;
----------------
See above


https://reviews.llvm.org/D36791