[Bug tree-optimization/94043] [9/10 Regression] ICE in superloop_at_depth, at cfgloop.c:78

[Bug tree-optimization/94043] [9/10 Regression] ICE in superloop_at_depth, at cfgloop.c:78

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|9.3                         |9.4

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 9.3.0 has been released, adjusting target milestone.

[Bug tree-optimization/94043] [9/10 Regression] ICE in superloop_at_depth, at cfgloop.c:78

[marxin at gcc dot gnu.org]

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

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2020-03-17
             Status|UNCONFIRMED                 |NEW
     Ever confirmed|0                           |1
                 CC|                            |linkw at gcc dot gnu.org,
                   |                            |marxin at gcc dot gnu.org

--- Comment #2 from Martin Liška <marxin at gcc dot gnu.org> ---
Started with r10-4524-g103cba80bd84f0f3c7e644783e1fb7154d07f522, but it
probably only exposed a latent issue.

[Bug tree-optimization/94043] [9/10 Regression] ICE in superloop_at_depth, at cfgloop.c:78

[linkw at gcc dot gnu.org]

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

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

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

--- Comment #3 from Kewen Lin <linkw at gcc dot gnu.org> ---
Yes, very likely to just expose one latent bug, anyway I'll have a first look.

[Bug tree-optimization/94043] [9/10 Regression] ICE in superloop_at_depth, at cfgloop.c:78

[linkw at gcc dot gnu.org]

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

--- Comment #4 from Kewen Lin <linkw at gcc dot gnu.org> ---
This was just exposed from my commit, it can also be reproduced without my
commit but with -fno-vect-cost-model.

Some loops we have for this case:
;; Loop 1
;;  header 3, latch 10
;;  depth 1, outer 0
;;  nodes: 3 10 8 23 25 34 35 26 29 32 33 38 4 11 37 31

;; Loop 2
;;  header 4, latch 11
;;  depth 2, outer 1
;;  nodes: 4 11

;; Loop 4
;;  header 26, latch 29
;;  depth 2, outer 1
;;  nodes: 26 29


When we are doing versioning for loop4 required for aliasing check, the related
 scalar_loop_iters is based on e2.2_31, which is defined in BB 4, that is:

  <bb 4> [local count: 4343773762]:
  # e2.2_31 = PHI <_15(11), 1(37)>
  # ivtmp_14 = PHI <ivtmp_23(11), 4(37)>


For the codes:

        if ((def_bb = gimple_bb (SSA_NAME_DEF_STMT (USE_FROM_PTR (use_p))))
            && flow_bb_inside_loop_p (outermost, def_bb))
          outermost = superloop_at_depth (loop, bb_loop_depth (def_bb) + 1)

bb_loop_depth is 2, the +1 make the assertion in superloop_at_depth fail since
the current loop 4 only has the depth 2. I think the existing code has the
assumption that all operands in stmts of cond_expr_stmt_list are defined in
some outer loop of current, but the assumption breaks in this case.

I guess the current scalar_loop_iters is valid? Then the fix can be:

--- a/gcc/tree-vect-loop-manip.c
+++ b/gcc/tree-vect-loop-manip.c
@@ -3312,7 +3312,13 @@ vect_loop_versioning (loop_vec_info loop_vinfo,
       FOR_EACH_SSA_USE_OPERAND (use_p, stmt, iter, SSA_OP_USE)
        if ((def_bb = gimple_bb (SSA_NAME_DEF_STMT (USE_FROM_PTR (use_p))))
            && flow_bb_inside_loop_p (outermost, def_bb))
-         outermost = superloop_at_depth (loop, loop_depth (outermost) + 1);
+         {
+           /* Def block can be in either one outer loop of loop_to_version or
+              one sibling of outer loop of loop_to_version.  */
+           class loop *common_loop
+             = find_common_loop (def_bb->loop_father, loop);
+           outermost = superloop_at_depth (loop, loop_depth (common_loop) +
1);
+         }
     }

