[Bug tree-optimization/105940] New: suggested_unroll_factor applying place looks wrong

[Bug tree-optimization/105940] New: suggested_unroll_factor applying place looks wrong

[linkw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105940

            Bug ID: 105940
           Summary: suggested_unroll_factor applying place looks wrong
           Product: gcc
           Version: 13.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: linkw at gcc dot gnu.org
  Target Milestone: ---

I tried to evaluate if we can get some performance gains by setting
suggested_unroll_factor on Power, but met one ICE coming from the line:

          cached_vf_per_mode[last_mode_i]
            = exact_div (LOOP_VINFO_VECT_FACTOR (loop_vinfo),
                         loop_vinfo->suggested_unroll_factor);

With below simple hacking in rs6000 backend:

diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
index d7a7cfe860f..dcf2e8fc0ba 100644
--- a/gcc/config/rs6000/rs6000.cc
+++ b/gcc/config/rs6000/rs6000.cc
@@ -5490,6 +5490,8 @@ rs6000_cost_data::finish_cost (const vector_costs
*scalar_costs)
           && LOOP_VINFO_VECT_FACTOR (loop_vinfo) == 2
           && LOOP_REQUIRES_VERSIONING (loop_vinfo))
         m_costs[vect_body] += 10000;
+
+      m_suggested_unroll_factor = 4;
     }

   vector_costs::finish_cost (scalar_costs);


We can get the ICE reproduced on the below reduced test case:

_Complex *a;
_Complex b, e;
int c, d;
void f() {
  _Complex g;
  for (; d; d++)
    g += a[d * c] * e;
  b = g;
}

option: -Ofast -mcpu=power10

[Bug tree-optimization/105940] suggested_unroll_factor applying place looks wrong

