Re: Make fix for PR 83965 handle SLP reduction chains
On Mon, Feb 26, 2018 at 3:37 PM, Richard Sandiford
wrote:
> Richard Biener writes:
>> On Tue, Feb 20, 2018 at 5:53 PM, Richard Sandiford
>> wrote:
>>> This patch prevents pattern-matching of fold-left SLP reduction chains,
>>> which the previous patch for 83965 didn't handle properly. It only
>>> stops the last statement in the group from being matched, but that's
>>> enough to cause the group to be dissolved later.
>>>
>>> A better fix would be to put all the information about the reduction
>>> on the the first statement in the reduction chain, so that every
>>> statement in the group can tell what the group is doing. That doesn't
>>> seem like stage 4 material though.
>>>
>>> As it stands, things seem to be a bit of a mess. In
>>> vect_force_simple_reduction we attach the reduction type and
>>> phi pointer to the last statement in a reduction chain:
>>>
>>> reduc_def_info = vinfo_for_stmt (def);
>>> STMT_VINFO_REDUC_TYPE (reduc_def_info) = v_reduc_type;
>>> STMT_VINFO_REDUC_DEF (reduc_def_info) = phi;
>>>
>>> and mark it as vect_reduction_type in vect_analyze_scalar_cycles_1:
>>>
>>> STMT_VINFO_DEF_TYPE (vinfo_for_stmt (reduc_stmt)) =
>>>
>>> vect_reduction_def;
>>>
>>> This code in vectorizable_reduction gave the impression that
>>> everything really is keyed off the last statement:
>>>
>>> /* In case of reduction chain we switch to the first stmt in the chain,
>>> but
>>> we don't update STMT_INFO, since only the last stmt is marked as
>>> reduction
>>> and has reduction properties. */
>>> if (GROUP_FIRST_ELEMENT (stmt_info)
>>> && GROUP_FIRST_ELEMENT (stmt_info) != stmt)
>>> {
>>> stmt = GROUP_FIRST_ELEMENT (stmt_info);
>>> first_p = false;
>>> }
>>>
>>> But this code is dead these days. GROUP_FIRST_ELEMENT is only nonnull
>>> for SLP reduction chains, since we dissolve the group if SLP fails.
>>> And SLP only analyses the first statement in the group, not the last:
>>>
>>> stmt = SLP_TREE_SCALAR_STMTS (node)[0];
>>> stmt_vec_info stmt_info = vinfo_for_stmt (stmt);
>>> [...]
>>> bool res = vect_analyze_stmt (stmt, &dummy, node, node_instance);
>>>
>>> So from that point of view the DEF_TYPE, REDUC_TYPE and REDUC_DEF
>>> are being attached to the wrong statement, since we only analyse
>>> the first one. But it turns out that REDUC_TYPE and REDUC_DEF
>>> don't matter for the first statement in the group, since that
>>> takes the phi as an input, and when the phi is a direct input,
>>> we use *its* REDUC_TYPE and REDUC_DEF instead of the statement's
>>> own info. The DEF_TYPE problem is handled by:
>>>
>>> /* Mark the first element of the reduction chain as reduction to
>>> properly
>>> transform the node. In the reduction analysis phase only the last
>>> element of the chain is marked as reduction. */
>>> if (!STMT_VINFO_GROUPED_ACCESS (vinfo_for_stmt (stmt)))
>>> STMT_VINFO_DEF_TYPE (vinfo_for_stmt (stmt)) = vect_reduction_def;
>>>
>>> in vect_analyze_slp_instance (cancelled by:
>>>
>>> STMT_VINFO_DEF_TYPE (vinfo_for_stmt (first_element))
>>> = vect_internal_def;
>>>
>>> in vect_analyze_slp on failure), with the operation being repeated
>>> in vect_schedule_slp_instance (redundantly AFAICT):
>>>
>>> /* Mark the first element of the reduction chain as reduction to properly
>>> transform the node. In the analysis phase only the last element of the
>>> chain is marked as reduction. */
>>> if (GROUP_FIRST_ELEMENT (stmt_info) && !STMT_VINFO_GROUPED_ACCESS
>>> (stmt_info)
>>> && GROUP_FIRST_ELEMENT (stmt_info) == stmt)
>>> {
>>> STMT_VINFO_DEF_TYPE (stmt_info) = vect_reduction_def;
>>> STMT_VINFO_TYPE (stmt_info) = reduc_vec_info_type;
>>> }
>>>
>>> Tested on aarch64-linux-gnu, x86_64-linux-gnu and powerpc64-linux-gnu.
>>> OK to install?
>>
>> Ok for stage1.
>
> It's a GCC 8 regression, so OK for stage4?
Oh, ok then.
Richard.
> Richard
>
>> Richard.
>>
>>> Richard
>>>
>>>
>>> 2018-02-20 Richard Sandiford
>>>
>>> gcc/
>>> PR tree-optimization/83965
>>> * tree-vect-patterns.c (vect_reassociating_reduction_p): Assume
>>> that grouped statements are part of a reduction chain. Return
>>> true if the statement is not marked as a reduction itself but
>>> is part of a group.
>>> (vect_recog_dot_prod_pattern): Don't check whether the statement
>>> is part of a group here.
>>> (vect_recog_sad_pattern): Likewise.
>>> (vect_recog_widen_sum_pattern): Likewise.
>>>
>>> gcc/testsuite/
>>> PR tree-optimization/83965
>>> * gcc.dg/vect/pr83965-2.c: New test.
>>>
>>> Index: gcc/tree-vect-patterns.c
>>> ===
>>> --- gcc/tree-vect-patterns.c2018-02-20 09:40:41.843451227 +
>>> +++ gcc/tree-vect-pat
Re: Make fix for PR 83965 handle SLP reduction chains
Richard Biener writes:
> On Tue, Feb 20, 2018 at 5:53 PM, Richard Sandiford
> wrote:
>> This patch prevents pattern-matching of fold-left SLP reduction chains,
>> which the previous patch for 83965 didn't handle properly. It only
>> stops the last statement in the group from being matched, but that's
>> enough to cause the group to be dissolved later.
>>
>> A better fix would be to put all the information about the reduction
>> on the the first statement in the reduction chain, so that every
>> statement in the group can tell what the group is doing. That doesn't
>> seem like stage 4 material though.
>>
>> As it stands, things seem to be a bit of a mess. In
>> vect_force_simple_reduction we attach the reduction type and
>> phi pointer to the last statement in a reduction chain:
>>
>> reduc_def_info = vinfo_for_stmt (def);
>> STMT_VINFO_REDUC_TYPE (reduc_def_info) = v_reduc_type;
>> STMT_VINFO_REDUC_DEF (reduc_def_info) = phi;
>>
>> and mark it as vect_reduction_type in vect_analyze_scalar_cycles_1:
>>
>> STMT_VINFO_DEF_TYPE (vinfo_for_stmt (reduc_stmt)) =
>>
>> vect_reduction_def;
>>
>> This code in vectorizable_reduction gave the impression that
>> everything really is keyed off the last statement:
>>
>> /* In case of reduction chain we switch to the first stmt in the chain, but
>> we don't update STMT_INFO, since only the last stmt is marked as
>> reduction
>> and has reduction properties. */
>> if (GROUP_FIRST_ELEMENT (stmt_info)
>> && GROUP_FIRST_ELEMENT (stmt_info) != stmt)
>> {
>> stmt = GROUP_FIRST_ELEMENT (stmt_info);
>> first_p = false;
>> }
>>
>> But this code is dead these days. GROUP_FIRST_ELEMENT is only nonnull
>> for SLP reduction chains, since we dissolve the group if SLP fails.
>> And SLP only analyses the first statement in the group, not the last:
>>
>> stmt = SLP_TREE_SCALAR_STMTS (node)[0];
>> stmt_vec_info stmt_info = vinfo_for_stmt (stmt);
>> [...]
>> bool res = vect_analyze_stmt (stmt, &dummy, node, node_instance);
>>
>> So from that point of view the DEF_TYPE, REDUC_TYPE and REDUC_DEF
>> are being attached to the wrong statement, since we only analyse
>> the first one. But it turns out that REDUC_TYPE and REDUC_DEF
>> don't matter for the first statement in the group, since that
>> takes the phi as an input, and when the phi is a direct input,
>> we use *its* REDUC_TYPE and REDUC_DEF instead of the statement's
>> own info. The DEF_TYPE problem is handled by:
>>
>> /* Mark the first element of the reduction chain as reduction to
>> properly
>> transform the node. In the reduction analysis phase only the last
>> element of the chain is marked as reduction. */
>> if (!STMT_VINFO_GROUPED_ACCESS (vinfo_for_stmt (stmt)))
>> STMT_VINFO_DEF_TYPE (vinfo_for_stmt (stmt)) = vect_reduction_def;
>>
>> in vect_analyze_slp_instance (cancelled by:
>>
>> STMT_VINFO_DEF_TYPE (vinfo_for_stmt (first_element))
>> = vect_internal_def;
>>
>> in vect_analyze_slp on failure), with the operation being repeated
>> in vect_schedule_slp_instance (redundantly AFAICT):
>>
>> /* Mark the first element of the reduction chain as reduction to properly
>> transform the node. In the analysis phase only the last element of the
>> chain is marked as reduction. */
>> if (GROUP_FIRST_ELEMENT (stmt_info) && !STMT_VINFO_GROUPED_ACCESS
>> (stmt_info)
>> && GROUP_FIRST_ELEMENT (stmt_info) == stmt)
>> {
>> STMT_VINFO_DEF_TYPE (stmt_info) = vect_reduction_def;
>> STMT_VINFO_TYPE (stmt_info) = reduc_vec_info_type;
>> }
>>
>> Tested on aarch64-linux-gnu, x86_64-linux-gnu and powerpc64-linux-gnu.
>> OK to install?
>
> Ok for stage1.
It's a GCC 8 regression, so OK for stage4?
Richard
> Richard.
>
>> Richard
>>
>>
>> 2018-02-20 Richard Sandiford
>>
>> gcc/
>> PR tree-optimization/83965
>> * tree-vect-patterns.c (vect_reassociating_reduction_p): Assume
>> that grouped statements are part of a reduction chain. Return
>> true if the statement is not marked as a reduction itself but
>> is part of a group.
>> (vect_recog_dot_prod_pattern): Don't check whether the statement
>> is part of a group here.
>> (vect_recog_sad_pattern): Likewise.
>> (vect_recog_widen_sum_pattern): Likewise.
>>
>> gcc/testsuite/
>> PR tree-optimization/83965
>> * gcc.dg/vect/pr83965-2.c: New test.
>>
>> Index: gcc/tree-vect-patterns.c
>> ===
>> --- gcc/tree-vect-patterns.c2018-02-20 09:40:41.843451227 +
>> +++ gcc/tree-vect-patterns.c2018-02-20 16:28:55.636762056 +
>> @@ -222,13 +222,16 @@ vect_recog_temp_ssa_var (tree type, gimp
>> }
>>
>> /* Return true if STMT_VINFO describes a reduction for which reassociation
>> - i
Re: Make fix for PR 83965 handle SLP reduction chains
On Tue, Feb 20, 2018 at 5:53 PM, Richard Sandiford
wrote:
> This patch prevents pattern-matching of fold-left SLP reduction chains,
> which the previous patch for 83965 didn't handle properly. It only
> stops the last statement in the group from being matched, but that's
> enough to cause the group to be dissolved later.
>
> A better fix would be to put all the information about the reduction
> on the the first statement in the reduction chain, so that every
> statement in the group can tell what the group is doing. That doesn't
> seem like stage 4 material though.
>
> As it stands, things seem to be a bit of a mess. In
> vect_force_simple_reduction we attach the reduction type and
> phi pointer to the last statement in a reduction chain:
>
> reduc_def_info = vinfo_for_stmt (def);
> STMT_VINFO_REDUC_TYPE (reduc_def_info) = v_reduc_type;
> STMT_VINFO_REDUC_DEF (reduc_def_info) = phi;
>
> and mark it as vect_reduction_type in vect_analyze_scalar_cycles_1:
>
> STMT_VINFO_DEF_TYPE (vinfo_for_stmt (reduc_stmt)) =
>vect_reduction_def;
>
> This code in vectorizable_reduction gave the impression that
> everything really is keyed off the last statement:
>
> /* In case of reduction chain we switch to the first stmt in the chain, but
> we don't update STMT_INFO, since only the last stmt is marked as
> reduction
> and has reduction properties. */
> if (GROUP_FIRST_ELEMENT (stmt_info)
> && GROUP_FIRST_ELEMENT (stmt_info) != stmt)
> {
> stmt = GROUP_FIRST_ELEMENT (stmt_info);
> first_p = false;
> }
>
> But this code is dead these days. GROUP_FIRST_ELEMENT is only nonnull
> for SLP reduction chains, since we dissolve the group if SLP fails.
> And SLP only analyses the first statement in the group, not the last:
>
> stmt = SLP_TREE_SCALAR_STMTS (node)[0];
> stmt_vec_info stmt_info = vinfo_for_stmt (stmt);
> [...]
> bool res = vect_analyze_stmt (stmt, &dummy, node, node_instance);
>
> So from that point of view the DEF_TYPE, REDUC_TYPE and REDUC_DEF
> are being attached to the wrong statement, since we only analyse
> the first one. But it turns out that REDUC_TYPE and REDUC_DEF
> don't matter for the first statement in the group, since that
> takes the phi as an input, and when the phi is a direct input,
> we use *its* REDUC_TYPE and REDUC_DEF instead of the statement's
> own info. The DEF_TYPE problem is handled by:
>
> /* Mark the first element of the reduction chain as reduction to
> properly
> transform the node. In the reduction analysis phase only the last
> element of the chain is marked as reduction. */
> if (!STMT_VINFO_GROUPED_ACCESS (vinfo_for_stmt (stmt)))
> STMT_VINFO_DEF_TYPE (vinfo_for_stmt (stmt)) = vect_reduction_def;
>
> in vect_analyze_slp_instance (cancelled by:
>
> STMT_VINFO_DEF_TYPE (vinfo_for_stmt (first_element))
> = vect_internal_def;
>
> in vect_analyze_slp on failure), with the operation being repeated
> in vect_schedule_slp_instance (redundantly AFAICT):
>
> /* Mark the first element of the reduction chain as reduction to properly
> transform the node. In the analysis phase only the last element of the
> chain is marked as reduction. */
> if (GROUP_FIRST_ELEMENT (stmt_info) && !STMT_VINFO_GROUPED_ACCESS
> (stmt_info)
> && GROUP_FIRST_ELEMENT (stmt_info) == stmt)
> {
> STMT_VINFO_DEF_TYPE (stmt_info) = vect_reduction_def;
> STMT_VINFO_TYPE (stmt_info) = reduc_vec_info_type;
> }
>
> Tested on aarch64-linux-gnu, x86_64-linux-gnu and powerpc64-linux-gnu.
> OK to install?
Ok for stage1.
Richard.
> Richard
>
>
> 2018-02-20 Richard Sandiford
>
> gcc/
> PR tree-optimization/83965
> * tree-vect-patterns.c (vect_reassociating_reduction_p): Assume
> that grouped statements are part of a reduction chain. Return
> true if the statement is not marked as a reduction itself but
> is part of a group.
> (vect_recog_dot_prod_pattern): Don't check whether the statement
> is part of a group here.
> (vect_recog_sad_pattern): Likewise.
> (vect_recog_widen_sum_pattern): Likewise.
>
> gcc/testsuite/
> PR tree-optimization/83965
> * gcc.dg/vect/pr83965-2.c: New test.
>
> Index: gcc/tree-vect-patterns.c
> ===
> --- gcc/tree-vect-patterns.c2018-02-20 09:40:41.843451227 +
> +++ gcc/tree-vect-patterns.c2018-02-20 16:28:55.636762056 +
> @@ -222,13 +222,16 @@ vect_recog_temp_ssa_var (tree type, gimp
> }
>
> /* Return true if STMT_VINFO describes a reduction for which reassociation
> - is allowed. */
> + is allowed. If STMT_INFO is part of a group, assume that it's part of
> + a reduction chain and optimistically assume that all statements
> + except the last allow reassociatio
