[PATCH] [LAA] Merge memchecks for accesses separated by a constant offset

[Adam Nemet]

Longish comment down is my main question.  We should probably resolve that first.


================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:335
@@ -334,1 +334,3 @@
 
+    /// A grouping a pointers. A single memcheck is required between
+    /// two groups.
----------------
A grouping *of* pointers

================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:337
@@ +336,3 @@
+    /// two groups.
+    struct CheckGroup {
+      CheckGroup() : High(0), Low(0) {}
----------------
CheckingGroup or CheckingPtrGroup probably sounds better.

================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:338
@@ +337,3 @@
+    struct CheckGroup {
+      CheckGroup() : High(0), Low(0) {}
+      /// Index of the pointer wich will represent the upper bound
----------------
s/0/nullptr

================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:339
@@ +338,3 @@
+      CheckGroup() : High(0), Low(0) {}
+      /// Index of the pointer wich will represent the upper bound
+      /// of the memcheck.
----------------
s/wich/which

later too

================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:166-171
@@ +165,8 @@
+    MemoryDepChecker::DepCandidates &DepCands) {
+  // We build the groups from dependency candidates equivalence classes
+  // because:
+  //    - we know that pointers in the same equivalence class share
+  //      the same underlying object and therefore there is a chance
+  //      that we can compare pointers
+  //    - we don't need to check two pointers in the same equivalence class.
+
----------------
I don't get this last point -- seems pretty recursive.

================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:194
@@ +193,3 @@
+  SmallVector<unsigned, 2> Maxs(Pointers.size());
+  // Same as the Maxs vector, only holds indeces of mins.
+  SmallVector<unsigned, 2> Mins(Pointers.size());
----------------
s/indeces/indices

================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:196-199
@@ +195,6 @@
+  SmallVector<unsigned, 2> Mins(Pointers.size());
+  // For each pointer, holds the index to the first element
+  // that was added to the pointers equivalence class. We do this
+  // because the leader of an equivalence class can change, and
+  // would like to always compare against the same element.
+  SmallVector<unsigned, 2> FirstInSetIndex(Pointers.size());
----------------
Why do we need to compare against the same element?

================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:202-213
@@ +201,14 @@
+
+  // Go through all equivalence classes, get the the "pointer check groups"
+  // and add them to the overall solution.
+  while (DI != DE) {
+    auto MI = DepCands.member_begin(DI);
+    auto ME = DepCands.member_end();
+
+    // Each pointer is part of an equivalence class. If two
+    // pointers are part of the same equivalence class, they will
+    // be part of the same group.
+    EquivalenceClasses<unsigned> CheckClass;
+
+    while (MI != ME) {
+      unsigned Pointer = PositionMap[MI->getPointer()];
----------------
Shouldn't we only look through the members if DI is a leader?

================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:208-211
@@ +207,6 @@
+
+    // Each pointer is part of an equivalence class. If two
+    // pointers are part of the same equivalence class, they will
+    // be part of the same group.
+    EquivalenceClasses<unsigned> CheckClass;
+
----------------
I don't understand what you gain by using an EC for this.

It seems to me that you just want a vector of CheckGroups for each DepCands set.

For each pointer then you go through this vector and try to merge to an existing group.

If nothing found you add a new group at the end of the vector.

As another general comment, it would be good to push some of the low-level mechanics into the CheckingGroup class so that the outline of algorithm is separated from the details.

================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:242-243
@@ +241,4 @@
+        // reasonable.
+        TotalComparisons++;
+        if (TotalComparisons > MemoryCheckMergeThreshold)
+          break;
----------------
Nit: fold the increment in the comparison?

http://reviews.llvm.org/D10386

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/