Re: [PATCH 3/3] vect: inbranch SIMD clones

[Jakub Jelinek <jakub@redhat.com>]

On Tue, Aug 09, 2022 at 02:23:50PM +0100, Andrew Stubbs wrote:
> 
> There has been support for generating "inbranch" SIMD clones for a long time,
> but nothing actually uses them (as far as I can see).

Thanks for working on this.

Note, there is another case where the inbranch SIMD clones could be used
and I even thought it is implemented, but apparently it isn't or it doesn't
work:
#ifndef TYPE
#define TYPE int
#endif

/* A simple function that will be cloned.  */
#pragma omp declare simd inbranch
TYPE __attribute__((noinline))
foo (TYPE a)
{
  return a + 1;
}

/* Check that "inbranch" clones are called correctly.  */

void __attribute__((noinline))
masked (TYPE * __restrict a, TYPE * __restrict b, int size)
{
  #pragma omp simd
  for (int i = 0; i < size; i++)
    b[i] = foo(a[i]);
}

Here, IMHO we should use the inbranch clone for vectorization (better
than not vectorizing it, worse than when we'd have a notinbranch clone)
and just use mask of all ones.
But sure, it can be done incrementally, just mentioning it for completeness.

> This patch add supports for a sub-set of possible cases (those using
> mask_mode == VOIDmode).  The other cases fail to vectorize, just as before,
> so there should be no regressions.
> 
> The sub-set of support should cover all cases needed by amdgcn, at present.
> 
> gcc/ChangeLog:
> 
> 	* omp-simd-clone.cc (simd_clone_adjust_argument_types): Set vector_type
> 	for mask arguments also.
> 	* tree-if-conv.cc: Include cgraph.h.
> 	(if_convertible_stmt_p): Do if conversions for calls to SIMD calls.
> 	(predicate_statements): Pass the predicate to SIMD functions.
> 	* tree-vect-stmts.cc (vectorizable_simd_clone_call): Permit calls
> 	to clones with mask arguments, in some cases.
> 	Generate the mask vector arguments.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* gcc.dg/vect/vect-simd-clone-16.c: New test.
> 	* gcc.dg/vect/vect-simd-clone-16b.c: New test.
> 	* gcc.dg/vect/vect-simd-clone-16c.c: New test.
> 	* gcc.dg/vect/vect-simd-clone-16d.c: New test.
> 	* gcc.dg/vect/vect-simd-clone-16e.c: New test.
> 	* gcc.dg/vect/vect-simd-clone-16f.c: New test.
> 	* gcc.dg/vect/vect-simd-clone-17.c: New test.
> 	* gcc.dg/vect/vect-simd-clone-17b.c: New test.
> 	* gcc.dg/vect/vect-simd-clone-17c.c: New test.
> 	* gcc.dg/vect/vect-simd-clone-17d.c: New test.
> 	* gcc.dg/vect/vect-simd-clone-17e.c: New test.
> 	* gcc.dg/vect/vect-simd-clone-17f.c: New test.
> 	* gcc.dg/vect/vect-simd-clone-18.c: New test.
> 	* gcc.dg/vect/vect-simd-clone-18b.c: New test.
> 	* gcc.dg/vect/vect-simd-clone-18c.c: New test.
> 	* gcc.dg/vect/vect-simd-clone-18d.c: New test.
> 	* gcc.dg/vect/vect-simd-clone-18e.c: New test.
> 	* gcc.dg/vect/vect-simd-clone-18f.c: New test.

> --- a/gcc/tree-if-conv.cc
> +++ b/gcc/tree-if-conv.cc
> @@ -1074,13 +1076,19 @@ if_convertible_stmt_p (gimple *stmt, vec<data_reference_p> refs)
>  	tree fndecl = gimple_call_fndecl (stmt);
>  	if (fndecl)
>  	  {
> +	    /* We can vectorize some builtins and functions with SIMD
> +	       clones.  */
>  	    int flags = gimple_call_flags (stmt);
> +	    struct cgraph_node *node = cgraph_node::get (fndecl);
>  	    if ((flags & ECF_CONST)
>  		&& !(flags & ECF_LOOPING_CONST_OR_PURE)
> -		/* We can only vectorize some builtins at the moment,
> -		   so restrict if-conversion to those.  */
>  		&& fndecl_built_in_p (fndecl))
>  	      return true;
> +	    else if (node && node->simd_clones != NULL)
> +	      {
> +		need_to_predicate = true;

I think it would be worth it to check that at least one of the
node->simd_clones clones has ->inbranch set, because if all calls
are declare simd notinbranch, then predicating the loop will be just a
wasted effort.

> +		return true;
> +	      }
>  	  }
>  	return false;
>        }
> @@ -2614,6 +2622,31 @@ predicate_statements (loop_p loop)
>  	      gimple_assign_set_rhs1 (stmt, ifc_temp_var (type, rhs, &gsi));
>  	      update_stmt (stmt);
>  	    }
> +
> +	  /* Add a predicate parameter to functions that have a SIMD clone.
> +	     This will cause the vectorizer to match the "in branch" clone
> +	     variants because they also have the extra parameter, and serves
> +	     to build the mask vector in a natural way.  */
> +	  gcall *call = dyn_cast <gcall *> (gsi_stmt (gsi));
> +	  if (call && !gimple_call_internal_p (call))
> +	    {
> +	      tree orig_fndecl = gimple_call_fndecl (call);
> +	      int orig_nargs = gimple_call_num_args (call);
> +	      auto_vec<tree> args;
> +	      for (int i=0; i < orig_nargs; i++)
> +		args.safe_push (gimple_call_arg (call, i));
> +	      args.safe_push (cond);
> +
> +	      /* Replace the call with a new one that has the extra
> +		 parameter.  The FUNCTION_DECL remains unchanged so that
> +		 the vectorizer can find the SIMD clones.  This call will
> +		 either be deleted or replaced at that time, so the
> +		 mismatch is short-lived and we can live with it.  */
> +	      gcall *new_call = gimple_build_call_vec (orig_fndecl, args);
> +	      gimple_call_set_lhs (new_call, gimple_call_lhs (call));
> +	      gsi_replace (&gsi, new_call, true);

I think this is way too dangerous to represent conditional calls that way,
there is nothing to distinguish those from non-conditional calls.
I think I'd prefer (but please see what Richi thinks too) to represent
the conditional calls as a call to a new internal function, say
IFN_COND_CALL or IFN_MASK_CALL, which would have the arguments the original
call had, plus 2 extra ones first (or 3?), one that would be saved copy of
original gimple_call_fn (i.e. usually &fndecl), another one that would be the
condition (and dunno about whether we need also something to represent
gimple_call_fntype, or whether we simply should punt during ifcvt
on conditional calls where gimple_call_fntype is incompatible with
the function type of fndecl.  Another question is about
gimple_call_chain.  Punt or copy it over to the ifn and back.

	Jakub