[Bug target/104015] New: [12 regression] gcc.dg/vect/slp-perm-9.c fails on power 9 (only)

[Bug target/104015] New: [12 regression] gcc.dg/vect/slp-perm-9.c fails on power 9 (only)

[seurer at gcc dot gnu.org]

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

            Bug ID: 104015
           Summary: [12 regression] gcc.dg/vect/slp-perm-9.c fails on
                    power 9 (only)
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: seurer at gcc dot gnu.org
  Target Milestone: ---

g:016bd7523131b645bca5b5530c81ab5149922743, r12-6523

make  -k check-gcc RUNTESTFLAGS="vect.exp=gcc.dg/vect/slp-perm-9.c"

FAIL: gcc.dg/vect/slp-perm-9.c scan-tree-dump-times vect "permutation requires
at least three vectors" 1
FAIL: gcc.dg/vect/slp-perm-9.c -flto -ffat-lto-objects  scan-tree-dump-times
vect "permutation requires at least three vectors" 1
# of expected passes            8
# of unexpected failures        2

This only fails on power 9.

This worked as of g:828474fafd2ed33430172fe227f9da7d6fb98723, r12-6419.  At
r12-6420 the build was broken for powerpc64 until r12-6523 when I noticed this.

[Bug target/104015] [12 regression] gcc.dg/vect/slp-perm-9.c fails on power 9 (only)