[linkw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105940

--- Comment #1 from Kewen Lin <linkw at gcc dot gnu.org> ---
Created attachment 53126
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=53126&action=edit
move_applying

[Bug tree-optimization/105940] suggested_unroll_factor applying place looks wrong

[linkw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105940

Kewen Lin <linkw at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2022-06-13
           Assignee|unassigned at gcc dot gnu.org      |linkw at gcc dot gnu.org
                 CC|                            |rguenth at gcc dot gnu.org,
                   |                            |rsandifo at gcc dot gnu.org

[Bug tree-optimization/105940] suggested_unroll_factor applying place looks wrong

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105940

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Kewen Lin from comment #1)
> Created attachment 53126 [details]
> move_applying

LGTM (maybe the suggested unroll factor should be only applied if the
suggestion was from a matching with/without SLP analysis, or in fact
vect_analyze_loop_1 should communicate that down - disabling SLP when
the one suggesting unrolling did the re-analysis).

[Bug tree-optimization/105940] suggested_unroll_factor applying place looks wrong

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105940

--- Comment #3 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Kewen Lin <linkw@gcc.gnu.org>:

https://gcc.gnu.org/g:f907cf4c07cf51863dadbe90894e2ae3382bada5

commit r13-1083-gf907cf4c07cf51863dadbe90894e2ae3382bada5
Author: Kewen Lin <linkw@linux.ibm.com>
Date:   Tue Jun 14 00:57:01 2022 -0500

    vect: Move suggested_unroll_factor applying [PR105940]

    As PR105940 shown, when rs6000 port tries to assign
    m_suggested_unroll_factor by 4 or so, there will be ICE on:

      exact_div (LOOP_VINFO_VECT_FACTOR (loop_vinfo),
                 loop_vinfo->suggested_unroll_factor);

    In function vect_analyze_loop_2, the current place of
    suggested_unroll_factor applying can't guarantee it's
    applied for all cases.  As the case shows, vectorizer
    could retry with SLP forced off, the vf is reset by
    saved_vectorization_factor which isn't applied with
    suggested_unroll_factor before.  It means it can end
    up with one vf which neglects suggested_unroll_factor.
    I think it's off design, we should move the applying
    of suggested_unroll_factor after start_over.

            PR tree-optimization/105940

    gcc/ChangeLog:

            * tree-vect-loop.cc (vect_analyze_loop_2): Move the place of
            applying suggested_unroll_factor after start_over.

[Bug tree-optimization/105940] suggested_unroll_factor applying place looks wrong

[linkw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105940

--- Comment #4 from Kewen Lin <linkw at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #2)
> (In reply to Kewen Lin from comment #1)
> > Created attachment 53126 [details]
> > move_applying
> 
> LGTM (maybe the suggested unroll factor should be only applied if the
> suggestion was from a matching with/without SLP analysis, or in fact
> vect_analyze_loop_1 should communicate that down - disabling SLP when
> the one suggesting unrolling did the re-analysis).

Oops, just noticed the nice suggestion.  Will make a follow up patch for this.
It would looks like:
  when working out suggested unroll factor, save slp decision into one passed
down variable from vect_analyze_loop_1.
  when applying suggested unroll factor, if the save slp is false, directly
ignore slp handlings, otherwise, go the normal slp path but won't start over
for slp off.

[Bug tree-optimization/105940] suggested_unroll_factor applying place looks wrong

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105940

--- Comment #5 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The trunk branch has been updated by Richard Sandiford <rsandifo@gcc.gnu.org>:

https://gcc.gnu.org/g:2636660b6f35423e0cfbf53bfad5c5fed6ae6471

commit r13-1103-g2636660b6f35423e0cfbf53bfad5c5fed6ae6471
Author: Richard Sandiford <richard.sandiford@arm.com>
Date:   Wed Jun 15 11:12:51 2022 +0100

    aarch64: Revert bogus fix for PR105254

    In f2ebf2d98efe0ac2314b58cf474f44cb8ebd5244 I'd forced the
    chosen unroll factor to be a factor of the VF, in order to
    work around an exact_div ICE in PR105254.  This was completely
    bogus -- clearly I didn't look in enough detail at why we ended
    up with an unrolled VF that wasn't a multiple of the UF.

    Kewen has since fixed the bug properly for PR105940, so this
    patch reverts my earlier attempt.  Sorry for the stupidity.

    gcc/
            PR tree-optimization/105254
            PR tree-optimization/105940

            Revert:

            * config/aarch64/aarch64.cc
            (aarch64_vector_costs::determine_suggested_unroll_factor): Take a
            loop_vec_info as argument.  Restrict the unroll factor to values
            that divide the VF.
            (aarch64_vector_costs::finish_cost): Update call accordingly.

    gcc/testsuite/
            * gcc.target/aarch64/sve/cost_model_14.c: New test.

[Bug tree-optimization/105940] suggested_unroll_factor applying place looks wrong

[linkw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105940

--- Comment #6 from Kewen Lin <linkw at gcc dot gnu.org> ---
(In reply to Kewen Lin from comment #4)
> (In reply to Richard Biener from comment #2)
> > (In reply to Kewen Lin from comment #1)
> > > Created attachment 53126 [details]
> > > move_applying
> > 
> > LGTM (maybe the suggested unroll factor should be only applied if the
> > suggestion was from a matching with/without SLP analysis, or in fact
> > vect_analyze_loop_1 should communicate that down - disabling SLP when
> > the one suggesting unrolling did the re-analysis).
> 
> Oops, just noticed the nice suggestion.  Will make a follow up patch for
> this.
> It would looks like:
>   when working out suggested unroll factor, save slp decision into one
> passed down variable from vect_analyze_loop_1.
>   when applying suggested unroll factor, if the save slp is false, directly
> ignore slp handlings, otherwise, go the normal slp path but won't start over
> for slp off.

I tested one patch which was bootstrapped and regress-tested on x86, aarch64
and powerpc64le, but found some failures on SPEC2017 run with rs6000 hackings,
the reason to fail is that we can save reduction chain in
vect_is_simple_reduction for further analysis in SLP detection, if we
aggressively skip SLP related analyses in vect_analyze_loop_2, there is ICE in
vectorizable_reduction

    if (REDUC_GROUP_FIRST_ELEMENT (stmt_info))
      gcc_assert (slp_node
                  && REDUC_GROUP_FIRST_ELEMENT (stmt_info) == stmt_info);

There seem to be some alternatives:
  1) check if applying_suggested_uf && !slp_done_for_suggested_uf initially in
vect_analyze_loop_2, if yes, pass slp disabled information down to
vect_is_simple_reduction, not to save reduction chain for later slp analysis
(not existed).
  2) before we are going to do the slp related analyses (vect_analyze_slp
etc.), check if applying_suggested_uf && !slp_done_for_suggested_uf, if yes,
dissolve LOOP_VINFO_REDUCTION_CHAINS(loop_info).
  3) for the case applying_suggested_uf && !slp_done_for_suggested_uf, we still
let it go through slp related analyses but not applying suggested unroll
factor, only applying for its retry without slp.

3) is simple but seems to be the worst since we still do some useless analyses.
1) can save the reduction chain handlings, seems to be the best, not sure if
it's too intrusive, and if there are some similar needs (in future) to do so.
2) is similar to 1), it's to add one common place to undo those previous
analysis results which are only for slp analyses and can cause unexpected
result if we don't undo it.

Any suggestions on this?

[Bug tree-optimization/105940] suggested_unroll_factor applying place looks wrong

[linkw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105940

Kewen Lin <linkw at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #53126|0                           |1
        is obsolete|                            |

--- Comment #7 from Kewen Lin <linkw at gcc dot gnu.org> ---
Created attachment 53152
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=53152&action=edit
Tested patch but fail with SPEC build

[Bug tree-optimization/105940] suggested_unroll_factor applying place looks wrong

[linkw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105940

--- Comment #8 from Kewen Lin <linkw at gcc dot gnu.org> ---
(In reply to Kewen Lin from comment #6)
> (In reply to Kewen Lin from comment #4)
> > (In reply to Richard Biener from comment #2)
> > > (In reply to Kewen Lin from comment #1)
> > > > Created attachment 53126 [details]
> > > > move_applying
> > > 
> > > LGTM (maybe the suggested unroll factor should be only applied if the
> > > suggestion was from a matching with/without SLP analysis, or in fact
> > > vect_analyze_loop_1 should communicate that down - disabling SLP when
> > > the one suggesting unrolling did the re-analysis).
> > 
> > Oops, just noticed the nice suggestion.  Will make a follow up patch for
> > this.
> > It would looks like:
> >   when working out suggested unroll factor, save slp decision into one
> > passed down variable from vect_analyze_loop_1.
> >   when applying suggested unroll factor, if the save slp is false, directly
> > ignore slp handlings, otherwise, go the normal slp path but won't start over
> > for slp off.
> 
> I tested one patch which was bootstrapped and regress-tested on x86, aarch64
> and powerpc64le, but found some failures on SPEC2017 run with rs6000
> hackings, the reason to fail is that we can save reduction chain in
> vect_is_simple_reduction for further analysis in SLP detection, if we
> aggressively skip SLP related analyses in vect_analyze_loop_2, there is ICE
> in vectorizable_reduction
> 
>     if (REDUC_GROUP_FIRST_ELEMENT (stmt_info))
>       gcc_assert (slp_node
>                   && REDUC_GROUP_FIRST_ELEMENT (stmt_info) == stmt_info);
> 
> There seem to be some alternatives:
>   1) check if applying_suggested_uf && !slp_done_for_suggested_uf initially
> in vect_analyze_loop_2, if yes, pass slp disabled information down to
> vect_is_simple_reduction, not to save reduction chain for later slp analysis
> (not existed).
>   2) before we are going to do the slp related analyses (vect_analyze_slp
> etc.), check if applying_suggested_uf && !slp_done_for_suggested_uf, if yes,
> dissolve LOOP_VINFO_REDUCTION_CHAINS(loop_info).
>   3) for the case applying_suggested_uf && !slp_done_for_suggested_uf, we
> still let it go through slp related analyses but not applying suggested
> unroll factor, only applying for its retry without slp.
> 
> 3) is simple but seems to be the worst since we still do some useless
> analyses.
> 1) can save the reduction chain handlings, seems to be the best, not sure if
> it's too intrusive, and if there are some similar needs (in future) to do so.
> 2) is similar to 1), it's to add one common place to undo those previous
> analysis results which are only for slp analyses and can cause unexpected
> result if we don't undo it.
> 
> Any suggestions on this?

