[PATCH] vect: Treat vector widening IFN calls as 'simple' [PR110436]

[PATCH] vect: Treat vector widening IFN calls as 'simple' [PR110436]

[Andre Vieira (lists)]

[-- Attachment #1: Type: text/plain, Size: 712 bytes --]

Hi,

This patch makes the vectorizer treat any vector widening IFN as simple, 
like
it did with the tree codes VEC_WIDEN_*.

I wasn't sure whether I should make all IFN's simple and then exclude 
some (like GOMP_ ones), or include more than just the new widening IFNs. 
But since this is the only behaviour that changed with the ifn patch, I 
decided to only special case the widening IFNs for now. Let me know if 
you have different thoughts on this.

Bootstrapped and regression tested on aarch64-unknow-linux-gnu.

gcc/ChangeLog:

	PR tree-optimization/110436
	* tree-vect-stmts.cc (is_simple_and_all_uses_invariant): Treat widening
	IFN's as simple.

gcc/testsuite/ChangeLog:

	* gcc.dg/pr110436.c: New test.

[-- Attachment #2: pr110436.patch --]
[-- Type: text/plain, Size: 1014 bytes --]

diff --git a/gcc/testsuite/gcc.dg/pr110436.c b/gcc/testsuite/gcc.dg/pr110436.c
new file mode 100644
index 0000000000000000000000000000000000000000..c146f99fac9f0524eaa3b1230b56e9f94eed5bda
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr110436.c
@@ -0,0 +1,5 @@
+/* { dg-do compile } */
+/* { dg-options "-O3" } */
+
+#include "pr83089.c"
+
diff --git a/gcc/tree-vect-stmts.cc b/gcc/tree-vect-stmts.cc
index d642d3c257f8d540a8562eedbcd40372b9550959..706055e9af94f0c1500c25faf4bd74fc08bf3cd6 100644
--- a/gcc/tree-vect-stmts.cc
+++ b/gcc/tree-vect-stmts.cc
@@ -296,8 +296,11 @@ is_simple_and_all_uses_invariant (stmt_vec_info stmt_info,
   tree op;
   ssa_op_iter iter;
 
-  gassign *stmt = dyn_cast <gassign *> (stmt_info->stmt);
-  if (!stmt)
+  gimple *stmt = stmt_info->stmt;
+  if (!is_gimple_assign (stmt)
+      && !(is_gimple_call (stmt)
+	   && gimple_call_internal_p (stmt)
+	   && widening_fn_p (gimple_call_combined_fn (stmt))))
     return false;
 
   FOR_EACH_SSA_TREE_OPERAND (op, stmt, iter, SSA_OP_USE)

Re: [PATCH] vect: Treat vector widening IFN calls as 'simple' [PR110436]

[Richard Biener]

On Mon, 3 Jul 2023, Andre Vieira (lists) wrote:

> Hi,
> 
> This patch makes the vectorizer treat any vector widening IFN as simple, like
> it did with the tree codes VEC_WIDEN_*.
> 
> I wasn't sure whether I should make all IFN's simple and then exclude some
> (like GOMP_ ones), or include more than just the new widening IFNs. But since
> this is the only behaviour that changed with the ifn patch, I decided to only
> special case the widening IFNs for now. Let me know if you have different
> thoughts on this.
> 
> Bootstrapped and regression tested on aarch64-unknow-linux-gnu.

But could we expand all VEC_WIDEN_* with scalar operands properly?  Can
we do that for the IFNs?  I think that's what we will end up doing then?

How do we end up with !STMT_VINFO_LIVE_P here anyway?

vectorizable_live_operation does

  /* If STMT is not relevant and it is a simple assignment and its inputs 
are
     invariant then it can remain in place, unvectorized.  The original 
last
     scalar value that it computes will be used.  */
  if (!STMT_VINFO_RELEVANT_P (stmt_info))
    {
      gcc_assert (is_simple_and_all_uses_invariant (stmt_info, 
loop_vinfo));
      if (dump_enabled_p ())
        dump_printf_loc (MSG_NOTE, vect_location,
                         "statement is simple and uses invariant.  Leaving 
in "
                         "place.\n");
      return true;


> gcc/ChangeLog:
> 
> 	PR tree-optimization/110436
> 	* tree-vect-stmts.cc (is_simple_and_all_uses_invariant): Treat
> 	widening
> 	IFN's as simple.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* gcc.dg/pr110436.c: New test.
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)

Re: [PATCH] vect: Treat vector widening IFN calls as 'simple' [PR110436]

[Richard Biener]

On Tue, 4 Jul 2023, Richard Biener wrote:

> On Mon, 3 Jul 2023, Andre Vieira (lists) wrote:
> 
> > Hi,
> > 
> > This patch makes the vectorizer treat any vector widening IFN as simple, like
> > it did with the tree codes VEC_WIDEN_*.
> > 
> > I wasn't sure whether I should make all IFN's simple and then exclude some
> > (like GOMP_ ones), or include more than just the new widening IFNs. But since
> > this is the only behaviour that changed with the ifn patch, I decided to only
> > special case the widening IFNs for now. Let me know if you have different
> > thoughts on this.
> > 
> > Bootstrapped and regression tested on aarch64-unknow-linux-gnu.
> 
> But could we expand all VEC_WIDEN_* with scalar operands properly?  Can
> we do that for the IFNs?  I think that's what we will end up doing then?
> 
> How do we end up with !STMT_VINFO_LIVE_P here anyway?
> 
> vectorizable_live_operation does
> 
>   /* If STMT is not relevant and it is a simple assignment and its inputs 
> are
>      invariant then it can remain in place, unvectorized.  The original 
> last
>      scalar value that it computes will be used.  */
>   if (!STMT_VINFO_RELEVANT_P (stmt_info))
>     {
>       gcc_assert (is_simple_and_all_uses_invariant (stmt_info, 
> loop_vinfo));
>       if (dump_enabled_p ())
>         dump_printf_loc (MSG_NOTE, vect_location,
>                          "statement is simple and uses invariant.  Leaving 
> in "
>                          "place.\n");
>       return true;

I _think_ the issue is that we do

pr83089.c:15:23: note:   mark relevant 0, live 1: zy_24 = hb_20 + 1;
pr83089.c:15:23: note:   last stmt in pattern. don't mark relevant/live.

and instead mark

pr83089.c:15:23: note:   worklist: examine stmt: patt_47 = .VEC_WIDEN_PLUS 
(el_36, 1);

so we used the is_simple_and_all_uses_invariant predicate on the
"wrong" stmt in the vect_stmt_relevant_p call.

I think we want something like below.  We then get

pr83089.c:15:23: note:   init: stmt relevant? zy_24 = hb_20 + 1;
pr83089.c:15:23: note:   vec_stmt_relevant_p: used out of loop.
pr83089.c:15:23: note:   vect_is_simple_use: operand (int) el_36, type of 
def: external
pr83089.c:15:23: note:   mark relevant 0, live 1: zy_24 = hb_20 + 1;
pr83089.c:15:23: note:   last stmt in pattern. don't mark relevant/live.
pr83089.c:15:23: note:   vec_stmt_relevant_p: forcing live patern stmt 
relevant.
pr83089.c:15:23: note:   mark relevant 1, live 1: patt_47 = 
.VEC_WIDEN_PLUS (el_36, 1);

that's not optimal from a code gen perspective but it's how the
vectorizer works (we can't cancel a pattern).

I'm going to test this.

Richard.

diff --git a/gcc/tree-vect-stmts.cc b/gcc/tree-vect-stmts.cc
index a0c39268bf0..c3e6f2d34ed 100644
--- a/gcc/tree-vect-stmts.cc
+++ b/gcc/tree-vect-stmts.cc
@@ -261,11 +261,26 @@ vect_mark_relevant (vec<stmt_vec_info> *worklist, stmt_vec_info stmt_info,
 	dump_printf_loc (MSG_NOTE, vect_location,
 			 "last stmt in pattern. don't mark"
 			 " relevant/live.\n");
+
       stmt_vec_info old_stmt_info = stmt_info;
       stmt_info = STMT_VINFO_RELATED_STMT (stmt_info);
       gcc_assert (STMT_VINFO_RELATED_STMT (stmt_info) == old_stmt_info);
       save_relevant = STMT_VINFO_RELEVANT (stmt_info);
       save_live_p = STMT_VINFO_LIVE_P (stmt_info);
+
+      if (live_p && relevant == vect_unused_in_scope)
+	{
+	  if (dump_enabled_p ())
+	    dump_printf_loc (MSG_NOTE, vect_location,
+			     "vec_stmt_relevant_p: forcing live patern stmt "
+			     "relevant.\n");
+	  relevant = vect_used_only_live;
+	}
+
+      if (dump_enabled_p ())
+	dump_printf_loc (MSG_NOTE, vect_location,
+			 "mark relevant %d, live %d: %G", relevant, live_p,
+			 stmt_info->stmt);
     }
 
   STMT_VINFO_LIVE_P (stmt_info) |= live_p;