[PATCH] D13831: [Polly][FIX][WIP] Restructure invariant load equivalence classes

[Tobias Grosser via llvm-commits]

grosser added a comment.

Hi Johannes,

thanks for working on this. This fixes PR25150 for me and also makes the boost::ublas test case work again. Very nice! PR25137 has not been reproducible anymore.

Most of these changes are in equivalence class code. This means, even thought the patch is large, it should have little impact on the remainder of Polly. This is good. However, if there are possibilities to split this patch into smaller pieces this would be very much appreciated. This does not only make it easier for other people to understand the changes, but also will help in case we need to bisect bugs in the future. Maybe you could split off the type casting stuff?

Also, a couple of the test cases you added are super large. I understand these are only regression tests, but I believe we should still bugpoint reduce them.  A 1000 line is unlikely to give a lot of benefits.

Besides this just a couple of minor typos/grammar items I pointed out.

Feel free to commit, if possible in pieces.


================
Comment at: include/polly/CodeGen/IslNodeBuilder.h:18
@@ -17,2 +17,3 @@
+
 #include "polly/CodeGen/IslExprBuilder.h"
 #include "polly/CodeGen/LoopGenerators.h"
----------------
Why do you add an empty line?

================
Comment at: include/polly/ScopInfo.h:682
@@ -682,1 +681,3 @@
+using InvariantEquivClassTy =
+    std::tuple<const SCEV *, MemoryAccessList, isl_set *>;
 
----------------
Could you describe what the different elements stand for / describe?

I know this was not here before, but reading through this would like to understand what the isl_set * is e.g. supposed to represent.

================
Comment at: include/polly/ScopInfo.h:1138
@@ +1137,3 @@
+  /// @brief Mapping from invariant loads to the representing invariant load of
+  ///        their equivalnce class.
+  ValueToValueMap InvEquivClassVMap;
----------------
equvalence

================
Comment at: include/polly/ScopInfo.h:1226
@@ -1227,1 +1225,3 @@
 
+  /// @brief Add invairant loads listed in @p InvMAs with the domain of @p Stmt.
+  void addInvariantLoads(ScopStmt &Stmt, MemoryAccessList &InvMAs);
----------------
invariant

================
Comment at: lib/Analysis/ScopInfo.cpp:1485
@@ -1484,3 +1412,1 @@
-
-  isl_set_free(DomainCtx);
 }
----------------
Great to have less functionality in this function. It was really a little long before.

================
Comment at: lib/Analysis/ScopInfo.cpp:2454
@@ +2453,3 @@
+  if (!LInst)
+    return nullptr;
+
----------------
Evtl? "auto *Load ="

================
Comment at: lib/Analysis/ScopInfo.cpp:2457
@@ +2456,3 @@
+  if (Value *Rep = InvEquivClassVMap.lookup(LInst))
+    LInst = cast<LoadInst>(Rep);
+
----------------
If this map is known to only contain elements of type LoadInst, could we change the type of the map accordingly?

================
Comment at: lib/Analysis/ScopInfo.cpp:2475
@@ +2474,3 @@
+
+  // Project out all parameters that relate to loads in this statement that
+  // we will hoist. Otherwise we would have cyclic dependences on the
----------------
this statement -> the statement

================
Comment at: lib/Analysis/ScopInfo.cpp:2493
@@ +2492,3 @@
+  for (MemoryAccess *MA : InvMAs) {
+
+    // Check for another invariant access that accesses the same location as
----------------
Why an empty line here?

================
Comment at: lib/Analysis/ScopInfo.cpp:2680
@@ -2679,3 +2608,1 @@
-  std::stable_sort(InvariantEquivClasses.begin(), InvariantEquivClasses.end(),
-                   compareInvariantAccesses);
 }
----------------
Nice to see this go away.

================
Comment at: test/Isl/CodeGen/inv-load-lnt-crash-cyclic-dependences.ll:2
@@ +1,3 @@
+; RUN: opt %loadPolly -polly-codegen -S < %s
+;
+; This crashed at some point as we did not simplify the unified execution domain of the
----------------
Some of these test cases are very big. Are these minimal test cases as produced by bugpoint?

================
Comment at: test/Isl/CodeGen/inv-load-lnt-crash-wrong-order-2.ll:1089
@@ +1088,2 @@
+
+!0 = !{!"clang version 3.8.0 (http://llvm.org/git/clang.git 5e171f9c91fb59933382f67a589af82fd8409eb7)"}
----------------
A one thousand line test case? Was this bugpoint reduced?


http://reviews.llvm.org/D13831