[linkw at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
     Ever confirmed|0                           |1
                 CC|                            |avieira at gcc dot gnu.org,
                   |                            |linkw at gcc dot gnu.org
   Last reconfirmed|                            |2022-01-14

--- Comment #1 from Kewen Lin <linkw at gcc dot gnu.org> ---
I think it's caused by r12-6240, since I only cherry-picked r12-6523 onto it,
the issue can be reproduced. It's likely duplicated of PR103997.

On Power9, we don't support partial vector so vectorizer should not try to use
the same vector mode for the epilogue?

gcc/testsuite/gcc.dg/vect/slp-perm-9.c:17:17: note:  ***** Analysis succeeded
with vector mode V8HI
gcc/testsuite/gcc.dg/vect/slp-perm-9.c:17:17: note:  ***** Choosing vector mode
V8HI
gcc/testsuite/gcc.dg/vect/slp-perm-9.c:17:17: note:  ***** Re-trying epilogue
analysis with vector mode V8HI

[Bug target/104015] [12 regression] gcc.dg/vect/slp-perm-9.c fails on power 9 (only)

[linkw at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
                 CC|                            |rguenth at gcc dot gnu.org,
                   |                            |rsandifo at gcc dot gnu.org
           Assignee|unassigned at gcc dot gnu.org      |linkw at gcc dot gnu.org

--- Comment #2 from Kewen Lin <linkw at gcc dot gnu.org> ---
With further investigation, this isn't duplicated. Now we have the function
partial_vectors_supported_p to get boolean supports_partial_vectors.

on rs6000, it's:

bool
partial_vectors_supported_p (void)
{
  return HAVE_len_load_v16qi || HAVE_len_store_v16qi;
}

#define HAVE_len_load_v16qi (TARGET_P9_VECTOR && TARGET_64BIT)
#define HAVE_len_store_v16qi (TARGET_P9_VECTOR && TARGET_64BIT)

The above optabs are supported from Power9 already.

However, we only enable it from Power10 due to known performance issue on
Power9.

      /* The lxvl/stxvl instructions don't perform well before Power10.  */
      if (TARGET_POWER10)
        SET_OPTION_IF_UNSET (&global_options, &global_options_set,
                             param_vect_partial_vector_usage, 1);
      else
        SET_OPTION_IF_UNSET (&global_options, &global_options_set,
                             param_vect_partial_vector_usage, 0);

So checking optab supports look not robust.

I had a check with the below fix, it works:

diff --git a/gcc/tree-vect-loop.c b/gcc/tree-vect-loop.c
index ba67de490bb..49d53fb3383 100644
--- a/gcc/tree-vect-loop.c
+++ b/gcc/tree-vect-loop.c
@@ -3026,7 +3026,8 @@ vect_analyze_loop (class loop *loop, vec_info_shared
*shared)
   vector_modes[0] = autodetected_vector_mode;
   mode_i = 0;

-  bool supports_partial_vectors = partial_vectors_supported_p ();
+  bool supports_partial_vectors =
+    partial_vectors_supported_p () && param_vect_partial_vector_usage != 0;
   poly_uint64 first_vinfo_vf = LOOP_VINFO_VECT_FACTOR (first_loop_vinfo);

   while (1)


But, is there some reason not use the LOOP_VINFO_CAN_USE_PARTIAL_VECTORS_P of
first_loop_vinfo? It respects param_vect_partial_vector_usage and checks
partial vector supports (LOOP_VINFO_MASKS and LOOP_VINFO_LENS) during the
analysis phase, it looks good fit for this need of supports_partial_vectors.

[Bug target/104015] [12 regression] gcc.dg/vect/slp-perm-9.c fails on power 9 (only)

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |12.0

[Bug target/104015] [12 regression] gcc.dg/vect/slp-perm-9.c fails on power 9 (only)

[avieira at gcc dot gnu.org]

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

--- Comment #3 from avieira at gcc dot gnu.org ---
Hi Kewen,

Thanks for the analysis. The param_vect_partial_vector_usage suggestion seems
valid, but that shouldn't be the root cause. 

 I would expect an unpredicated V8HI epilogue to fail for a V8HI main loop
(unless the main loop was unrolled).

That is what the following code in vect_analyze_loop_2 is responsible for:
  /* If we're vectorizing an epilogue loop, the vectorized loop either needs
     to be able to handle fewer than VF scalars, or needs to have a lower VF
     than the main loop.  */
  if (LOOP_VINFO_EPILOGUE_P (loop_vinfo)
      && !LOOP_VINFO_CAN_USE_PARTIAL_VECTORS_P (loop_vinfo)
      && maybe_ge (LOOP_VINFO_VECT_FACTOR (loop_vinfo),
                   LOOP_VINFO_VECT_FACTOR (orig_loop_vinfo)))
    return opt_result::failure_at (vect_location,
                                   "Vectorization factor too high for"
                                   " epilogue loop.\n");

So PR103997 is looking at fixing the skipping, because we skip too much now.
You seem to be describing a case where it doesn't skip enough, but like I said
that should be dealt with the code above, so I have a feeling there may be some
confusion here.

I have a patch for the earlier bug at
https://gcc.gnu.org/pipermail/gcc-patches/2022-January/588330.html 
This is still under review whils we work out a better way of dealing with the
issue. Could you maybe check whether that fixes your failures? I'll start a
cross build for powerpc in the meantime to see if I can check out these tests. 

As for why I don't use LOOP_VINFO_CAN_USE_PARTIAL_VECTORS_P on the first loop
vinfo to skip epilogue modes, that's because it is possible to have a
non-predicated main loop with a predicated epilogue. The test I added for
aarch64 with that patch is a motivating case.

On another note, unfortunately LOOP_VINFO_EPIL_USING_PARTIAL_VECTORS_P only
'forces' the use of partial vectors it doesn't tell us whether it is possible
or not AFAIU, hence why I introduced that new function, that really only checks
whether the target is at all capable of partial vector generation, since if we
know it's not possible at all we can skip more modes and avoid unnecessary
analysis.

[Bug target/104015] [12 regression] gcc.dg/vect/slp-perm-9.c fails on power 9 (only)

[linkw at gcc dot gnu.org]

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

--- Comment #4 from Kewen Lin <linkw at gcc dot gnu.org> ---
Hi Andre,

Thanks for the detailed explanations all below!

(In reply to avieira from comment #3)
> Hi Kewen,
> 
> Thanks for the analysis. The param_vect_partial_vector_usage suggestion
> seems valid, but that shouldn't be the root cause.
> 
>  I would expect an unpredicated V8HI epilogue to fail for a V8HI main loop
> (unless the main loop was unrolled).
> 
> That is what the following code in vect_analyze_loop_2 is responsible for:
>   /* If we're vectorizing an epilogue loop, the vectorized loop either needs
>      to be able to handle fewer than VF scalars, or needs to have a lower VF
>      than the main loop.  */
>   if (LOOP_VINFO_EPILOGUE_P (loop_vinfo)
>       && !LOOP_VINFO_CAN_USE_PARTIAL_VECTORS_P (loop_vinfo)
>       && maybe_ge (LOOP_VINFO_VECT_FACTOR (loop_vinfo),
>                    LOOP_VINFO_VECT_FACTOR (orig_loop_vinfo)))
>     return opt_result::failure_at (vect_location,
>                                    "Vectorization factor too high for"
>                                    " epilogue loop.\n");
> 

For this failure rs6000 specific, this check does take effect, but it's too
late for this case. Since we already decide to re-try vector modes for epilogue
in vect_analyze_loop, the retrying generates one extra unexpected statement in
dumping file. Without the culprit commit, it doesn't have any re-tries.

According to the comments for the snippet with supports_partial_vectors, I
still feel the root cause of this failure is we don't set
supports_partial_vectors right on Power9. Since it's considered as true for
Power9 but actually no as the partial vector capability also respecting
param_vect_partial_vector_usage.

  if (!supports_partial_vectors
           && maybe_ge (GET_MODE_NUNITS (vector_modes[mode_i]),
first_vinfo_vf))

> So PR103997 is looking at fixing the skipping, because we skip too much now.
> You seem to be describing a case where it doesn't skip enough, but like I
> said that should be dealt with the code above, so I have a feeling there may
> be some confusion here.

It seems to me that the case on Power9 even doesn't have a chance to be decided
skipped or not. :)

> 
> I have a patch for the earlier bug at
> https://gcc.gnu.org/pipermail/gcc-patches/2022-January/588330.html 
> This is still under review whils we work out a better way of dealing with
> the issue. Could you maybe check whether that fixes your failures? I'll
> start a cross build for powerpc in the meantime to see if I can check out
> these tests. 
> 

Thanks for the pointer, I just tried the patch and sorry that it didn't help
this failure on rs6000. If my understanding above is correct, it's expected
since the patch being reviewed just touched the code guarded with
!supports_partial_vectors, while this failed case on Power9 doesn't even enter
that block.

> As for why I don't use LOOP_VINFO_CAN_USE_PARTIAL_VECTORS_P on the first
> loop vinfo to skip epilogue modes, that's because it is possible to have a
> non-predicated main loop with a predicated epilogue. The test I added for
> aarch64 with that patch is a motivating case.
> 

Thanks for the clarification! On rs6000 it's default with
param_vect_partial_vector_usage 1 that the main loop won't use partial vector
but the epilogue can use, I'd expect the LOOP_VINFO_CAN_USE_PARTIAL_VECTORS_P
of main loop is true if the epilogue can use partial vector. It seems it's
different on aarch64.

[Bug target/104015] [12 regression] gcc.dg/vect/slp-perm-9.c fails on power 9 (only)

[avieira at gcc dot gnu.org]

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

--- Comment #5 from avieira at gcc dot gnu.org ---
Thanks Kewen, that seems worrying, I'll have a look.

[Bug target/104015] [12 regression] gcc.dg/vect/slp-perm-9.c fails on power 9 (only)

[rsandifo at gcc dot gnu.org]

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

--- Comment #6 from rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> ---
I think the patch in comment 2 is the correct fix (OK to commit).

(In reply to Kewen Lin from comment #4)
> (In reply to avieira from comment #3)
> > Hi Kewen,
> > 
> > Thanks for the analysis. The param_vect_partial_vector_usage suggestion
> > seems valid, but that shouldn't be the root cause.
> > 
> >  I would expect an unpredicated V8HI epilogue to fail for a V8HI main loop
> > (unless the main loop was unrolled).
> > 
> > That is what the following code in vect_analyze_loop_2 is responsible for:
> >   /* If we're vectorizing an epilogue loop, the vectorized loop either needs
> >      to be able to handle fewer than VF scalars, or needs to have a lower VF
> >      than the main loop.  */
> >   if (LOOP_VINFO_EPILOGUE_P (loop_vinfo)
> >       && !LOOP_VINFO_CAN_USE_PARTIAL_VECTORS_P (loop_vinfo)
> >       && maybe_ge (LOOP_VINFO_VECT_FACTOR (loop_vinfo),
> >                    LOOP_VINFO_VECT_FACTOR (orig_loop_vinfo)))
> >     return opt_result::failure_at (vect_location,
> >                                    "Vectorization factor too high for"
> >                                    " epilogue loop.\n");
> > 
> 
> For this failure rs6000 specific, this check does take effect, but it's too
> late for this case. Since we already decide to re-try vector modes for
> epilogue in vect_analyze_loop, the retrying generates one extra unexpected
> statement in dumping file. Without the culprit commit, it doesn't have any
> re-tries.
Yeah, I think this just shows that it's a flaky test.  Does it really
matter that:

  permutation requires at least three vectors

is printed exactly once, rather than at least once?  It's inevitable
that we'll start printing more stuff when trying (and possibly failing)
to use other vectorisation strategies.

[Bug target/104015] [12 regression] gcc.dg/vect/slp-perm-9.c fails on power 9 (only)

[avieira at gcc dot gnu.org]

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

--- Comment #7 from avieira at gcc dot gnu.org ---
Yeah I'm with Richard on this one, I just checked and the generated assembly is
the same for before and after my patch, so this looks like a testism.


And yeah I agree, if we were to decide to unroll this for instance then you'd
likely see it being printed more too, since you would likely end up with the
epilogue using the same mode.

I'll suggest changing it to just testing the existance of that string, rather
than requring it N times.

Having said that, the fail will go away for this particular case with the param
change.

[Bug target/104015] [12 regression] gcc.dg/vect/slp-perm-9.c fails on power 9 (only)

[cvs-commit at gcc dot gnu.org]

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

--- Comment #8 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:6d51a9c6447bace21f860e70aed13c6cd90971bd

commit r12-6582-g6d51a9c6447bace21f860e70aed13c6cd90971bd
Author: Kewen Lin <linkw@linux.ibm.com>
Date:   Fri Jan 14 07:02:10 2022 -0600

    vect: Check partial vector param for supports_partial_vectors [PR104015]

    As described in PR104015, the function partial_vectors_supported_p
    mainly checks optabs for partial vectors support query, but we
    still have one parameter param_vect_partial_vector_usage to control
    the capability.

    Power9 introduces vector with length instructions (for
    len_load/len_store) but we don't enable partial vector on it by
    default. It should be considered as not supporting partial vector by
    default. This fix is to make the flag supports_partial_vectors check
    param_vect_partial_vector_usage accordingly.

    gcc/ChangeLog:

            PR tree-optimization/104015
            * tree-vect-loop.c (vect_analyze_loop): Check
            param_vect_partial_vector_usage for supports_partial_vectors.

[Bug target/104015] [12 regression] gcc.dg/vect/slp-perm-9.c fails on power 9 (only)

[linkw at gcc dot gnu.org]

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

--- Comment #9 from Kewen Lin <linkw at gcc dot gnu.org> ---
(In reply to rsandifo@gcc.gnu.org from comment #6)
> I think the patch in comment 2 is the correct fix (OK to commit).
> 

Thanks for the review and approval Richard!

I totally agree this test case can be fragile when facing different
vectorisation strategies, but I'm not sure if leaving the exact number
checkings still has a bit value since this case seems the only case to catch
the redundant re-try (at least on Power?), once we fix it not to check any
times, we might miss some sensitive coverage on some useless re-tries, though
the compiling time influence is very very tiny (seems not a big deal :)).

[Bug target/104015] [12 regression] gcc.dg/vect/slp-perm-9.c fails on power 9 (only)

[rsandifo at gcc dot gnu.org]

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

--- Comment #10 from rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> ---
(In reply to Kewen Lin from comment #9)
> I totally agree this test case can be fragile when facing different
> vectorisation strategies, but I'm not sure if leaving the exact number
> checkings still has a bit value since this case seems the only case to catch
> the redundant re-try (at least on Power?), once we fix it not to check any
> times, we might miss some sensitive coverage on some useless re-tries,
> though the compiling time influence is very very tiny (seems not a big deal
> :)).
Checking the number of tries might be useful, but if so, I think
it should be done by a test that was written for that specific
purpose.  The tst can then be updated if the original way of checking
the number of tries no longer worked for some reason.  The test
would likely be target-specific (since, for example, the expectations
are different for Power 9 and Power 10).

But this test was added to test how the SLP permute code handled
groups of 3 elements.  IMO that's the only thing that it should test.
Even the existing target selectors are so complicated as to be almost
incomprehensible. :-)

[Bug target/104015] [12 regression] gcc.dg/vect/slp-perm-9.c fails on power 9 (only)

[linkw at gcc dot gnu.org]

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

--- Comment #11 from Kewen Lin <linkw at gcc dot gnu.org> ---
(In reply to rsandifo@gcc.gnu.org from comment #10)
> Checking the number of tries might be useful, but if so, I think
> it should be done by a test that was written for that specific
> purpose.  The tst can then be updated if the original way of checking
> the number of tries no longer worked for some reason.  The test
> would likely be target-specific (since, for example, the expectations
> are different for Power 9 and Power 10).
> 
> But this test was added to test how the SLP permute code handled
> groups of 3 elements.  IMO that's the only thing that it should test.
> Even the existing target selectors are so complicated as to be almost
> incomprehensible. :-)

Good point and idea! I just posted one patch
https://gcc.gnu.org/pipermail/gcc-patches/2022-January/588658.html following
this nice suggestion.

[Bug target/104015] [12 regression] gcc.dg/vect/slp-perm-9.c fails on power 9 (only)

[cvs-commit at gcc dot gnu.org]

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

--- Comment #12 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:fc6cd798c07a94d6b0bcc16b175e6e5d6e594c7e

commit r12-6717-gfc6cd798c07a94d6b0bcc16b175e6e5d6e594c7e
Author: Kewen Lin <linkw@linux.ibm.com>
Date:   Tue Jan 18 20:31:46 2022 -0600

    testsuite: Adjust possibly fragile slp-perm-9.c [PR104015]

    As Richard pointed out in PR104015, the test case slp-perm-9.c
    can be fragile when vectorizer tries to use different
    vectorisation strategies.

    As suggested, this patch tries to make the check not sensitive
    on the re-trying times by removing the times checking.  To still
    retain the test coverage on unnecessary re-trying, for example
    it exposes this PR104015 on Power9, I added two test cases to
    powerpc testsuite.

    gcc/testsuite/ChangeLog:

            PR tree-optimization/104015
            * gcc.dg/vect/slp-perm-9.c: Adjust.
            * gcc.target/powerpc/pr104015-1.c: New test.
            * gcc.target/powerpc/pr104015-2.c: New test.

[Bug target/104015] [12 regression] gcc.dg/vect/slp-perm-9.c fails on power 9 (only)

[linkw at gcc dot gnu.org]

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

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

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

--- Comment #13 from Kewen Lin <linkw at gcc dot gnu.org> ---
Should be fixed on latest trunk.