[Bug tree-optimization/69466] [6 Regression] ICE: Invalid PHI argument after vectorization (on -O2)

[rguenth at gcc dot gnu.org]

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

Richard Biener  changed:

   What|Removed |Added

 Status|ASSIGNED|RESOLVED
 Resolution|--- |FIXED

--- Comment #10 from Richard Biener  ---
Fixed.

[Bug tree-optimization/69466] [6 Regression] ICE: Invalid PHI argument after vectorization (on -O2)

[rguenth at gcc dot gnu.org]

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

--- Comment #11 from Richard Biener  ---
Author: rguenth
Date: Thu Jan 28 09:10:30 2016
New Revision: 232916

URL: https://gcc.gnu.org/viewcvs?rev=232916=gcc=rev
Log:
2016-01-28  Richard Biener  

PR tree-optimization/69466
* tree-vect-loop-manip.c (slpeel_duplicate_current_defs_from_edges):
Account for PHIs we couldn't duplicate.

* gfortran.dg/vect/pr69466.f90: New testcase.

Added:
trunk/gcc/testsuite/gfortran.dg/vect/pr69466.f90
Modified:
trunk/gcc/ChangeLog
trunk/gcc/testsuite/ChangeLog
trunk/gcc/tree-vect-loop-manip.c

[Bug tree-optimization/69466] [6 Regression] ICE: Invalid PHI argument after vectorization (on -O2)

[aoliva at gcc dot gnu.org]

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

--- Comment #5 from Alexandre Oliva  ---
Created attachment 37486
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=37486=edit
Patch I'm testing to fix the problem

The problem occurs because we call set_current_def for phi nodes after
duplicating some blocks, but it isn't always the case that the blocks passed to
slpeel_duplicate_current_defs_from_edges were copied from one another.  In the
given testcase, the exit blocks passed to this function don't seem to be
related at all, so taking information from corresponding phi nodes to call
set_current_def is most certainly incorrect.  Indeed, it's calling
set_current_def for a virtual operand from a "corresponding" non-virtual
operand that causes slpeel_duplicate_current_defs_from_edges's use of
get_current_def to set up an incorrect, non-virtual value for the
virtual-operand phi node.  With this patch, we refrain from setting current
defs when blocks don't have analogous phi nodes, and this is enough to address
the problem.

It would be nice, however, if someone more familiar with the loop vectorizer
would check whether our calling this function with mismatched blocks indicates
another latent problem, or whether we could check for the mismatch and bypass
the call more efficiently at the caller.  For this testcase, the mismatching
call is the single_exit (scalar_loop) one in
slpeel_tree_duplicate_loop_to_edge_cfg.

[Bug tree-optimization/69466] [6 Regression] ICE: Invalid PHI argument after vectorization (on -O2)

[rguenth at gcc dot gnu.org]

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

--- Comment #9 from Richard Biener  ---
I am testing that patch now.

[Bug tree-optimization/69466] [6 Regression] ICE: Invalid PHI argument after vectorization (on -O2)

[rguenth at gcc dot gnu.org]

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

--- Comment #8 from Richard Biener  ---
Ok, it doesn't fix it, it somehow just makes -fno-vect-cost-model necessary. 
We still split the edge, but later in vect_loop_versioning then, so the reason
for the bug is still the same - we're looking at forwarder edge PHIs.

OTOH the values there should be ok to use.  We're ICEing with the 2nd peeling
now.

So the reason for the mismatch is that when split-edge comes along splitting

:
...


:
# prephitmp_112 = PHI 
# e_lsm.38_108 = PHI <1(14)>
# e_lsm.37_100 = PHI 

