[Bug tree-optimization/94261] New: [10 Regression] ICE in vect_get_vec_def_for_operand_1 for 3-element condition reduction

[Bug tree-optimization/94261] New: [10 Regression] ICE in vect_get_vec_def_for_operand_1 for 3-element condition reduction

[rsandifo at gcc dot gnu.org]

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

            Bug ID: 94261
           Summary: [10 Regression] ICE in vect_get_vec_def_for_operand_1
                    for 3-element condition reduction
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Keywords: ice-on-valid-code
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: rsandifo at gcc dot gnu.org
                CC: rguenth at gcc dot gnu.org
  Target Milestone: ---
            Target: aarch64*-*-*

The following test ICEs when compiled with
-O3 -ffinite-math-only -march=armv8.2-a+sve
on aarch64:

void
f (float *x, float *y, float z)
{
  float res0 = 0, res1 = 1, res2 = 2;
  for (int i = 0; i < 100; ++i)
    {
      res0 = 100 <= y[i * 3] ? res0 : z;
      res1 = 101 <= y[i * 3 + 1] ? res1 : z;
      res2 = y[i * 3 + 2] < 102 ? z : res2;
    }
  x[0] = res0;
  x[1] = res1;
  x[2] = res2;
}

The ICE is:

b.c:2:1: internal compiler error: in vect_get_vec_def_for_operand_1, at
tree-vect-stmts.c:1555
    2 | f (float *x, float *y, float z)
      | ^
0x10a86c3 vect_get_vec_def_for_operand_1(_stmt_vec_info*, vect_def_type)
        gcc/tree-vect-stmts.c:1555
0x10af8e1 vect_get_vec_def_for_operand(tree_node*, _stmt_vec_info*, tree_node*)
        gcc/tree-vect-stmts.c:1617
0x10b258f vectorizable_condition
        gcc/tree-vect-stmts.c:10213
0x10d01ea vect_transform_stmt(_stmt_vec_info*, gimple_stmt_iterator*,
_slp_tree*, _slp_instance*)
        gcc/tree-vect-stmts.c:11012
0x10d4226 vect_transform_loop_stmt
        gcc/tree-vect-loop.c:8307
0x10ec31a vect_transform_loop(_loop_vec_info*, gimple*)
        gcc/tree-vect-loop.c:8707
0x110f797 try_vectorize_loop_1
        gcc/tree-vectorizer.c:990
0x1110319 vectorize_loops()
        gcc/tree-vectorizer.c:1126

The problem comes from trying both SVE and Advanced SIMD and
eventually picking SVE.  The SVE version can't treat the loop
as a single 3-element SLP reduction because we don't yet
support loading { 100, 101, 102 } repeating for variable-length
vectors.  We therefore fail the SLP build before doing anything
about the awkward comparison arrangement.  With this version,
each ?: is a separate reduction and each one has its own,
correct, STMT_VINFO_REDUC_IDX.

But then we try the Advanced SIMD version.  This doesn't have
a problem with loading the constants, so we get as far as
dealing with mismatched comparisons:

          /* Swap.  */
          if (*swap == 1)
            {
              swap_ssa_operands (stmt, &TREE_OPERAND (cond, 0),
                                 &TREE_OPERAND (cond, 1));
              TREE_SET_CODE (cond, swap_tree_comparison (code));
            }
          /* Invert.  */
          else
            {
              swap_ssa_operands (stmt, gimple_assign_rhs2_ptr (stmt),
                                 gimple_assign_rhs3_ptr (stmt));
              if (STMT_VINFO_REDUC_IDX (stmt_info) == 1)
                STMT_VINFO_REDUC_IDX (stmt_info) = 2;
              else if (STMT_VINFO_REDUC_IDX (stmt_info) == 2)
                STMT_VINFO_REDUC_IDX (stmt_info) = 1;
              bool honor_nans = HONOR_NANS (TREE_OPERAND (cond, 0));
              code = invert_tree_comparison (TREE_CODE (cond), honor_nans);
              gcc_assert (code != ERROR_MARK);
              TREE_SET_CODE (cond, code);
            }

But these changes to the gimple stmt persist even if the SLP build
fails later, or if the SLP build succeeds and we decide not to
vectorise that way.  That doesn't matter too much for the current
loop_vinfo, because the STMT_VINFO_REDUC_IDXs are still consistent
with the gimple stmt.  But it means that the STMT_VINFO_REDUC_IDXs
for the older SVE loop_vinfo are now no longer correct.

