[llvm-commits] dependence analysis

[Preston Briggs]

Hal, thanks for all the comments.
How should I make/submit these sorts of changes?
Change my source and create a replacement patch file?


On Tue, Jul 31, 2012 at 12:44 PM, Hal Finkel <hfinkel at anl.gov> wrote:
>
> Preston,
>
> Thanks! A few comments:
>
> > +    /// subtractForComparison - When comparing two SCEVs for
> > equality,
> > +    /// we subtract one from the other and test the difference using
> > isZero()
> > +    /// and isKnownNonZero(). Using ordinary subtraction for SCEVs,
> > we get
> > +    /// confused by sign- and zero-extensions, thinking that
> > expressions that
> > +    /// are clearly not equal might be equal. subtractForComparison
> > tries to
> > +    /// avoid this problem.
> > +    const SCEV *subtractForComparison(const SCEV *X, const SCEV *Y)
> > const;
>
> Should we be improving SCEV here? The comment makes this sound like a
> general problem.

I think it's a weakness in the SCEVs.
Basically, I want to know if X and Y can be equal.
Apparently the way to do this is to find the difference,
then test it to see if it's known to be non-zero.
I asked about it earlier, here:
http://lists.cs.uiuc.edu/pipermail/llvmdev/2012-June/050851.html
This is my hack at a solution, but it may be brittle
(in that it may not continue to be useful if people change the
implementation of SCEVs).


> > +    /// collectUpperBound - All subscripts are the same type (on my
> > machine,
> > +    /// an i64). The loop bound may be a smaller type.
> > collectUpperBound
>
> I don't know what your machine is ;) -- if you're going to mention this
> in the comment, say on x86_64/Linux (or whatever), for example, the
> following is true.

Makes sense.


> > +// Right now, the implementation lacks an MIV test and cannot
> > propagate +// constraints between coupled RDIV subscripts. I'll
> > probably implement +// the Power Test as an MIV test.
>
> As a general rule, don't have comments that refer to yourself. I would
> phrase this as, "The Power Test can also be implemented as an MIV test".

Sounds good.


> "cannot propagate constraints between coupled RDIV subscripts" - This
> yield only false positive dependencies, right?

Right. The weakness is mine. The paper explains how to do this particular trick
in a fast and accurate fashion. I don't understand it (yet), so the
implementation
isn't quite as accurate as it should be. That is, it'll be
conservative in the presence
of coupled RDIV subscripts.

When an MIV test gets written, it'll handled the coupled RDIV subscripts,
but (probably) at a greater compile-time cost. To an extent, the
entire Delta test
is about finding the easy cases and solving them quickly, without having to fall
back on a more expensive general approach like the Power Test.


> > +// When the appropriate fix propagates into the release, replace the
> > various +// calls to APInt::sdiv and APInt::srem with APInt::sdivrem,
> > where appropriate. +//
>
> To which fix are you referring. To commit to trunk, you only need the
> fix in trunk ;)

I found an error in the implementation of APInt::sdivrem.
I submitted a fix a while back (but after 3.1).\, hence my confusion.
I'll replace sdiv and srem pairs with sdivrem where appropriate.


> > +// In addition, the implementation depends on the GEP instruction to
> > +// differentiate subscripts. Unfortunately, since Clang linearizes
> > subscripts +// for most arrays, this will be inadequate. We trust
> > that the GEP instruction +// will eventually be extended.
>
> I am not sure about your trust in this; we may need to (and, indeed,
> may want to) delinearize. We should probably discuss this in more
> detail in a different thread. For the purpose of commiting this code,
> I'd remove this comment.

OK. Delinearization wil be fun to work on.
Also, a good MIV test will naturally attack many of these cases.
Long term, we should continue to press for an improved GEP,
'cause it will give us the best results.


> > +// Used to test the dependence analyzer.
> > +// Looks through the function, noting the first store instruction
> > +// and the first load instruction
> > +// (which always follows the first load in our tests).
> > +// Calls depends() and prints out the result.
> > +// Ignores all other instructions.
> > +static
> > +void dumpExampleDependence(raw_ostream &OS, Function *F,
> > +                           DependenceAnalysis *DA) {
> > +  for (inst_iterator SrcI = inst_begin(F), SrcE = inst_end(F);
> > +       SrcI != SrcE; ++SrcI) {
> > +    if (const StoreInst *Src = dyn_cast<StoreInst>(&*SrcI)) {
> > +      for (inst_iterator DstI = SrcI, DstE = inst_end(F);
> > +           DstI != DstE; ++DstI) {
> > +        if (const LoadInst *Dst = dyn_cast<LoadInst>(&*DstI)) {
> > +          if (Dependence *D = DA->depends(Src, Dst, true)) {
> > +            D->dump(OS);
> > +            delete D;
> > +          }
> > +          else
> > +            OS << "none\n";
> > +          return;
> > +        }
> > +      }
> > +    }
> > +  }
> > +}
>
> I think that it would be better to print a prefix here, it will make it
> easier to grep the debug output, and will make sure that the FileCheck
> patterns don't accidentally match 'none' that occurs as a substring of
> something else (like the build path).