Sent out one tested patch for 1) at
https://gcc.gnu.org/pipermail/gcc-patches/2022-June/596788.html

[Bug tree-optimization/105940] suggested_unroll_factor applying place looks wrong

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105940

--- Comment #9 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Kewen Lin <linkw@gcc.gnu.org>:

https://gcc.gnu.org/g:86882d9feb6a534325d7162216696266898e36d0

commit r13-1173-g86882d9feb6a534325d7162216696266898e36d0
Author: Kewen Lin <linkw@linux.ibm.com>
Date:   Mon Jun 20 07:44:21 2022 -0500

    vect: Respect slp decision when applying suggested uf [PR105940]

    This follows Richi's suggestion in PR105940, it aims to avoid
    inconsistent slp decision between when the suggested unroll
    factor is worked out and when the suggested unroll factor is
    applied.

    If the previous slp decision is true when the suggested unroll
    factor is worked out, when we are applying unroll factor we
    don't need to start over with slp off if the analysis with slp
    on fails.  On the other hand, if the previous slp decision is
    false when the suggested unroll factor is worked out, when we
    are applying unroll factor we can skip the slp handlings.

    Function vect_is_simple_reduction saves reduction chains for
    subsequent slp analyses, we have to disable this early otherwise
    there is an ICE in vectorizable_reduction for below:

      if (REDUC_GROUP_FIRST_ELEMENT (stmt_info))
        gcc_assert (slp_node
                    && REDUC_GROUP_FIRST_ELEMENT (stmt_info)
                       == stmt_info);

            PR tree-optimization/105940

    gcc/ChangeLog:

            * tree-vect-loop.cc (vect_analyze_loop_2): Add new parameter
            slp_done_for_suggested_uf and adjust with it accordingly.
            (vect_analyze_loop_1): Add new variable slp_done_for_suggested_uf,
            pass it down to vect_analyze_loop_2 for the initial analysis and
            applying suggested unroll factor.
            (vect_is_simple_reduction): Add parameter slp and adjust with it.
            (vect_analyze_scalar_cycles_1): Add parameter slp and pass down.
            (vect_analyze_scalar_cycles): Likewise.

