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

[Adam Nemet]

================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:350-352
@@ +349,5 @@
+
+    // Decide if we need to add a check between two groups of pointers,
+    // according to needsAnyChecking.
+    bool hasDependentMembers(struct CheckGroup &M, struct CheckGroup &N,
+                             const SmallVectorImpl<int> *PtrPartition) const;
----------------
How about calling it "needsChecking" then (but on Groups now)?  "Dependent" does not seem specific enough in this context.

Three slashes and \brief in the comment.  There is more of the same later.

================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:351
@@ +350,3 @@
+    // Decide if we need to add a check between two groups of pointers,
+    // according to needsAnyChecking.
+    bool hasDependentMembers(struct CheckGroup &M, struct CheckGroup &N,
----------------
You must mean "needsChecking" here.

================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:357-358
@@ +356,4 @@
+    // between two different groups.
+    void groupChecks(SmallVectorImpl<struct CheckGroup> &GroupedChecks,
+                          const SmallVectorImpl<int> *PtrPartition) const;
+
----------------
If GroupedChecks is the return value here, it should be returned by value rather than passed-by-reference.

================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:181
@@ +180,3 @@
+    // Create a new set containing only this element.
+    unsigned Leader = CheckClass.getOrInsertLeaderValue(I);
+    SetLeaderIndex.push_back(Leader);
----------------
Why isn't this just insert and Leader == I?

================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:190-191
@@ +189,4 @@
+
+    // We don't want to reason about non-inbounds pointers.
+    if (!isInBoundsGep(Pointers[I])) continue;
+
----------------
Why is inbounds relevant here?

================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:209-212
@@ +208,6 @@
+    // which can include this pointer.
+    for (EquivalenceClasses<unsigned>::iterator I = CheckClass.begin(),
+                                             E = CheckClass.end();
+                                             I != E; ++I) {
+       if (!I->isLeader()) continue;
+       unsigned Value = I->getData();
----------------
Please run this through clang-format-diff.py

================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:214
@@ +213,3 @@
+       unsigned Value = I->getData();
+       if (Leader == Value) continue;
+       Value = SetLeaderIndex[Value];
----------------
Can we already have Leader at this point in the EC?

I also think that Value and Leader are not really good names here.  Perhaps "Set" and "Pointer"?

I read this far but I am not sure I understand this algorithm.  It probably needs a big comment at the beginning explaining what's going on.

Also it looks quadratic.  Could we do something better by perhaps analyzing the start values of the ARs and then only trying to match those that share the same base pointer?  We may actually already have these "related" pointers in AccessAnalysis::DepCands.  (The name is not great.  This are candidates for dependence analysis, i.e. pointers that share the same underlying object.)

================
Comment at: test/Analysis/LoopAccessAnalysis/number-of-memchecks.ll:1
@@ -1,4 +1,2 @@
-; RUN: opt -loop-accesses -analyze < %s | FileCheck %s
-
-; 3 reads and 3 writes should need 12 memchecks
+; RUN: opt -O1 -loop-accesses -analyze < %s | FileCheck %s
 
----------------
I don't understand this change.  Now instead of unit-testing a pass, you run the entire -O1 pipeline?!

================
Comment at: test/Analysis/LoopAccessAnalysis/number-of-memchecks.ll:61-79
@@ +60,21 @@
+
+; 3 reads and 2 writes - two of the reads can be merged,
+; and the writes can be merged as well. This gives us a
+; total of 2 memory checks.
+
+; CHECK: function 'testg':
+; CHECK: Memory dependences are safe with run-time checks
+
+; CHECK: Run-time memory checks:
+; CHECK-NEXT: Check 0:
+; CHECK: Check 1:
+; CHECK-NOT: Check 2:
+; CHECK: Group 0:
+; CHECK: Group 1:
+; CHECK: Group 2:
+; CHECK-NOT: Group 3:
+
+define void @testg(i16* %a,
+               i16* %b,
+               i16* %c) {
+entry:
----------------
It would be good to have a C version of these loop in comment as well.

http://reviews.llvm.org/D10386

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