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

[Adam Nemet]

[Moved this from an inline comment so that it's easier to reply inline]

> Ah, yes. That makes sense. I think it would be possible to check the value in

>  DependencySetId instead of getting access to DepCands?


Yes, I believe so.  There is a fallback case where we disable the MemoryDependenceChecker.  In this case we could disable merging too.

> I agree, the idea behind this is to get the min and max of these objects and

>  use that for the whole class. I think we are essentially doing the same thing

>  here, except we only know how to compare some pairs of pointers (the very

>  simple case where the difference is a constant).

> 

> The biggest problem as far as I can see is that we could in theory have

>  pointers to the same underlying object which we can't order (and it might be

>  impossible to determine the full order in all cases)


I think we already have this distinction available as well.  You're right that this could happen within a DepCands set.  These I believe would be marked Dependence::Unknown.

So perhaps the approach should be to build the groups from the InterestingDependences.  Any known dependence would put the two participating pointers in the same group.

(You may need to start recording forward dependences as well which we don't do currently.)

> I think we have two

>  options in this case:

> 

> - we can create separate groups, where we can order all elements within a group (we do this in the current solution)


I'd go for this initially.

> - we can create umin/umax SCEV expressions - this leads to huge SCEV expressions and is essentially a way to hide the cost of the checks.

> 

>   I will refactor the code so that we will have some comparison function for pointers and no mentions to constants outside it, and that can be easily extended to support more cases for pointer comparisons. Does that sound like a good idea?


I think so but I am not completely sure I understand this last paragraph.

Adam


================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:214
@@ +213,3 @@
+       unsigned Value = I->getData();
+       if (Leader == Value) continue;
+       Value = SetLeaderIndex[Value];
----------------
sbaranga wrote:
> anemet wrote:
> > sbaranga wrote:
> > > 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.
> > > 
> > > 
> > > 
> > > 
> > > 
> > A DepCands equivalence class does not need *runtime* checks between its elements.  (We analyze the accesses within such set with the MemoryDependenceChecker.)
> > 
> > I think that all we may need to do is to find the min start bound and the max end bound of a class within DepCands.  This should cover the range for the entire class.
> > 
> > What do you think?
> Ah, yes. That makes sense. I think it would be possible to check the value in DependencySetId instead of getting access to DepCands?
> 
> I agree, the idea behind this is to get the min and max of these objects and use that for the whole class. I think we are essentially doing the same thing here, except we only know how to compare some pairs of pointers (the very simple case where the difference is a constant).
> 
> The biggest problem as far as I can see is that we could in theory have pointers to the same underlying object which
> we can't order (and it might be impossible to determine the full order in all cases) I think we have two options in this case:
>       - we can create separate groups, where we can order all elements within a group (we do this in the current solution)
>       - we can create umin/umax SCEV expressions - this leads to huge SCEV expressions and is essentially
>         a way to hide the cost of the checks.
> 
> I will refactor the code so that we will have some comparison function for pointers and no mentions to constants outside it, and that can be easily extended to support more cases for pointer comparisons. Does that sound like a good idea?
I can't quote the lines in reply to an inline comment so moving this to the main comment area.

http://reviews.llvm.org/D10386

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