The ICE triggers when we go back to the SVE loop_vinfo as the
cheapest implementation and try to code-generate the reduction.

The testcase is reduced from 481.wrf compiled with LTO.

[Bug tree-optimization/94261] [10 Regression] ICE in vect_get_vec_def_for_operand_1 for 3-element condition reduction

[rguenther at suse dot de]

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

--- Comment #1 from rguenther at suse dot de <rguenther at suse dot de> ---
On March 22, 2020 12:46:45 PM GMT+01:00, "rsandifo at gcc dot gnu.org"
<gcc-bugzilla@gcc.gnu.org> wrote:
>https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94261
>
>            Bug ID: 94261
>         Summary: [10 Regression] ICE in vect_get_vec_def_for_operand_1
>                    for 3-element condition reduction
>           Product: gcc
>           Version: unknown
>            Status: UNCONFIRMED
>          Keywords: ice-on-valid-code
>          Severity: normal
>          Priority: P3
>         Component: tree-optimization
>          Assignee: unassigned at gcc dot gnu.org
>          Reporter: rsandifo at gcc dot gnu.org
>                CC: rguenth at gcc dot gnu.org
>  Target Milestone: ---
>            Target: aarch64*-*-*
>
>The following test ICEs when compiled with
>-O3 -ffinite-math-only -march=armv8.2-a+sve
>on aarch64:
>
>void
>f (float *x, float *y, float z)
>{
>  float res0 = 0, res1 = 1, res2 = 2;
>  for (int i = 0; i < 100; ++i)
>    {
>      res0 = 100 <= y[i * 3] ? res0 : z;
>      res1 = 101 <= y[i * 3 + 1] ? res1 : z;
>      res2 = y[i * 3 + 2] < 102 ? z : res2;
>    }
>  x[0] = res0;
>  x[1] = res1;
>  x[2] = res2;
>}
>
>The ICE is:
>
>b.c:2:1: internal compiler error: in vect_get_vec_def_for_operand_1, at
>tree-vect-stmts.c:1555
>    2 | f (float *x, float *y, float z)
>      | ^
>0x10a86c3 vect_get_vec_def_for_operand_1(_stmt_vec_info*,
>vect_def_type)
>        gcc/tree-vect-stmts.c:1555
>0x10af8e1 vect_get_vec_def_for_operand(tree_node*, _stmt_vec_info*,
>tree_node*)
>        gcc/tree-vect-stmts.c:1617
>0x10b258f vectorizable_condition
>        gcc/tree-vect-stmts.c:10213
>0x10d01ea vect_transform_stmt(_stmt_vec_info*, gimple_stmt_iterator*,
>_slp_tree*, _slp_instance*)
>        gcc/tree-vect-stmts.c:11012
>0x10d4226 vect_transform_loop_stmt
>        gcc/tree-vect-loop.c:8307
>0x10ec31a vect_transform_loop(_loop_vec_info*, gimple*)
>        gcc/tree-vect-loop.c:8707
>0x110f797 try_vectorize_loop_1
>        gcc/tree-vectorizer.c:990
>0x1110319 vectorize_loops()
>        gcc/tree-vectorizer.c:1126
>
>The problem comes from trying both SVE and Advanced SIMD and
>eventually picking SVE.  The SVE version can't treat the loop
>as a single 3-element SLP reduction because we don't yet
>support loading { 100, 101, 102 } repeating for variable-length
>vectors.  We therefore fail the SLP build before doing anything
>about the awkward comparison arrangement.  With this version,
>each ?: is a separate reduction and each one has its own,
>correct, STMT_VINFO_REDUC_IDX.
>
>But then we try the Advanced SIMD version.  This doesn't have
>a problem with loading the constants, so we get as far as
>dealing with mismatched comparisons:
>
>          /* Swap.  */
>          if (*swap == 1)
>            {
>              swap_ssa_operands (stmt, &TREE_OPERAND (cond, 0),
>                                 &TREE_OPERAND (cond, 1));
>              TREE_SET_CODE (cond, swap_tree_comparison (code));
>            }
>          /* Invert.  */
>          else
>            {
>              swap_ssa_operands (stmt, gimple_assign_rhs2_ptr (stmt),
>                                 gimple_assign_rhs3_ptr (stmt));
>              if (STMT_VINFO_REDUC_IDX (stmt_info) == 1)
>                STMT_VINFO_REDUC_IDX (stmt_info) = 2;
>              else if (STMT_VINFO_REDUC_IDX (stmt_info) == 2)
>                STMT_VINFO_REDUC_IDX (stmt_info) = 1;
>              bool honor_nans = HONOR_NANS (TREE_OPERAND (cond, 0));
>          code = invert_tree_comparison (TREE_CODE (cond), honor_nans);
>              gcc_assert (code != ERROR_MARK);
>              TREE_SET_CODE (cond, code);
>            }
>
>But these changes to the gimple stmt persist even if the SLP build
>fails later, or if the SLP build succeeds and we decide not to
>vectorise that way.  That doesn't matter too much for the current
>loop_vinfo, because the STMT_VINFO_REDUC_IDXs are still consistent
>with the gimple stmt.  But it means that the STMT_VINFO_REDUC_IDXs
>for the older SVE loop_vinfo are now no longer correct.
>
>The ICE triggers when we go back to the SVE loop_vinfo as the
>cheapest implementation and try to code-generate the reduction.
>
>The testcase is reduced from 481.wrf compiled with LTO.