then slpeel_update_phi_nodes_for_guard2 fails to "copy" the PHI with constant
argument (which wasn't needed in the first place):

  for (gsi = gsi_start_phis (update_bb); !gsi_end_p (gsi); gsi_next ())
{
  tree new_res;
  update_phi = gsi.phi ();
  orig_phi = update_phi;
  orig_def = PHI_ARG_DEF_FROM_EDGE (orig_phi, e);
  /* This loop-closed-phi actually doesn't represent a use
 out of the loop - the phi arg is a constant.  */
  if (TREE_CODE (orig_def) != SSA_NAME)
continue;

this later confuses the current-def-setting code.  This means the circumstances
are somewhat "controlled" and thus the following fix "works".  Of course it
makes the whole thing even more awkward ;)

Index: tree-vect-loop-manip.c
===
--- tree-vect-loop-manip.c  (revision 232867)
+++ tree-vect-loop-manip.c  (working copy)
@@ -727,17 +727,26 @@ slpeel_duplicate_current_defs_from_edges

   for (gsi_from = gsi_start_phis (from->dest),
gsi_to = gsi_start_phis (to->dest);
-   !gsi_end_p (gsi_from) && !gsi_end_p (gsi_to);
-   gsi_next (_from), gsi_next (_to))
+   !gsi_end_p (gsi_from) && !gsi_end_p (gsi_to);)
 {
   gimple *from_phi = gsi_stmt (gsi_from);
   gimple *to_phi = gsi_stmt (gsi_to);
   tree from_arg = PHI_ARG_DEF_FROM_EDGE (from_phi, from);
+  if (TREE_CODE (from_arg) != SSA_NAME)
+   {
+ gsi_next (_from);
+ continue;
+   }
   tree to_arg = PHI_ARG_DEF_FROM_EDGE (to_phi, to);
-  if (TREE_CODE (from_arg) == SSA_NAME
- && TREE_CODE (to_arg) == SSA_NAME
- && get_current_def (to_arg) == NULL_TREE)
+  if (TREE_CODE (to_arg) != SSA_NAME)
+   {
+ gsi_next (_to);
+ continue;
+   }
+  if (get_current_def (to_arg) == NULL_TREE)
set_current_def (to_arg, get_current_def (from_arg));
+  gsi_next (_from);
+  gsi_next (_to);
 }
 }

[Bug tree-optimization/69466] [6 Regression] ICE: Invalid PHI argument after vectorization (on -O2)

[rguenth at gcc dot gnu.org]

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

--- Comment #7 from Richard Biener  ---
I think the fix is wrong, slpeel_duplicate_current_defs_from_edges assumes
from->dest and to->dest are the same (or the same kind of) block.  Your
patch merely papers over the issue that one of the exits leads to an
unexpected forwarder block.

Note that the whole slpeel infrastructure is quite fragile and should be
ditched - certainly it shouldn't "abuse" set/get_current_def.  But well...

It seems this bug has been introduced with adding versioning to if-conversion
and using the non-if-converted body for scalar iterations.  And the "bug"
is likely introduced by running a CFG cleanup requring an exit edge split
in vect_analyze_loop_form_1.

Removing that edge splitting "fixes" this testcase.  I'm going to see why
we needed that there.

[Bug tree-optimization/69466] [6 Regression] ICE: Invalid PHI argument after vectorization (on -O2)

[rguenth at gcc dot gnu.org]

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

--- Comment #6 from Richard Biener  ---
I'll have a look as well.

[Bug tree-optimization/69466] [6 Regression] ICE: Invalid PHI argument after vectorization (on -O2)

[aoliva at gcc dot gnu.org]

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

Alexandre Oliva  changed:

   What|Removed |Added

 Status|NEW |ASSIGNED
 CC||aoliva at gcc dot gnu.org
   Assignee|unassigned at gcc dot gnu.org  |aoliva at gcc dot 
gnu.org

--- Comment #4 from Alexandre Oliva  ---
On it

[Bug tree-optimization/69466] [6 Regression] ICE: Invalid PHI argument after vectorization (on -O2)

[rguenth at gcc dot gnu.org]

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

Richard Biener  changed:

   What|Removed |Added

   Priority|P3  |P1

[Bug tree-optimization/69466] [6 Regression] ICE: Invalid PHI argument after vectorization (on -O2)

[jakub at gcc dot gnu.org]

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

Jakub Jelinek  changed:

   What|Removed |Added

   Target Milestone|--- |6.0
Summary|ICE: Invalid PHI argument   |[6 Regression] ICE: Invalid
   |after vectorization (on |PHI argument after
   |-O2)|vectorization (on -O2)