Reductions [patch 2 & 3]

[Tobias Grosser]

On 15/06/2014 23:45, jdoerfert at codeaurora.org wrote:
> Comments inlined.

Dito.

>
> --
>
> Johannes Doerfert
> Employee of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> hosted by The Linux Foundation
>
>>>>>> +static cl::opt<bool, true>
>>>>>> +XEnableReductions("polly-reductions",
>>>>>> +                  cl::desc("Relax the reduction dependences before
>>> scheduling"),
>>>>>> +                  cl::Hidden, cl::location(polly::EnableReductions),
>>>>>> +                  cl::init(false), cl::ZeroOrMore,
>>>>>> +cl::cat(PollyCategory)); bool polly::EnableReductions = false;
>>>>>
>>>>> Should we enable this by default? Do we even need an option?
>>> Without an updated code generation I would like to keep it under a flag.
>>
>> Why? This should not cause any correctness issues so enabling it gives
>> us test coverage. Also, the schedule optimizer may already benefit from
>> the increased freedom due to removing the reduction dependences.
> It will affect correctness as soon as the vectorizer or parallelizer is
> used, furthermore we need "privatization dependences" before we can safely
> use it in a sequential environment. As soon as it is safe to enable it
> (maybe only in sequential scenarios) I will.

So there is no way to make the initial patches save by default? I am 
trying to keep patches as small as possible, but if some pieces are 
needed for correctness than we should add those. Specifically, can
we ensure that the analysis the vectorizer/parallizer use do include the 
reduction dependences as long as they do not yet perform privatization.

>>   > Even for unary
>>> reductions there will always be WAW dependences (but then no RAWs).
>>> It should not be necessary to intersect them with RAW, my bad.
>>
>> In my previous mail I was thinking of intersecting them with the union
>> of WAW and RAW dependences. Why is there no need to look at the RAW
>> dependences at all?
> I talked about it in the last mail. I looked at RAW only because I wanted
> it to be restricted to binary reductions, I will from now work with both
> (unary + binary) and look only at WAW dependences.

Sorry. I don't follow this. I don't understand the term unary reduction. 
Also, the relationship between what kind of dependences you look at and 
the reduction types is not clear to me. Can you explain this (possibly 
in a comment).

>>>>> Also, we need to add dependences between S1 and all S2 as well as S2
>>>>> and
>>> all S3, no? Any idea how to do this?
>>>>>
>>>>> S1: A[0] = 0;
>>>>>
>>>>>        for (i)
>>>>> S2:    A[0 += i;
>>>>>
>>>>> S3: A[0] *= 10;
>>> Yes, I call/called them privatization dependences. I know how to do this
>>> and
>>> a patch is ready. However it is easier to do it on memory access level,
>>> thus
>>> it is included in the next group of patches.
>>
>> Why are they called privatization dependences? Even without
>> privatization removing reduction dependences gives the scheduler more
>> freedom. So they don't seem to be related to privatization.
> Because they represent the dependences manual privatization would
> introduce if performed at the outer most possible place (after the last
> write and before the next access to the same location).

OK. That is not obvious. Please explain what they are the first time you 
introduce them.

>> Also, I think we should fix this in this very patch, as otherwise the
>> patch is not self contained because the computed dependences are wrong.
> Without a proper codegen they are wrong when parallel code generation is
> enabled anyway, however I try to add them here too when I change the small
> things.

Sure, we need to adjust the parallelism check in the code generator to 
take into account dependences between reduction dependences as long as
no privatization is supported.

Cheers,
Tobias