[Bug tree-optimization/94043] [9/10 Regression] ICE in superloop_at_depth, at cfgloop.c:78

[rguenther at suse dot de]

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

--- Comment #5 from rguenther at suse dot de <rguenther at suse dot de> ---
On Fri, 20 Mar 2020, linkw at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94043
> 
> --- Comment #4 from Kewen Lin <linkw at gcc dot gnu.org> ---
> This was just exposed from my commit, it can also be reproduced without my
> commit but with -fno-vect-cost-model.
> 
> Some loops we have for this case:
> ;; Loop 1
> ;;  header 3, latch 10
> ;;  depth 1, outer 0
> ;;  nodes: 3 10 8 23 25 34 35 26 29 32 33 38 4 11 37 31
> 
> ;; Loop 2
> ;;  header 4, latch 11
> ;;  depth 2, outer 1
> ;;  nodes: 4 11
> 
> ;; Loop 4
> ;;  header 26, latch 29
> ;;  depth 2, outer 1
> ;;  nodes: 26 29
> 
> 
> When we are doing versioning for loop4 required for aliasing check, the related
>  scalar_loop_iters is based on e2.2_31, which is defined in BB 4, that is:
> 
>   <bb 4> [local count: 4343773762]:
>   # e2.2_31 = PHI <_15(11), 1(37)>
>   # ivtmp_14 = PHI <ivtmp_23(11), 4(37)>
> 
> 
> For the codes:
> 
>         if ((def_bb = gimple_bb (SSA_NAME_DEF_STMT (USE_FROM_PTR (use_p))))
>             && flow_bb_inside_loop_p (outermost, def_bb))
>           outermost = superloop_at_depth (loop, bb_loop_depth (def_bb) + 1)
> 
> bb_loop_depth is 2, the +1 make the assertion in superloop_at_depth fail since
> the current loop 4 only has the depth 2. I think the existing code has the
> assumption that all operands in stmts of cond_expr_stmt_list are defined in
> some outer loop of current, but the assumption breaks in this case.
> 
> I guess the current scalar_loop_iters is valid? Then the fix can be:

What is not valid I think is that e2.2_31 should have a loop-closed PHI
node which would place it in an outer loop.  You'd have to see why
either the loop-closed PHI is not present or why the aliasing check
doesn't use that (it's more likely this)

> --- a/gcc/tree-vect-loop-manip.c
> +++ b/gcc/tree-vect-loop-manip.c
> @@ -3312,7 +3312,13 @@ vect_loop_versioning (loop_vec_info loop_vinfo,
>        FOR_EACH_SSA_USE_OPERAND (use_p, stmt, iter, SSA_OP_USE)
>         if ((def_bb = gimple_bb (SSA_NAME_DEF_STMT (USE_FROM_PTR (use_p))))
>             && flow_bb_inside_loop_p (outermost, def_bb))
> -         outermost = superloop_at_depth (loop, loop_depth (outermost) + 1);
> +         {
> +           /* Def block can be in either one outer loop of loop_to_version or
> +              one sibling of outer loop of loop_to_version.  */
> +           class loop *common_loop
> +             = find_common_loop (def_bb->loop_father, loop);
> +           outermost = superloop_at_depth (loop, loop_depth (common_loop) +
> 1);
> +         }
>      }
> 
>

[Bug tree-optimization/94043] [9/10 Regression] ICE in superloop_at_depth, at cfgloop.c:78

[linkw at gcc dot gnu.org]

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