Stmt modifications considered harmful.. I've pondered with using alternate
Gimple stmts for the slp node here, similar to patterns but of course not
registered with the main stmt_info as that would get us back to the very same
issue. 

But only pondered, not actually tried implementing this.

[Bug tree-optimization/94261] [10 Regression] ICE in vect_get_vec_def_for_operand_1 for 3-element condition reduction

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |10.0

[Bug tree-optimization/94261] [10 Regression] ICE in vect_get_vec_def_for_operand_1 for 3-element condition reduction

[rguenth at gcc dot gnu.org]

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

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
So I think we can avoid swapping operands but what we cannot (current) avoid
is adjusting the tree code.  The operand swapping should be subsumed by
get_slp_defs no longer looking at the scalar stmt for mapping operand number
to defs but using the SLP children exclusively.  But of course we're still
looking at the actual comparison code - which we _could_ copy to the SLP
node as well - or is the comparison code adjustment not an issue here?
Quickly scanning I only see vectorizable_condition ever looking at the code
so there would be only a single place to adjust.

I'm not sure about test coverage, on x86_64 I see only

FAIL: gcc.dg/vect/vect-cselim-1.c (internal compiler error)
FAIL: gcc.dg/vect/vect-cselim-1.c (test for excess errors)
FAIL: gcc.dg/vect/vect-cselim-1.c scan-tree-dump-times vect "vectorized 1
loops" 1
FAIL: gcc.dg/vect/slp-cond-1.c (internal compiler error)
FAIL: gcc.dg/vect/slp-cond-1.c (test for excess errors)
FAIL: gcc.dg/vect/slp-cond-1.c scan-tree-dump-times vect "vectorizing stmts
using SLP" 3
FAIL: gcc.dg/vect/slp-cond-2-big-array.c (internal compiler error)
FAIL: gcc.dg/vect/slp-cond-2-big-array.c (test for excess errors)
FAIL: gcc.dg/vect/slp-cond-2-big-array.c scan-tree-dump-times vect "vectorizing
stmts using SLP" 3
FAIL: gcc.dg/vect/slp-cond-2.c (internal compiler error)
FAIL: gcc.dg/vect/slp-cond-2.c (test for excess errors)
FAIL: gcc.dg/vect/slp-cond-2.c scan-tree-dump-times vect "vectorizing stmts
using SLP" 3
FAIL: gcc.dg/vect/slp-cond-3.c (internal compiler error)
FAIL: gcc.dg/vect/slp-cond-3.c (test for excess errors)
FAIL: gcc.dg/vect/slp-cond-3.c scan-tree-dump-times vect "vectorizing stmts
using SLP" 1

when placing gcc_unreachable () at the swapping place and most testcases
still pass when removing the IL operand swapping, only vect-cselim-1.c
runfails (investigating).

I'd love to get rid of that last operand swapping case (because I'd like to
get rid of STMT_VINFO_NUM_SLP_USES...).  Yes, I'm working on adding a vectype
to slp_tree ...

[Bug tree-optimization/94261] [10 Regression] ICE in vect_get_vec_def_for_operand_1 for 3-element condition reduction

[rguenth at gcc dot gnu.org]

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

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #2)
> when placing gcc_unreachable () at the swapping place and most testcases
> still pass when removing the IL operand swapping, only vect-cselim-1.c
> runfails (investigating).

Ah, SLP ultimately fails here so the scalar IL is broken by the operation
code adjustment.  In the end we should be able to avoid doing tree code
adjustments as well since we're swapping to make the code the same
as the first stmts code and we only ever look at the first scalar stmt
during SLP code-gen/analysis. 

