[Polly] Reduction dependences [V3]

[Tobias Grosser]

On 19/06/2014 23:12, Johannes Doerfert wrote:
> 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.

Anyhow. ;-)

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

Works.

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

Thinking more about it, I am afraid with an arbitrary schedule and any 
kind of approximation, we may cause backwards dependences.

>> >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?

To understand if a dependency is forward.

S[i] -> S[i+1] is a backwards dependency in case the schedule is S[i] -> 
[-i].

> 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

Right. Just remove any backwards dependences according to the schedule 
from the transitive closure.

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

Yes, just drop it.

As I think that it is highly unlikely that the over approximation will 
cause any troubles, I am OK with you committing this patch as it is now. 
You can (and should) commit a subsequent patch that fixes the loop hole 
in the overapproximation.

Tobias