--- Comment #6 from Kewen Lin <linkw at gcc dot gnu.org> ---
(In reply to rguenther@suse.de from comment #5)
> On Fri, 20 Mar 2020, linkw at gcc dot gnu.org wrote:
> 
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94043
> > 
> > --- Comment #4 from Kewen Lin <linkw at gcc dot gnu.org> ---
> > This was just exposed from my commit, it can also be reproduced without my
> > commit but with -fno-vect-cost-model.
> > 
> > Some loops we have for this case:
> > ;; Loop 1
> > ;;  header 3, latch 10
> > ;;  depth 1, outer 0
> > ;;  nodes: 3 10 8 23 25 34 35 26 29 32 33 38 4 11 37 31
> > 
> > ;; Loop 2
> > ;;  header 4, latch 11
> > ;;  depth 2, outer 1
> > ;;  nodes: 4 11
> > 
> > ;; Loop 4
> > ;;  header 26, latch 29
> > ;;  depth 2, outer 1
> > ;;  nodes: 26 29
> > 
> > 
> > When we are doing versioning for loop4 required for aliasing check, the related
> >  scalar_loop_iters is based on e2.2_31, which is defined in BB 4, that is:
> > 
> >   <bb 4> [local count: 4343773762]:
> >   # e2.2_31 = PHI <_15(11), 1(37)>
> >   # ivtmp_14 = PHI <ivtmp_23(11), 4(37)>
> > 
> > 
> > For the codes:
> > 
> >         if ((def_bb = gimple_bb (SSA_NAME_DEF_STMT (USE_FROM_PTR (use_p))))
> >             && flow_bb_inside_loop_p (outermost, def_bb))
> >           outermost = superloop_at_depth (loop, bb_loop_depth (def_bb) + 1)
> > 
> > bb_loop_depth is 2, the +1 make the assertion in superloop_at_depth fail since
> > the current loop 4 only has the depth 2. I think the existing code has the
> > assumption that all operands in stmts of cond_expr_stmt_list are defined in
> > some outer loop of current, but the assumption breaks in this case.
> > 
> > I guess the current scalar_loop_iters is valid? Then the fix can be:
> 
> What is not valid I think is that e2.2_31 should have a loop-closed PHI
> node which would place it in an outer loop.  You'd have to see why
> either the loop-closed PHI is not present or why the aliasing check
> doesn't use that (it's more likely this)
> 

Thanks for the confirmation Richi! There is a loop-closed PHI for it in bb 33:

  <bb 4> [local count: 35145078524]:
  # e2.2_31 = PHI <_15(11), 1(31)>
  # ivtmp_14 = PHI <ivtmp_23(11), 4(31)>
  _11 = (integer(kind=8)) e2.2_31;
  _12 = _10 + _11;
  _13 = _12 + -7;
  hx[_13] = 0;
  _15 = e2.2_31 + 1;
  ivtmp_23 = ivtmp_14 - 1;
  if (ivtmp_23 == 0)
    goto <bb 33>; [11.00%]
  else
    goto <bb 11>; [89.00%]

  <bb 33> [local count: 3865958617]:
  # _51 = PHI <_15(4)>

I'll further investigate why the scalar_loop_iters is constructed directly from
e2.2_31 instead of _51.

[Bug tree-optimization/94043] [9/10 Regression] ICE in superloop_at_depth, at cfgloop.c:78

[rguenther at suse dot de]

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