[Bug tree-optimization/105940] suggested_unroll_factor applying place looks wrong

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105940

--- Comment #10 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-12 branch has been updated by Kewen Lin <linkw@gcc.gnu.org>:

https://gcc.gnu.org/g:b3200ac82fd5aed39293a54e0e83258bb6caa600

commit r12-8504-gb3200ac82fd5aed39293a54e0e83258bb6caa600
Author: Kewen Lin <linkw@linux.ibm.com>
Date:   Tue Jun 14 00:57:01 2022 -0500

    vect: Move suggested_unroll_factor applying [PR105940]

    As PR105940 shown, when rs6000 port tries to assign
    m_suggested_unroll_factor by 4 or so, there will be ICE on:

      exact_div (LOOP_VINFO_VECT_FACTOR (loop_vinfo),
                 loop_vinfo->suggested_unroll_factor);

    In function vect_analyze_loop_2, the current place of
    suggested_unroll_factor applying can't guarantee it's
    applied for all cases.  As the case shows, vectorizer
    could retry with SLP forced off, the vf is reset by
    saved_vectorization_factor which isn't applied with
    suggested_unroll_factor before.  It means it can end
    up with one vf which neglects suggested_unroll_factor.
    I think it's off design, we should move the applying
    of suggested_unroll_factor after start_over.

            PR tree-optimization/105940

    gcc/ChangeLog:

            * tree-vect-loop.cc (vect_analyze_loop_2): Move the place of
            applying suggested_unroll_factor after start_over.

    (cherry picked from commit f907cf4c07cf51863dadbe90894e2ae3382bada5)

[Bug tree-optimization/105940] suggested_unroll_factor applying place looks wrong

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105940

--- Comment #11 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-12 branch has been updated by Kewen Lin <linkw@gcc.gnu.org>:

https://gcc.gnu.org/g:bb1a6d92989a50df13b1a21085c86625089c9e53

commit r12-8505-gbb1a6d92989a50df13b1a21085c86625089c9e53
Author: Richard Sandiford <richard.sandiford@arm.com>
Date:   Wed Jun 15 11:12:51 2022 +0100

    aarch64: Revert bogus fix for PR105254

    In f2ebf2d98efe0ac2314b58cf474f44cb8ebd5244 I'd forced the
    chosen unroll factor to be a factor of the VF, in order to
    work around an exact_div ICE in PR105254.  This was completely
    bogus -- clearly I didn't look in enough detail at why we ended
    up with an unrolled VF that wasn't a multiple of the UF.

    Kewen has since fixed the bug properly for PR105940, so this
    patch reverts my earlier attempt.  Sorry for the stupidity.

    gcc/
            PR tree-optimization/105254
            PR tree-optimization/105940

            Revert:

            * config/aarch64/aarch64.cc
            (aarch64_vector_costs::determine_suggested_unroll_factor): Take a
            loop_vec_info as argument.  Restrict the unroll factor to values
            that divide the VF.
            (aarch64_vector_costs::finish_cost): Update call accordingly.

    gcc/testsuite/
            * gcc.target/aarch64/sve/cost_model_14.c: New test.

    (cherry picked from commit 2636660b6f35423e0cfbf53bfad5c5fed6ae6471)

[Bug tree-optimization/105940] suggested_unroll_factor applying place looks wrong

[linkw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105940

Kewen Lin <linkw at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #12 from Kewen Lin <linkw at gcc dot gnu.org> ---
Both backports were bootstrapped on ppc64le and aarch64, the extra aarch64 case
was also verified.

Guess we don't want to backport r13-1173 as the fixed inconsistent slp decision
will not cause functionality correctness issue? Marking this as resolved.

[Bug tree-optimization/105940] suggested_unroll_factor applying place looks wrong

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105940

--- Comment #13 from rguenther at suse dot de <rguenther at suse dot de> ---
On Thu, 23 Jun 2022, linkw at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105940
> 
> Kewen Lin <linkw at gcc dot gnu.org> changed:
> 
>            What    |Removed                     |Added
> ----------------------------------------------------------------------------
>          Resolution|---                         |FIXED
>              Status|ASSIGNED                    |RESOLVED
> 
> --- Comment #12 from Kewen Lin <linkw at gcc dot gnu.org> ---
> Both backports were bootstrapped on ppc64le and aarch64, the extra aarch64 case
> was also verified.
> 
> Guess we don't want to backport r13-1173 as the fixed inconsistent slp decision
> will not cause functionality correctness issue? Marking this as resolved.

Yeah, that one doesn't look for backporting.