Re: [PATCH v2 4/16]middle-end: Add dissolve code for when SLP fails and non-SLP loop vectorization is to be tried.

[Richard Biener <rguenther@suse.de>]

On Fri, 25 Sep 2020, Tamar Christina wrote:

> Hi All,
> 
> This adds the dissolve code to undo the patterns created by the pattern matcher
> in case SLP is to be aborted.
> 
> As mentioned in the cover letter this has one issue in that the number of copies
> can needed can change depending on whether TWO_OPERATORS is needed or not.
> 
> Because of this I don't analyze the original statement when it's replaced by a
> pattern and attempt to correct it here by analyzing it after dissolve.
> 
> This however seems too late and I would need to change the unroll factor, which
> seems a bit odd.  Any advice would be appreciated.
> 
> Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.

Ah, this is what you mean with the need to dissolve.  Yeah ...

@@ -2427,6 +2513,11 @@ again:
   /* Ensure that "ok" is false (with an opt_problem if dumping is 
enabled).  */
   gcc_assert (!ok);

+  /* Dissolve any SLP patterns created by the SLP pattern matcher.  */
+  opt_result dissolved = vect_dissolve_slp_only_patterns (loop_vinfo);
+  if (!dissolved)
+    return dissolved;
+
   /* Try again with SLP forced off but if we didn't do any SLP there is
      no point in re-trying.  */
   if (!slp)

I think this only belongs after

  if (dump_enabled_p ())
    dump_printf_loc (MSG_NOTE, vect_location,
                     "re-trying with SLP disabled\n");

  /* Roll back state appropriately.  No SLP this time.  */
  slp = false;

thus where everything else is "restored".  In fact I wonder if
it cannot be done as part of

  /* Reset SLP type to loop_vect on all stmts.  */
  for (i = 0; i < LOOP_VINFO_LOOP (loop_vinfo)->num_nodes; ++i)
    {
      basic_block bb = LOOP_VINFO_BBS (loop_vinfo)[i];
...

?  In particular

+       /* Now we have to re-analyze the statement since we skipped it in 
the
+          the initial analysis due to the differences in copies.  */
+       res = vect_analyze_stmt (loop_vinfo, related_stmt_info,
+                                &need_to_vectorize, NULL, NULL, 
&cost_vec);

looks unneeded since we're going to re-analyze all stmts anyway?

The thing is, there's no way to recover the original pattern stmt
in case your SLP pattern stmt was composed of "regular" pattern
stmts (the STMT_VINFO_RELATED_STMT simply gets replaced).  I
wonder if it would be easier to record the SLP pattern stmt
only in SLP_TREE_REPRESENTATIVE but _not_ in SLP_TREE_SCALAR_STMTS
(just leave those alone)?

Richard.


> Thanks,
> Tamar
> 
> gcc/ChangeLog:
> 
> 	* tree-vect-loop.c (vect_dissolve_slp_only_patterns): New
> 	(vect_dissolve_slp_only_groups): Call vect_dissolve_slp_only_patterns.
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imend