--- Comment #7 from rguenther at suse dot de <rguenther at suse dot de> ---
On Fri, 20 Mar 2020, linkw at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94043
> 
> --- Comment #6 from Kewen Lin <linkw at gcc dot gnu.org> ---
> (In reply to rguenther@suse.de from comment #5)
> > On Fri, 20 Mar 2020, linkw at gcc dot gnu.org wrote:
> > 
> > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94043
> > > 
> > > --- Comment #4 from Kewen Lin <linkw at gcc dot gnu.org> ---
> > > This was just exposed from my commit, it can also be reproduced without my
> > > commit but with -fno-vect-cost-model.
> > > 
> > > Some loops we have for this case:
> > > ;; Loop 1
> > > ;;  header 3, latch 10
> > > ;;  depth 1, outer 0
> > > ;;  nodes: 3 10 8 23 25 34 35 26 29 32 33 38 4 11 37 31
> > > 
> > > ;; Loop 2
> > > ;;  header 4, latch 11
> > > ;;  depth 2, outer 1
> > > ;;  nodes: 4 11
> > > 
> > > ;; Loop 4
> > > ;;  header 26, latch 29
> > > ;;  depth 2, outer 1
> > > ;;  nodes: 26 29
> > > 
> > > 
> > > When we are doing versioning for loop4 required for aliasing check, the related
> > >  scalar_loop_iters is based on e2.2_31, which is defined in BB 4, that is:
> > > 
> > >   <bb 4> [local count: 4343773762]:
> > >   # e2.2_31 = PHI <_15(11), 1(37)>
> > >   # ivtmp_14 = PHI <ivtmp_23(11), 4(37)>
> > > 
> > > 
> > > For the codes:
> > > 
> > >         if ((def_bb = gimple_bb (SSA_NAME_DEF_STMT (USE_FROM_PTR (use_p))))
> > >             && flow_bb_inside_loop_p (outermost, def_bb))
> > >           outermost = superloop_at_depth (loop, bb_loop_depth (def_bb) + 1)
> > > 
> > > bb_loop_depth is 2, the +1 make the assertion in superloop_at_depth fail since
> > > the current loop 4 only has the depth 2. I think the existing code has the
> > > assumption that all operands in stmts of cond_expr_stmt_list are defined in
> > > some outer loop of current, but the assumption breaks in this case.
> > > 
> > > I guess the current scalar_loop_iters is valid? Then the fix can be:
> > 
> > What is not valid I think is that e2.2_31 should have a loop-closed PHI
> > node which would place it in an outer loop.  You'd have to see why
> > either the loop-closed PHI is not present or why the aliasing check
> > doesn't use that (it's more likely this)
> > 
> 
> Thanks for the confirmation Richi! There is a loop-closed PHI for it in bb 33:
> 
>   <bb 4> [local count: 35145078524]:
>   # e2.2_31 = PHI <_15(11), 1(31)>
>   # ivtmp_14 = PHI <ivtmp_23(11), 4(31)>
>   _11 = (integer(kind=8)) e2.2_31;
>   _12 = _10 + _11;
>   _13 = _12 + -7;
>   hx[_13] = 0;
>   _15 = e2.2_31 + 1;
>   ivtmp_23 = ivtmp_14 - 1;
>   if (ivtmp_23 == 0)
>     goto <bb 33>; [11.00%]
>   else
>     goto <bb 11>; [89.00%]
> 
>   <bb 33> [local count: 3865958617]:
>   # _51 = PHI <_15(4)>
> 
> I'll further investigate why the scalar_loop_iters is constructed directly from
> e2.2_31 instead of _51.

It's most likely either SCEV or expand_simple_operations looking throuhg
the single-arg PHI (which we should avoid for LC PHI nodes)

[Bug tree-optimization/94043] [9/10 Regression] ICE in superloop_at_depth, at cfgloop.c:78

[linkw at gcc dot gnu.org]

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

--- Comment #8 from Kewen Lin <linkw at gcc dot gnu.org> ---
> It's most likely either SCEV or expand_simple_operations looking throuhg
> the single-arg PHI (which we should avoid for LC PHI nodes)

Thanks Richi, I found the loop-closed PHI form was broken after we finished the
vectorization on the loop 2, BB 38 was inserted, the function
gimple_find_edge_insert_loc will get one new BB if the dest has phis, even it's
unrelated.

;; basic block 4, loop depth 2
;;  pred:       11
;;              37
...
_15 = e2.2_31 + 1;
...
if (ivtmp_59 >= 1)
  goto <bb 38>; [100.00%]
else
  goto <bb 11>; [0.00%]
;;  succ:       38
;;              11

