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

[silviu.baranga at arm.com]

Thanks! I've uploaded a new version.

-Silviu


================
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;
----------------
anemet wrote:
> 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.
Good idea! Should be changed now.

================
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);
----------------
anemet wrote:
> Why isn't this just insert and Leader == I?
It is. I also renamed I to Pointer for clarity in the newest version.

================
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;
+
----------------
anemet wrote:
> Why is inbounds relevant here?
I was concerned that possible pointer wrapping might affect the result of some comparisons.
For example if we have a (a pointer ) and c a positive constant, a + c might be smaller than a if we wrap.

However I see that we are already excluding this case somewhere else for all accesses, so it should be ok to remove this.

================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:214
@@ +213,3 @@
+       unsigned Value = I->getData();
+       if (Leader == Value) continue;
+       Value = SetLeaderIndex[Value];
----------------
anemet wrote:
> 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.)
We do have Leader (currently renamed) in the EC (the last one being added).

I've renamed to FirstIndexInSet and Pointer in the latest revision.

The algorithm is quadratic. However, the number of pointers is currently bounded (maximum 100 I believe). Also, the existing needsAnyChecking() etc are also quadratic, and we only run this when we would normally run something that is also quadratic, which made me think that it might not be that bad.

I had a look at the DepCands, and it looks like they would need to be processed further as we want to partition into a sets of pointers which don't need checks within each set (and I don't think that's the case with DepCands). I think this would bring us back at a quadratic algorithm.

One thing that we could do is iterate within DepCands for each pointer instead of iterating over the entire set of pointers. Matching only pointers with the same underlying object is also an option.






================
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
 
----------------
anemet wrote:
> I don't understand this change.  Now instead of unit-testing a pass, you run the entire -O1 pipeline?!
Removed the -O1. However we now don't see some functions as being 'safe', but that should be ok for testing this change.

http://reviews.llvm.org/D10386

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