Re: [PATCH GCC][12/13]Workaround reduction statements for distribution

[Richard Biener <richard.guenther@gmail.com>]

On Fri, Jun 23, 2017 at 12:26 PM, Bin.Cheng <amker.cheng@gmail.com> wrote:
> On Tue, Jun 20, 2017 at 12:36 PM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Tue, Jun 20, 2017 at 11:20 AM, Bin.Cheng <amker.cheng@gmail.com> wrote:
>>> On Fri, Jun 16, 2017 at 6:15 PM, Bin.Cheng <amker.cheng@gmail.com> wrote:
>>>> On Fri, Jun 16, 2017 at 11:21 AM, Richard Biener
>>>> <richard.guenther@gmail.com> wrote:
>>>>> On Mon, Jun 12, 2017 at 7:03 PM, Bin Cheng <Bin.Cheng@arm.com> wrote:
>>>>>> Hi,
>>>>>> For now, loop distribution handles variables used outside of loop as reduction.
>>>>>> This is inaccurate because all partitions contain statement defining induction
>>>>>> vars.
>>>>>
>>>>> But final induction values are usually not used outside of the loop...
>>>> This is in actuality for induction variable which is used outside of the loop.
>>>>>
>>>>> What is missing is loop distribution trying to change partition order.  In fact
>>>>> we somehow assume we can move a reduction across a detected builtin
>>>>> (I don't remember if we ever check for validity of that...).
>>>> Hmm, I am not sure when we can't.  If there is any dependence between
>>>> builtin/reduction partitions, it should be captured by RDG or PG,
>>>> otherwise the partitions are independent and can be freely ordered as
>>>> long as reduction partition is scheduled last?
>>>>>
>>>>>> Ideally we should factor out scev-propagation as a standalone interface
>>>>>> which can be called when necessary.  Before that, this patch simply workarounds
>>>>>> reduction issue by checking if the statement belongs to all partitions.  If yes,
>>>>>> the reduction must be computed in the last partition no matter how the loop is
>>>>>> distributed.
>>>>>> Bootstrap and test on x86_64 and AArch64.  Is it OK?
>>>>>
>>>>> stmt_in_all_partitions is not kept up-to-date during partition merging and if
>>>>> merging makes the reduction partition(s) pass the stmt_in_all_partitions
>>>>> test your simple workaround doesn't work ...
>>>> I think it doesn't matter because:
>>>>   A) it's really workaround for induction variables.  In general,
>>>> induction variables are included by all partition.
>>>>   B) After classify partition, we immediately fuses all reduction
>>>> partitions.  More stmt_in_all_partitions means we are fusing
>>>> non-reduction partition with reduction partition, so the newly
>>>> generated (stmt_in_all_partitions) are actually not reduction
>>>> statements.  The workaround won't work anyway even the bitmap is
>>>> maintained.
>>>>>
>>>>> As written it's a valid optimization but can you please note it's limitation in
>>>>> some comment please?
>>>> Yeah, I will add comment explaining it.
>>> Comment added in new version patch.  It also computes bitmap outside
>>> now, is it OK?
>>
>> Ok.  Can you add a testcase for this as well please?  I think the
>> series up to this
>> is now fully reviewed, I defered 1/n (the new IFN) to the last one
>> containing the
>> runtime versioning.  Can you re-post that (you can merge with the IFN patch)
>> to apply after the series has been applied up to this?
> Test case added.

Ok.

> Thanks,
> bin
> 2017-06-20  Bin Cheng  <bin.cheng@arm.com>
>
>     * tree-loop-distribution.c (classify_partition): New parameter and
>     better handle reduction statement.
>     (rdg_build_partitions): Revise comment.
>     (distribute_loop): Compute statements in all partitions and pass it
>     to classify_partition.
>
> gcc/testsuite/ChangeLog
> 2017-06-20  Bin Cheng  <bin.cheng@arm.com>
>
>     * gcc.dg/tree-ssa/ldist-26.c: New test.