Ah, I see. Good idea.


> > +// Returns true is a particular level is scalar; that is,
>
> Typo? (is -> if).

Yes, a typo


> > +// Returns true if if splitting this loop will break the dependence.
>
> Typo? (if if -> if)

Yes, a typo.


> Also, would it make sense to provide a method (or set of methods) which
> help(s) with splitting the loop? I don't understand the workflow
> necessary to eliminate a dependence by splitting a loop using the
> information provided by this pass.

Generally, I expect the dependence analyzer to be used to build
a dependence graph for a function (basically a map from instructions
to dependences). Looking for cycles in the graph shows us loops
that cannot be trivially vectorized/parallelized.

We can try to improve the situation by examining all the dependences
that make up the cycle, looking for ones we can break.
Sometimes, peeling the first or last iteration of a loop will break
dependences, and I've got flags for those possibilities.
Sometimes, splitting a loop at some other iteration will do the trick,
and I've got a flag for that case. Rather than waste the space to
record the exact iteration (since we rarely know), I decided to provide
a method that calculates the iteration. It's a drag that it must work from
scratch, but wonderful in that it's possible.

Here's an example:

    for (i = 0; i < 1000; i++)
        A[i] = ...
        ... = A[1000 - i - 1]

There's a loop-carried flow dependence from the store to the load,
found by the weak-crossing SIV test. The dependence will have a flag,
indicating that the dependence can be broken by splitting the loop.
Calling getSplitIteration will return 500.
Splitting the loop at the 500th iteration, thusly

    for (i = 0; i < 500; i++)
        A[i] = ...
        ... = A[1000 - i - 1]

    for (i = 500; i < 1000; i++)
        A[i] = ...
        ... = A[1000 - i - 1]

breaks the dependence and allows us to vectorize/parallelize
both loops.

Perhaps all this example should make it's way into a comment?


> > +// These are simple getters that hide the implementation
> > +// and check to be sure they are used consistently.
>
> I'm not sure that this comment is necessary. If you wish to keep it,
> then (they are -> it is (implementation is singular)).

"they" was supposed to refer to the getters,
but I'm happy to delete the comment.


> > llvm_unreachable("shouldn't reach the end of Constraint::dump");
>
> I would say, "Unknown constraint type in Constraint::dump"

OK.


> > +  // if (X->isPoint() && Y->isPoint())  This case can't occur.
>
> Then it should be an assert (or removed). As a general rule, no
> commented-out code.

I think the assert is better.
I'd like some marker left in place,
since the case is covered in Figure 4 from the paper.
Better still, I should add a comment arguing why it can't occur,
together with an assertion to the effect.


> > +    // yucko
> > +    return false;
>
> yucko?

An audible sigh, that having done all this work,
we can't glean any useful information.
I'll make a better comment.


> > +// Scrub expressions early, to expose AddRecs.
> > +// Walk through expression looking for sign- and zero-extensions.
> > +// If found, push them down as far as possible, hopefully
> > +// eliminating them entirely.
> > +const SCEV *DependenceAnalysis::scrubExpression(const SCEV *E) const
> > {
>
> May this should be named exposeAddRecs? (I don't like scrubExpression
> b/c that does not really say anything about what it does).

Sounds good.


> > +// Care is required to keep this code up to date w.r.t. the code
> > above.
>
> If this can't be refactored while still keeping the optimization, then
> you should at least also place warning comments in the "code above".

The additional comment makes sense.
I'll think about the refactoring.


> > +  if (Coupled.count()) { // why doesn't Coupled.empty() work
> > correctly?
>
> What do you mean?

I think there's an error in the implementation of empty().
In my tests, sometimes I saw Coupled.count() == 0
but Coupled.empty() == false.
I would think empty() would be slightly faster.

I looked briefly for the problem, but didn't find it.
The comment is a reminder to myself to look harder.

Thanks a lot,
Preston


> On Mon, 30 Jul 2012 17:48:51 -0700
> Preston Briggs <preston.briggs at gmail.com> wrote:
>
> > I've written a dependence analyzer for LLVM. It's a fairly complete
> > implementation of the paper
> >
> > Practical Dependence Testing
> > Gina Goff, Ken Kennedy, and Chau-Wen Tseng
> > PLDI 1991
> >
> >
> > It lacks an MIV test and cannot yet propagate constraints between
> > coupled RDIV subscripts (discussed in Section 5.3.2 of the paper). I
> > expect to add these in the future.
> >
> > It's organized as a FunctionPass with a single entry point that
> > supports testing for dependence between two instructions in a
> > function. If there's no dependence, it returns null. If there's a
> > dependence, it returns a pointer to a Dependence which can be queried
> > about details (what kind of dependence, is it loop independent,
> > direction and distance vector entries, etc). I haven't included every
> > imaginable feature, but there's a good selection that should be
> > adequate for supporting many loop transformations. Of course, it can
> > be extended as necessary.
> >
> > Included in the patch file are many test cases, commented with C code
> > showing the loops and array references.
> >
> > Preston
>
>
>
> --
> Hal Finkel
> Postdoctoral Appointee
> Leadership Computing Facility
> Argonne National Laboratory