So, it appears to work to comment _all_ of the code in
vect_get_and_check_slp_defs (including the reduc-idx adjustment).

But again, test coverage is probably bad.

[Bug tree-optimization/94261] [10 Regression] ICE in vect_get_vec_def_for_operand_1 for 3-element condition reduction

[rguenth at gcc dot gnu.org]

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

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> ---
Created attachment 48083
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=48083&action=edit
proposed patch

untested sofar.

[Bug tree-optimization/94261] [10 Regression] ICE in vect_get_vec_def_for_operand_1 for 3-element condition reduction

[rsandifo at gcc dot gnu.org]

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

--- Comment #5 from rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #3)
> (In reply to Richard Biener from comment #2)
> > when placing gcc_unreachable () at the swapping place and most testcases
> > still pass when removing the IL operand swapping, only vect-cselim-1.c
> > runfails (investigating).
> 
> Ah, SLP ultimately fails here so the scalar IL is broken by the operation
> code adjustment.  In the end we should be able to avoid doing tree code
> adjustments as well since we're swapping to make the code the same
> as the first stmts code and we only ever look at the first scalar stmt
> during SLP code-gen/analysis. 

Yeah, started wondering about that later too...

> So, it appears to work to comment _all_ of the code in
> vect_get_and_check_slp_defs (including the reduc-idx adjustment).
> 
> But again, test coverage is probably bad.

Would be great if it works.  Will give it a spin later on SVE,
although I guess in some ways that probably exercises SLP even
less (at least in length-agnostic mode).

[Bug tree-optimization/94261] [10 Regression] ICE in vect_get_vec_def_for_operand_1 for 3-element condition reduction

[rguenth at gcc dot gnu.org]

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

--- Comment #6 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to rsandifo@gcc.gnu.org from comment #5)
> (In reply to Richard Biener from comment #3)
> > (In reply to Richard Biener from comment #2)
> > > when placing gcc_unreachable () at the swapping place and most testcases
> > > still pass when removing the IL operand swapping, only vect-cselim-1.c
> > > runfails (investigating).
> > 
> > Ah, SLP ultimately fails here so the scalar IL is broken by the operation
> > code adjustment.  In the end we should be able to avoid doing tree code
> > adjustments as well since we're swapping to make the code the same
> > as the first stmts code and we only ever look at the first scalar stmt
> > during SLP code-gen/analysis. 
> 
> Yeah, started wondering about that later too...

So there's probably a single path where this is not true which is when we
go through vect_attempt_slp_rearrange_stmts thus when we have a SLPed
set of cond-reductions.  I'm trying to come up with a testcase ...

But I think the cure here should be to punt on vect_slp_rearrange_stmts,
do you agree?

[Bug tree-optimization/94261] [10 Regression] ICE in vect_get_vec_def_for_operand_1 for 3-element condition reduction

[rsandifo at gcc dot gnu.org]

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

--- Comment #7 from rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #6)
> (In reply to rsandifo@gcc.gnu.org from comment #5)
> > (In reply to Richard Biener from comment #3)
> > > (In reply to Richard Biener from comment #2)
> > > > when placing gcc_unreachable () at the swapping place and most testcases
> > > > still pass when removing the IL operand swapping, only vect-cselim-1.c
> > > > runfails (investigating).
> > > 
> > > Ah, SLP ultimately fails here so the scalar IL is broken by the operation
> > > code adjustment.  In the end we should be able to avoid doing tree code
> > > adjustments as well since we're swapping to make the code the same
> > > as the first stmts code and we only ever look at the first scalar stmt
> > > during SLP code-gen/analysis. 
> > 
> > Yeah, started wondering about that later too...
> 
> So there's probably a single path where this is not true which is when we
> go through vect_attempt_slp_rearrange_stmts thus when we have a SLPed
> set of cond-reductions.  I'm trying to come up with a testcase ...
> 
> But I think the cure here should be to punt on vect_slp_rearrange_stmts,
> do you agree?

You know SLP much better than me :-)  But yeah, that sounds good.
Would we need to punt even for swap==1, or just swap==2?

[Bug tree-optimization/94261] [10 Regression] ICE in vect_get_vec_def_for_operand_1 for 3-element condition reduction

[rguenth at gcc dot gnu.org]

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

--- Comment #8 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to rsandifo@gcc.gnu.org from comment #7)
> (In reply to Richard Biener from comment #6)
> > (In reply to rsandifo@gcc.gnu.org from comment #5)
> > > (In reply to Richard Biener from comment #3)
> > > > (In reply to Richard Biener from comment #2)
> > > > > when placing gcc_unreachable () at the swapping place and most testcases
> > > > > still pass when removing the IL operand swapping, only vect-cselim-1.c
> > > > > runfails (investigating).
> > > > 
> > > > Ah, SLP ultimately fails here so the scalar IL is broken by the operation
> > > > code adjustment.  In the end we should be able to avoid doing tree code
> > > > adjustments as well since we're swapping to make the code the same
> > > > as the first stmts code and we only ever look at the first scalar stmt
> > > > during SLP code-gen/analysis. 
> > > 
> > > Yeah, started wondering about that later too...
> > 
> > So there's probably a single path where this is not true which is when we
> > go through vect_attempt_slp_rearrange_stmts thus when we have a SLPed
> > set of cond-reductions.  I'm trying to come up with a testcase ...
> > 
> > But I think the cure here should be to punt on vect_slp_rearrange_stmts,
> > do you agree?
> 
> You know SLP much better than me :-)  But yeah, that sounds good.
> Would we need to punt even for swap==1, or just swap==2?

Both - we'd pick up the wrong comparison code.  OTOH looking at the
vect_slp_rearrange_stmts code the only SLP nodes we actually have to
re-arrange are leafs now that constants and invariants have their own
SLP nodes.  Which means we wouldn't touch COND_EXPR (or other) operation
nodes at all.

Of course that code has even less test coverage ... but I'll try to rework it
this way.

[Bug tree-optimization/94261] [10 Regression] ICE in vect_get_vec_def_for_operand_1 for 3-element condition reduction

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |rguenth at gcc dot gnu.org
   Last reconfirmed|                            |2020-03-23
     Ever confirmed|0                           |1

--- Comment #9 from Richard Biener <rguenth at gcc dot gnu.org> ---
Created attachment 48087
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=48087&action=edit
patch in testing

Like this.  The previous patch passed bootstrap and regtest plus a SPEC 2006
test run on x86 with -Ofast -march=haswell.

Re-testing this patch (bootstrap is in stage2).

[Bug tree-optimization/94261] [10 Regression] ICE in vect_get_vec_def_for_operand_1 for 3-element condition reduction

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #48083|0                           |1
        is obsolete|                            |
  Attachment #48087|0                           |1
        is obsolete|                            |

--- Comment #10 from Richard Biener <rguenth at gcc dot gnu.org> ---
Created attachment 48089
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=48089&action=edit
patch

That didn't work out.  Rather than trying to fix up I'm making this minimal
fix to the sofar theoretical issue - avoid rearranging SLP nodes with
COND_EXPRs.  Further cleanup of that code left for GCC 11.

[Bug tree-optimization/94261] [10 Regression] ICE in vect_get_vec_def_for_operand_1 for 3-element condition reduction

[cvs-commit at gcc dot gnu.org]

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

--- Comment #11 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Richard Biener <rguenth@gcc.gnu.org>:

https://gcc.gnu.org/g:4dcc4502f316a7320fe72b62c60af12c77e1c96c

commit r10-7335-g4dcc4502f316a7320fe72b62c60af12c77e1c96c
Author: Richard Biener <rguenther@suse.de>
Date:   Mon Mar 23 13:08:41 2020 +0100

    tree-optimization/94261 - avoid IL adjustments in SLP analysis

    The remaining IL adjustment done by SLP analysis turns out harmful
    since we share them in the now multiple analyses states.  It turns
    out we do not actually need those apart from the case where we
    reorg scalar stmts during re-arrangement when optimizing load
    permutations in SLP reductions.  But that isn't needed either now
    since we only need to permute non-isomorphic parts which now
    reside in separate SLP nodes who are all leafs.

    2020-03-23  Richard Biener  <rguenther@suse.de>

            PR tree-optimization/94261
            * tree-vect-slp.c (vect_get_and_check_slp_defs): Remove
            IL operand swapping code.
            (vect_slp_rearrange_stmts): Do not arrange isomorphic
            nodes that would need operation code adjustments.

[Bug tree-optimization/94261] [10 Regression] ICE in vect_get_vec_def_for_operand_1 for 3-element condition reduction

[rguenth at gcc dot gnu.org]

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

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

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

--- Comment #12 from Richard Biener <rguenth at gcc dot gnu.org> ---
Hopefully fixed.