;; basic block 38, loop depth 1
;;  pred:       4
_40 = BIT_FIELD_REF <vect__15.22_43, 32, 96>;
;;  succ:       33

;; basic block 33, loop depth 1
;;  pred:       38
# _51 = PHI <_15(38)> 
;;  succ:       34

The alternatives seems could be 1) extend the current
gimple_find_edge_insert_loc to handle the phi nodes, if the phis aren't
related, just insert there, otherwise, insert some phis for uses of those stmts
and remove the related phis and create new assignments after those new stmts,
or 2) call rewrite_into_loop_closed_ssa for each successful vectorization.

[Bug tree-optimization/94043] [9/10 Regression] ICE in superloop_at_depth, at cfgloop.c:78

[rguenth at gcc dot gnu.org]

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

--- Comment #9 from Richard Biener <rguenth at gcc dot gnu.org> ---
OK, so it's indeed vectorizable_live_operation not paying attention to
loop-closed SSA form.

What it should do before building the lane extract is create a _new_
loop-closed PHI node for the vectorized def (vec_lhs), and then
demote the loop-closed PHI node for the scalar def (lhs) which should
_always_ exist to a copy.  So from


 loop;

# lhs' = PHI <lhs>


go to

  loop;

# vec_lhs' = PHI <vec_lhs>
new_tree = BIT_FIELD_REF <vec_lhs', ...>;
lhs' = new_tree;

I think you can assert that the block of the loop-closed PHI
(single_exit()->dest) always has a single predecessor, otherwise
things will be more complicated.

Can you try rework the code in this way?  If that's too much just tell
me and I'll take care of this.

[Bug tree-optimization/94043] [9/10 Regression] ICE in superloop_at_depth, at cfgloop.c:78

[linkw at gcc dot gnu.org]

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

--- Comment #10 from Kewen Lin <linkw at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #9)
> OK, so it's indeed vectorizable_live_operation not paying attention to
> loop-closed SSA form.
> 
> What it should do before building the lane extract is create a _new_
> loop-closed PHI node for the vectorized def (vec_lhs), and then
> demote the loop-closed PHI node for the scalar def (lhs) which should
> _always_ exist to a copy.  So from
> 
> 
>  loop;
> 
> # lhs' = PHI <lhs>
> 
> 
> go to
> 
>   loop;
> 
> # vec_lhs' = PHI <vec_lhs>
> new_tree = BIT_FIELD_REF <vec_lhs', ...>;
> lhs' = new_tree;
> 
> I think you can assert that the block of the loop-closed PHI
> (single_exit()->dest) always has a single predecessor, otherwise
> things will be more complicated.
> 
> Can you try rework the code in this way?  If that's too much just tell
> me and I'll take care of this.

Thanks Richi, I'll give it a shot!
I'd like to ensure my understanding: with the proposed fix, we ensure the
single_exit()->dest should be the correct BB to be inserted, no chance like
gimple_find_edge_insert_loc to get one new BB to be inserted, is it right?

[Bug tree-optimization/94043] [9/10 Regression] ICE in superloop_at_depth, at cfgloop.c:78

[rguenther at suse dot de]

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

