[llvm-commits] dependence analysis

[Hal Finkel]

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.

> +    /// 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.

> +// 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".

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

> +//
> +// 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 ;)

> +// 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.

> +// 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).

> +// Returns true is a particular level is scalar; that is,

Typo? (is -> if).

> +// Returns true if if splitting this loop will break the dependence.

Typo? (if if -> if)

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.

> +// 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)).

> llvm_unreachable("shouldn't reach the end of Constraint::dump");

I would say, "Unknown constraint type in Constraint::dump"

> +  // 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.

> +    // yucko
> +    return false;

yucko?

> +// 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).

> +// 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".

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

What do you mean?

 -Hal

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