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

[Adam Nemet]

Wow, looks great, IMO.  Thanks for your work, Silviu!

Some minor things:


================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:346
@@ +345,3 @@
+
+      /// \brief Tries to add the pointer recored in RtCheck at index Index
+      /// to this pointer checking group. We can only add a pointer
----------------
s/recored/recorded

================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:357-362
@@ +356,8 @@
+      RuntimePointerCheck &RtCheck;
+      /// Index of the pointer which will represent the upper bound
+      /// of the memcheck.
+      const SCEV *High;
+      /// Index of the pointer which will represent the lower bound
+      /// of the memcheck.
+      const SCEV *Low;
+      /// Indices of all the pointers that constitute this grouping.
----------------
These are not indices but SCEVs.

For the record, I agree that they should be SCEVs but the comment is wrong.

================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:420-421
@@ +419,4 @@
+    SmallVector<const SCEV *, 2> Exprs;
+    /// Holds a partitioning of pointers into "check groups".
+    SmallVector<struct CheckingPtrGroup, 2> CheckGroups;
+    /// Holds a pointer to the ScalarEvolution analysis.
----------------
How about CheckingGroups?

================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:150
@@ +149,3 @@
+
+/// Compare I and J and return the minimum.
+/// Return nullptr in case we couldn't find an answer.
----------------
Put \p before I and J

================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:231-242
@@ +230,14 @@
+      // which can include this pointer.
+      for (auto EI = Groups.begin(), EE = Groups.end(); EI != EE; ++EI) {
+        // Don't perform more than a certain amount of comparisons.
+        // This should limit the cost of grouping the pointers to something
+        // reasonable.
+        if (TotalComparisons++ > MemoryCheckMergeThreshold)
+          break;
+
+        if (EI->addPointer(Pointer)) {
+          Merged = true;
+          break;
+        }
+      }
+
----------------
Looks like you could formulate this with a range-based loop.

================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:235-236
@@ +234,4 @@
+        // reasonable.
+        if (TotalComparisons++ > MemoryCheckMergeThreshold)
+          break;
+
----------------
I think that you also want to break out from the entire loop-nest here.  (I'd probably go with a goto in this case.)

================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:253-254
@@ +252,4 @@
+    // Save the results and continue with the next one.
+    for (auto I = Groups.begin(), E = Groups.end(); I != E; ++I)
+      CheckGroups.push_back(*I);
+  }
----------------
Can this be written with std::copy?  I.e.:

  std::copy(Groups.begin(), Groups.end(), std::back_inserter(CheckGroups))

================
Comment at: test/Analysis/LoopAccessAnalysis/unsafe-and-rt-checks.ll:16-19
@@ -15,5 +15,6 @@
 ; CHECK: Run-time memory checks:
 ; CHECK-NEXT: 0:
+; CHECK-NEXT:   %arrayidxA = getelementptr inbounds i16, i16* %a, i64 %storemerge3
 ; CHECK-NEXT:   %arrayidxA_plus_2 = getelementptr inbounds i16, i16* %a, i64 %add
 ; CHECK-NEXT:   %arrayidxB = getelementptr inbounds i16, i16* %b, i64 %storemerge3
 ; CHECK-NEXT: 1:
----------------
Can we make it clearer where one group ends and the other one starts when printing a check?

http://reviews.llvm.org/D10386

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