--- Comment #11 from rguenther at suse dot de <rguenther at suse dot de> ---
On March 24, 2020 3:59:35 AM GMT+01:00, "linkw at gcc dot gnu.org"
<gcc-bugzilla@gcc.gnu.org> wrote:
>https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94043
>
>--- Comment #10 from Kewen Lin <linkw at gcc dot gnu.org> ---
>(In reply to Richard Biener from comment #9)
>> OK, so it's indeed vectorizable_live_operation not paying attention
>to
>> loop-closed SSA form.
>> 
>> What it should do before building the lane extract is create a _new_
>> loop-closed PHI node for the vectorized def (vec_lhs), and then
>> demote the loop-closed PHI node for the scalar def (lhs) which should
>> _always_ exist to a copy.  So from
>> 
>> 
>>  loop;
>> 
>> # lhs' = PHI <lhs>
>> 
>> 
>> go to
>> 
>>   loop;
>> 
>> # vec_lhs' = PHI <vec_lhs>
>> new_tree = BIT_FIELD_REF <vec_lhs', ...>;
>> lhs' = new_tree;
>> 
>> I think you can assert that the block of the loop-closed PHI
>> (single_exit()->dest) always has a single predecessor, otherwise
>> things will be more complicated.
>> 
>> Can you try rework the code in this way?  If that's too much just
>tell
>> me and I'll take care of this.
>
>Thanks Richi, I'll give it a shot!
>I'd like to ensure my understanding: with the proposed fix, we ensure
>the
>single_exit()->dest should be the correct BB to be inserted, no chance
>like
>gimple_find_edge_insert_loc to get one new BB to be inserted, is it
>right?

Yes.

[Bug tree-optimization/94043] [9/10 Regression] ICE in superloop_at_depth, at cfgloop.c:78

[linkw at gcc dot gnu.org]

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

--- Comment #12 from Kewen Lin <linkw at gcc dot gnu.org> ---
Created attachment 48122
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=48122&action=edit
ppc64le tested patch

Thanks Richi!

A patch draft attached to ensure on the right track, also
bootstrapped/regresstested. I tried to reproduce the case that the stmts for
lane extracting is empty (due to folding) with test cases associated in that
old commit but failed. I think we don't need to deal with it? The new copy
assignment instead of the phi could not be caught by the LC-PHI check in
expand_simple_operations.

[Bug tree-optimization/94043] [9/10 Regression] ICE in superloop_at_depth, at cfgloop.c:78

[rguenth at gcc dot gnu.org]

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

--- Comment #13 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Kewen Lin from comment #12)
> Created attachment 48122 [details]
> ppc64le tested patch
> 
> Thanks Richi!
> 
> A patch draft attached to ensure on the right track, also
> bootstrapped/regresstested. I tried to reproduce the case that the stmts for
> lane extracting is empty (due to folding) with test cases associated in that
> old commit but failed. I think we don't need to deal with it? The new copy
> assignment instead of the phi could not be caught by the LC-PHI check in
> expand_simple_operations.

+      /* Find all SSA NAMEs in stmts which is defined in current loop, create
+        PHIs for them, and replace them with phi results accordingly.  */
+      for (gsi = gsi_start (stmts); !gsi_end_p (gsi); gsi_next (&gsi))
+       {
+         gimple *stmt = gsi_stmt (gsi);
+         update_stmt (stmt);
+
...

should not be necessary.  What's missing in your patch is that when the
current code has computed vec_lhs it needs to create a LC PHI node for it
_before_ computing the lane extraction and instead use vec_lhs' there.

[Bug tree-optimization/94043] [9/10 Regression] ICE in superloop_at_depth, at cfgloop.c:78

[linkw at gcc dot gnu.org]

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

--- Comment #14 from Kewen Lin <linkw at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #13)

> 
> +      /* Find all SSA NAMEs in stmts which is defined in current loop,
> create
> +        PHIs for them, and replace them with phi results accordingly.  */
> +      for (gsi = gsi_start (stmts); !gsi_end_p (gsi); gsi_next (&gsi))
> +       {
> +         gimple *stmt = gsi_stmt (gsi);
> +         update_stmt (stmt);
> +
> ...
> 
> should not be necessary.  What's missing in your patch is that when the
> current code has computed vec_lhs it needs to create a LC PHI node for it
> _before_ computing the lane extraction and instead use vec_lhs' there.

OK, I was thinking the mask for LOOP_VINFO_FULLY_MASKED_P part is probably a
SSA name and can live out, as your comments, it looks impossible. Will update
it and send for review after testing. Thanks again!

[Bug tree-optimization/94043] [9/10 Regression] ICE in superloop_at_depth, at cfgloop.c:78

[rguenther at suse dot de]

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

--- Comment #15 from rguenther at suse dot de <rguenther at suse dot de> ---
On Thu, 26 Mar 2020, linkw at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94043
> 
> --- Comment #14 from Kewen Lin <linkw at gcc dot gnu.org> ---
> (In reply to Richard Biener from comment #13)
> 
> > 
> > +      /* Find all SSA NAMEs in stmts which is defined in current loop,
> > create
> > +        PHIs for them, and replace them with phi results accordingly.  */
> > +      for (gsi = gsi_start (stmts); !gsi_end_p (gsi); gsi_next (&gsi))
> > +       {
> > +         gimple *stmt = gsi_stmt (gsi);
> > +         update_stmt (stmt);
> > +
> > ...
> > 
> > should not be necessary.  What's missing in your patch is that when the
> > current code has computed vec_lhs it needs to create a LC PHI node for it
> > _before_ computing the lane extraction and instead use vec_lhs' there.
> 
> OK, I was thinking the mask for LOOP_VINFO_FULLY_MASKED_P part is probably a
> SSA name and can live out, as your comments, it looks impossible. Will update
> it and send for review after testing. Thanks again!

I guess the result of vect_get_loop_mask might need a loop-closed PHI
as well though that could be addressed independently since it's a
pre-existing "bug"

[Bug tree-optimization/94043] [9/10 Regression] ICE in superloop_at_depth, at cfgloop.c:78

[linkw at gcc dot gnu.org]

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

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

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

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

[Bug tree-optimization/94043] [9/10 Regression] ICE in superloop_at_depth, at cfgloop.c:78

[rguenth at gcc dot gnu.org]

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

--- Comment #17 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Kewen Lin from comment #16)
> Created attachment 48125 [details]
> untested patch

LGTM

[Bug tree-optimization/94043] [9/10 Regression] ICE in superloop_at_depth, at cfgloop.c:78

[cvs-commit at gcc dot gnu.org]

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

--- Comment #18 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:bd0f22a8d5caea8905f38ff1fafce31c1b7d33ad

commit r10-7491-gbd0f22a8d5caea8905f38ff1fafce31c1b7d33ad
Author: Kewen Lin <linkw@linux.ibm.com>
Date:   Tue Mar 31 22:48:46 2020 -0500

    Fix PR94043 by making vect_live_op generate lc-phi

    As PR94043 shows, my commit r10-4524 exposed one issue in
    vectorizable_live_operation, which inserts one extra BB
    before the single exit, leading unexpected operand expansion
    and unexpected loop depth assertion.  As Richi suggested,
    this patch is to teach vectorizable_live_operation to
    generate loop closed phi for vec_lhs, it looks like:
         loop;
         # lhs' = PHI <lhs>
    =>
         loop;
         # vec_lhs' = PHI <vec_lhs>
         new_tree = BIT_FIELD_REF <vec_lhs', ...>;
         lhs' = new_tree;

    I noticed that there are some SLP cases that have same lhs
    and vec_lhs but different offsets, which can make us have
    more PHIs for the same vec_lhs there.  But I think it would
    be fine since only one of them is actually live, the others
    should be eliminated by the following dce.  So the patch
    doesn't check whether there is one phi for vec_lhs, just
    create one directly instead.

    Bootstrapped/regtested on powerpc64le-linux-gnu (LE) P8.

    2020-04-01  Kewen Lin  <linkw@gcc.gnu.org>

    gcc/ChangeLog

        PR tree-optimization/94043
        * tree-vect-loop.c (vectorizable_live_operation): Generate loop-closed
        phi for vec_lhs and use it for lane extraction.

    gcc/testsuite/ChangeLog

        PR tree-optimization/94043
        * gfortran.dg/graphite/vect-pr94043.f90: New test.

[Bug tree-optimization/94043] [9/10 Regression] ICE in superloop_at_depth, at cfgloop.c:78

[linkw at gcc dot gnu.org]

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

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

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

--- Comment #19 from Kewen Lin <linkw at gcc dot gnu.org> ---
should be fixed on trunk now.

[Bug tree-optimization/94043] [9 Regression] ICE in superloop_at_depth, at cfgloop.c:78

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P2
      Known to work|                            |10.0
            Summary|[9/10 Regression] ICE in    |[9 Regression] ICE in
                   |superloop_at_depth, at      |superloop_at_depth, at
                   |cfgloop.c:78                |cfgloop.c:78
             Status|RESOLVED                    |ASSIGNED
         Resolution|FIXED                       |---

--- Comment #20 from Richard Biener <rguenth at gcc dot gnu.org> ---
Re-open.  It's marked as broken in GCC 9 so a backport is in oder (if the issue
really reproduces there).

[Bug tree-optimization/94043] [9 Regression] ICE in superloop_at_depth, at cfgloop.c:78

[linkw at gcc dot gnu.org]

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

--- Comment #21 from Kewen Lin <linkw at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #20)
> Re-open.  It's marked as broken in GCC 9 so a backport is in oder (if the
> issue really reproduces there).

Thanks for pointing it out.  I'll backport it two weeks later with no
regressions found in trunk.

[Bug tree-optimization/94043] [9 Regression] ICE in superloop_at_depth, at cfgloop.c:78

[clyon at gcc dot gnu.org]

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

Christophe Lyon <clyon at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |clyon at gcc dot gnu.org

--- Comment #22 from Christophe Lyon <clyon at gcc dot gnu.org> ---
This caused an ICE on aarch64, see PR94456

[Bug tree-optimization/94043] [9 Regression] ICE in superloop_at_depth, at cfgloop.c:78

[cvs-commit at gcc dot gnu.org]

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

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

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

commit r9-8506-ga809efd70d18c079d41051dc408de5e5f86a9797
Author: Kewen Lin <linkw@linux.ibm.com>
Date:   Tue Mar 31 22:48:46 2020 -0500

    Fix PR94043 by making vect_live_op generate lc-phi

    As PR94043 shows, my commit r10-4524 exposed one issue in
    vectorizable_live_operation, which inserts one extra BB
    before the single exit, leading unexpected operand expansion
    and unexpected loop depth assertion.  As Richi suggested,
    this patch is to teach vectorizable_live_operation to
    generate loop closed phi for vec_lhs, it looks like:
         loop;
         # lhs' = PHI <lhs>
    =>
         loop;
         # vec_lhs' = PHI <vec_lhs>
         new_tree = BIT_FIELD_REF <vec_lhs', ...>;
         lhs' = new_tree;

    I noticed that there are some SLP cases that have same lhs
    and vec_lhs but different offsets, which can make us have
    more PHIs for the same vec_lhs there.  But I think it would
    be fine since only one of them is actually live, the others
    should be eliminated by the following dce.  So the patch
    doesn't check whether there is one phi for vec_lhs, just
    create one directly instead.

    Bootstrapped/regtested on powerpc64le-linux-gnu (LE) P8.

    Backport from mainline.

      2020-04-01  Kewen Lin  <linkw@gcc.gnu.org>

      gcc/ChangeLog

          PR tree-optimization/94043
          * tree-vect-loop.c (vectorizable_live_operation): Generate
loop-closed
          phi for vec_lhs and use it for lane extraction.

      gcc/testsuite/ChangeLog

          PR tree-optimization/94043
          * gfortran.dg/graphite/vect-pr94043.f90: New test.

[Bug tree-optimization/94043] [9 Regression] ICE in superloop_at_depth, at cfgloop.c:78

[linkw at gcc dot gnu.org]

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

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

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

--- Comment #24 from Kewen Lin <linkw at gcc dot gnu.org> ---
Backported via r9-8506 and its related r9-8507.