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

2020-11-03 Thread Tamar Christina via Gcc-patches
Hi All,

Here's a respin of this patch with the requested changes.

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.

> -Original Message-
> From: Gcc-patches  On Behalf Of Tamar
> Christina
> Sent: Friday, September 25, 2020 3:28 PM
> To: gcc-patches@gcc.gnu.org
> Cc: nd ; rguent...@suse.de; o...@ucw.cz
> Subject: [PATCH v2 4/16]middle-end: Add dissolve code for when SLP fails
> and non-SLP loop vectorization is to be tried.
> 
> 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.
> 
> 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.
> 
> --
diff --git a/gcc/tree-vect-loop.c b/gcc/tree-vect-loop.c
index 6fa185daa2836062814f9c9a6659011a3153c6a2..9601a83edcb05e994e27d4bb16a537190ad8471d 100644
--- a/gcc/tree-vect-loop.c
+++ b/gcc/tree-vect-loop.c
@@ -1979,6 +1979,63 @@ vect_get_datarefs_in_loop (loop_p loop, basic_block *bbs,
   return opt_result::success ();
 }
 
+/* For every SLP only pattern created by the pattern matched rooted in ROOT
+   restore the relevancy of the original statements over those of the pattern
+   and destroy the pattern relationship.  This restores the SLP tree to a state
+   where it can be used when SLP build is cancelled or re-tried.  */
+
+static void
+vect_dissolve_slp_only_patterns (loop_vec_info loop_vinfo,
+ hash_set *visited, slp_tree root)
+{
+  if (!root || visited->contains (root))
+return;
+
+  unsigned int i;
+  slp_tree node;
+  stmt_vec_info related_stmt_info;
+  stmt_vec_info stmt_info = SLP_TREE_REPRESENTATIVE (root);
+
+  visited->add (root);
+
+if (stmt_info && STMT_VINFO_SLP_VECT_ONLY (stmt_info)
+	 && (related_stmt_info = STMT_VINFO_RELATED_STMT (stmt_info)) != NULL)
+  {
+	if (dump_enabled_p ())
+	  dump_printf_loc (MSG_NOTE, vect_location,
+			   "dissolving relevancy of %G",
+			   STMT_VINFO_STMT (stmt_info));
+	STMT_VINFO_RELEVANT (stmt_info) = vect_unused_in_scope;
+	STMT_VINFO_RELEVANT (related_stmt_info) = vect_used_in_scope;
+	STMT_VINFO_IN_PATTERN_P (related_stmt_info) = false;
+	STMT_SLP_TYPE (related_stmt_info) = loop_vect;
+  }
+
+  FOR_EACH_VEC_ELT (SLP_TREE_CHILDREN (root), i, node)
+vect_dissolve_slp_only_patterns (loop_vinfo, visited, node);
+}
+
+/* Lookup any SLP Only Pattern statements created by the SLP pattern matcher in
+   all slp_instances in LOOP_VINFO and undo the relevancy of statements such
+   that the original SLP tree before the pattern matching is used.  */
+
+static void
+vect_dissolve_slp_only_patterns (loop_vec_info loop_vinfo)
+{
+
+  unsigned int i;
+  hash_set visited;
+
+  DUMP_VECT_SCOPE ("vect_dissolve_slp_only_patterns");
+
+  /* Unmark any SLP only patterns as relevant and restore the STMT_INFO of the
+ related instruction.  */
+  slp_instance instance;
+  FOR_EACH_VEC_ELT (LOOP_VINFO_SLP_INSTANCES (loop_vinfo), i, instance)
+vect_dissolve_slp_only_patterns (loop_vinfo, ,
+ SLP_INSTANCE_TREE (instance));
+}
+
 /* Look for SLP-only access groups and turn each individual access into its own
group.  */
 static void
@@ -2510,6 +2567,9 @@ 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.  */
+  vect_dissolve_slp_only_patterns (loop_vinfo);
+
   /* Try again with SLP forced off but if we didn't do any SLP there is
  no point in re-trying.  */
   if (!slp)



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

2020-09-28 Thread Richard Biener
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,
+_to_vectorize, NULL, NULL, 
_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 
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imend