[Polly] Reduction dependences [V3]

[Johannes Doerfert]

What do you think?

> On 19/06/2014 20:02, Johannes Doerfert wrote:
> > I put it into a loop now, this copy and paste code over multiple lines 
> > makes my dizzy sometimes.
> Great, a perfect use of range based for loops.
I have two sets of dependences to handle, so no range loop :(
It still looks much better in my opinion than the repetition did.

> >>> >>So an unary reduction is a statement that only has either an 
> >>> >>incoming or
> > an outcoming reduction dependency, but not both?
> > No, it just has only WAW dependences because it does not read the 
> > reduction location (btw. we do not detect them yet)
> >>> >>  Is the first reduction in a reduction chain also an unary
reduction?
> > No. A unary reduction is simply a loop carried write to the same 
> > location without anyone accessing the location for 2 or more iterations.
> Mh, unary reduction would then probably need some explanation. The other
option is to just not talk about them and just to comment in the context of
what is currently supported.
I skipped unary in the patch (except one FIXME see the end of this message).

> > We are limited to binary reductions at the moment and this is not one.
> > There are never at least two iterations which read __and__ write to the
same
> > location, thus we won't find the RAW and WAW dependences of a reduction,
thus we don't
> > find Reduction dependences.
> Interesting. Can you add this as a comment.
Done

> > I changed it again to "is only almost reduction like", can we go with
that
> > for now?
> I will comment in the patch.
Ok.

> > diff --git a/include/polly/Dependences.h b/include/polly/Dependences.h
> > index 9a704a8..655d4aa 100755
> > --- a/include/polly/Dependences.h
> > +++ b/include/polly/Dependences.h
> > @@ -46,6 +46,12 @@ public:
> >     static char ID;
> >
> >     /// @brief The type of the dependences.
> > +  ///
> > +  /// Reduction dependences are not included in "TYPE_ALL" dependences
> > +  /// as they are no "validity" dependences in the sense that we are
not allowed
> > +  /// to break them. Instead, they can be seen as dependences we want
to fulfill
> > +  /// but won't if it yields e.g., more parallelism.
> This is not very clear. Why do we want to fulfill them but won't
sometimes?
I'll make it more clear.

> It seems the reason is that reduction dependences can be ignored during 
> sequential execution, as the order in which statements are executed is
> not important, as long as two statements are not executed at the same 
> time. However, when executing code in parallel we need to consider the
> dependences to ensure that instructions are not executed in parallel,
> except if we took additional measures (such as privatization) to ensure 
> that parallel writes do not conflict.
Correct. Would this comment work:

// Reduction dependences are not included in TYPE_ALL dependences because we
// can ignore them during the scheduling. This is the case since the order
in
// which the reduction statements are executed does not matter. However, if
// they are executed in parallel we need to take additional measures (e.g.,
// privatization) to ensure a correct result.

Alternatively I could remove the TYPE_ALL and make it explicit everywhere
what
dependences are used.


> > +  /// @brief Calculate and at the privatization dependences
                               > add
Done.

> > +void Dependences::addPrivatizationDependences() {
> > +  isl_union_map *PrivRAW, *PrivWAW, *PrivWAR, *TransClosure;
> > +
> > +  // The transitive closure might be over approximated but we only use
it to
> > +  // compute the privatization dependences. Thus, overapproximation
will lead
> > +  // "only" to more conservative privatization dependences.

> Can this approximation introduce dependences to statements that are 
> scheduled earlier?
I don't know but I doubt it since we start only with strict forward
dependences.
We need to ask Sven if we somehow can get backward dependences due to
overapproximation.

> I am asking because I would like you to _verify_ that there is no way 
> that there will be wrong dependences introduced. If we are not sure, we 
> need to look at the dependences in the context of the schedule and 
> remove all negative dependences. I would like to avoid this.
Why do we need the schedule? We start with dependences strict forward in
some
direction and the result should look the same. So we could verify it like
this
until we know for sure there are no backward dependences created by the
transitive closure.

I was thinking we could do it like this:

 // The transitive closure might be over approximated but we only use it to
 // compute the privatization dependences. Thus, overapproximation will lead
 // "only" to more conservative privatization dependences. However, we take
 // precautions to ensure only forward dependences are created. This will
ensure
 // that we won't create dependency cycles.
 TransClosure = isl_union_map_transitive_closure(isl_union_map_copy(RED),
0);

 // [NEW] check for non positive dependences in the transitive closure and
substract them

 // Use the transitice closure as before


> > +  // 3) Relax the original RAW and WAW dependences by substracting the
actual
> > +  //    reduction dependences. Binary reductions (sum += A[i]) cause
both, and
> > +  //    the same, RAW and WAW dependences, but unary reductions (last =
A[i])
> > +  //    only WAW.
> Just skip the unary stuff. We can discuss it when we add unary
dependences.
Done.

> > +  // FIXME: Reduction dependences prevent parallelization at the
moment.
> // FIXME: We can remove ignore reduction dependences in case we
> // privatize the memory locations the reduction statements reduce into.
> > +  Deps = getDependences(TYPE_ALL | TYPE_RED);
Changed to your version.

> > +bool polly::EnableOpenMP;
> Leftover.
Gone.

> > +  // FIXME: Reduction dependences prevent parallelization at the
moment.
> See above.
Dito.

> > +  // FIXME: Reduction dependences prevent dce at the moment
> Good catch. Why do we need to fix anything here? how would we fix it?
We dont have to fix anything now, I reformulated the comment to make it more
clear:
// FIXME: Unary reductions will prevent dce as they will look live.
//        We should split reduction dependences into binary and unary
//        into RAW and WAW) to avoid this problem.

Later we could split reduction dependences into reduction RAW and reduction
WAW
and only add the RAW ones to the live set. The idea is that binary
reductions
are live at the end while unary are not.

Should I remove the comment as it is only a concern for the not yet
implemented
unary reductions or should I leave it ?

--

Johannes Doerfert
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by
The Linux Foundation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Model-statement-wise-reduction-dependences_v6.patch
Type: application/octet-stream
Size: 49461 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140619/d3751c77/attachment.obj>