RE: [PATCH 1/3 v3] vect: generate suitable convert insn for int -> int, float -> float and int <-> float.

[Richard Biener <rguenther@suse.de>]

On Thu, 20 Jun 2024, Hu, Lin1 wrote:

> > >    else if (ret_elt_bits > arg_elt_bits)
> > >      modifier = WIDEN;
> > >
> > > +  if (supportable_convert_operation (code, ret_type, arg_type, &code1))
> > > +    {
> > > +      g = gimple_build_assign (lhs, code1, arg);
> > > +      gsi_replace (gsi, g, false);
> > > +      return;
> > > +    }
> > 
> > Given the API change I suggest below it might make sense to have
> > supportable_indirect_convert_operation do the above and represent it as single-
> > step conversion?
> >
> 
> OK, if you want to supportable_indirect_convert_operation can do 
> something like supportable_convert_operation, I'll give it a try. This 
> functionality is really the part that this function can cover. But this 
> would require some changes not only the API change, because 
> supportable_indirect_convert_operation originally only supported Float 
> -> Int or Int ->Float.

I think I'd like to see a single API to handle direct and
(multi-)indirect-level converts that operate on vectors with all
the same number of lanes.

> >
> > > +  code_helper code2 = ERROR_MARK, code3 = ERROR_MARK;
> > > +  int multi_step_cvt = 0;
> > > +  vec<tree> interm_types = vNULL;
> > > +  if (supportable_indirect_convert_operation (NULL,
> > > +					      code,
> > > +					      ret_type, arg_type,
> > > +					      &code2, &code3,
> > > +					      &multi_step_cvt,
> > > +					      &interm_types, arg))
> > > +    {
> > > +      new_rhs = make_ssa_name (interm_types[0]);
> > > +      g = gimple_build_assign (new_rhs, (tree_code) code3, arg);
> > > +      gsi_insert_before (gsi, g, GSI_SAME_STMT);
> > > +      g = gimple_build_assign (lhs, (tree_code) code2, new_rhs);
> > > +      gsi_replace (gsi, g, false);
> > > +      return;
> > > +    }
> > > +
> > >    if (modifier == NONE && (code == FIX_TRUNC_EXPR || code ==
> > FLOAT_EXPR))
> > >      {
> > > -      if (supportable_convert_operation (code, ret_type, arg_type, &code1))
> > > -	{
> > > -	  g = gimple_build_assign (lhs, code1, arg);
> > > -	  gsi_replace (gsi, g, false);
> > > -	  return;
> > > -	}
> > >        /* Can't use get_compute_type here, as supportable_convert_operation
> > >  	 doesn't necessarily use an optab and needs two arguments.  */
> > >        tree vec_compute_type
> > > diff --git a/gcc/tree-vect-stmts.cc b/gcc/tree-vect-stmts.cc index
> > > 05a169ecb2d..0aa608202ca 100644
> > > --- a/gcc/tree-vect-stmts.cc
> > > +++ b/gcc/tree-vect-stmts.cc
> > > @@ -5175,7 +5175,7 @@ vectorizable_conversion (vec_info *vinfo,
> > >    tree scalar_dest;
> > >    tree op0, op1 = NULL_TREE;
> > >    loop_vec_info loop_vinfo = dyn_cast <loop_vec_info> (vinfo);
> > > -  tree_code tc1, tc2;
> > > +  tree_code tc1;
> > >    code_helper code, code1, code2;
> > >    code_helper codecvt1 = ERROR_MARK, codecvt2 = ERROR_MARK;
> > >    tree new_temp;
> > > @@ -5384,92 +5384,17 @@ vectorizable_conversion (vec_info *vinfo,
> > >  	break;
> > >        }
> > >
> > > -      /* For conversions between float and integer types try whether
> > > -	 we can use intermediate signed integer types to support the
> > > -	 conversion.  */
> > > -      if (GET_MODE_SIZE (lhs_mode) != GET_MODE_SIZE (rhs_mode)
> > > -	  && (code == FLOAT_EXPR ||
> > > -	      (code == FIX_TRUNC_EXPR && !flag_trapping_math)))
> > > -	{
> > > -	  bool demotion = GET_MODE_SIZE (rhs_mode) > GET_MODE_SIZE
> > (lhs_mode);
> > > -	  bool float_expr_p = code == FLOAT_EXPR;
> > > -	  unsigned short target_size;
> > > -	  scalar_mode intermediate_mode;
> > > -	  if (demotion)
> > > -	    {
> > > -	      intermediate_mode = lhs_mode;
> > > -	      target_size = GET_MODE_SIZE (rhs_mode);
> > > -	    }
> > > -	  else
> > > -	    {
> > > -	      target_size = GET_MODE_SIZE (lhs_mode);
> > > -	      if (!int_mode_for_size
> > > -		  (GET_MODE_BITSIZE (rhs_mode), 0).exists
> > (&intermediate_mode))
> > > -		goto unsupported;
> > > -	    }
> > > -	  code1 = float_expr_p ? code : NOP_EXPR;
> > > -	  codecvt1 = float_expr_p ? NOP_EXPR : code;
> > > -	  opt_scalar_mode mode_iter;
> > > -	  FOR_EACH_2XWIDER_MODE (mode_iter, intermediate_mode)
> > > -	    {
> > > -	      intermediate_mode = mode_iter.require ();
> > > -
> > > -	      if (GET_MODE_SIZE (intermediate_mode) > target_size)
> > > -		break;
> > > -
> > > -	      scalar_mode cvt_mode;
> > > -	      if (!int_mode_for_size
> > > -		  (GET_MODE_BITSIZE (intermediate_mode), 0).exists
> > (&cvt_mode))
> > > -		break;
> > > -
> > > -	      cvt_type = build_nonstandard_integer_type
> > > -		(GET_MODE_BITSIZE (cvt_mode), 0);
> > > -
> > > -	      /* Check if the intermediate type can hold OP0's range.
> > > -		 When converting from float to integer this is not necessary
> > > -		 because values that do not fit the (smaller) target type are
> > > -		 unspecified anyway.  */
> > > -	      if (demotion && float_expr_p)
> > > -		{
> > > -		  wide_int op_min_value, op_max_value;
> > > -		  if (!vect_get_range_info (op0, &op_min_value,
> > &op_max_value))
> > > -		    break;
> > > -
> > > -		  if (cvt_type == NULL_TREE
> > > -		      || (wi::min_precision (op_max_value, SIGNED)
> > > -			  > TYPE_PRECISION (cvt_type))
> > > -		      || (wi::min_precision (op_min_value, SIGNED)
> > > -			  > TYPE_PRECISION (cvt_type)))
> > > -		    continue;
> > > -		}
> > > -
> > > -	      cvt_type = get_vectype_for_scalar_type (vinfo, cvt_type, slp_node);
> > > -	      /* This should only happened for SLP as long as loop vectorizer
> > > -		 only supports same-sized vector.  */
> > > -	      if (cvt_type == NULL_TREE
> > > -		  || maybe_ne (TYPE_VECTOR_SUBPARTS (cvt_type), nunits_in)
> > > -		  || !supportable_convert_operation ((tree_code) code1,
> > > -						     vectype_out,
> > > -						     cvt_type, &tc1)
> > > -		  || !supportable_convert_operation ((tree_code) codecvt1,
> > > -						     cvt_type,
> > > -						     vectype_in, &tc2))
> > > -		continue;
> > > -
> > > -	      found_mode = true;
> > > -	      break;
> > > -	    }
> > > +      if (supportable_indirect_convert_operation (vinfo,
> > > +						  code,
> > > +						  vectype_out,
> > > +						  vectype_in,
> > > +						  &code1,
> > > +						  &codecvt1,
> > > +						  &multi_step_cvt,
> > > +						  &interm_types,
> > > +						  op0,slp_node))
> > > +	break;
> > >
> > > -	  if (found_mode)
> > > -	    {
> > > -	      multi_step_cvt++;
> > > -	      interm_types.safe_push (cvt_type);
> > > -	      cvt_type = NULL_TREE;
> > > -	      code1 = tc1;
> > > -	      codecvt1 = tc2;
> > > -	      break;
> > > -	    }
> > > -	}
> > >        /* FALLTHRU */
> > >      unsupported:
> > >        if (dump_enabled_p ())
> > > @@ -14626,6 +14551,153 @@ supportable_narrowing_operation
> > (code_helper code,
> > >    return false;
> > >  }
> > >
> > > +/* Function supportable_indirect_convert_operation
> > > +
> > > +   Check whether an operation represented by the code CODE is two
> > > +   convert operations that are supported by the target platform in
> > > +   vector form (i.e., when operating on arguments of type VECTYPE_IN
> > > +   producing a result of type VECTYPE_OUT).
> > > +
> > > +   Convert operations we currently support directly are FIX_TRUNC and FLOAT.
> > > +   This function checks if these operations are supported
> > > +   by the target platform directly (via vector tree-codes).
> > > +
> > > +   Output:
> > > +   - CODE1 is the code of a vector operation to be used when
> > > +   converting the operation in the first step, if available.
> > > +   - CODE2 is the code of a vector operation to be used when
> > > +   converting the operation in the second step, if available.
> > > +   - MULTI_STEP_CVT determines the number of required intermediate steps
> > in
> > > +   case of multi-step conversion (like int->short->char - in that case
> > > +   MULTI_STEP_CVT will be 1). In the function, it should be 1.
> > > +   - INTERM_TYPES contains the intermediate type required to perform the
> > > +   convert operation (short in the above example).   */
> > > +bool
> > > +supportable_indirect_convert_operation (vec_info *vinfo,
> > > +					code_helper code,
> > > +					tree vectype_out,
> > > +					tree vectype_in,
> > > +					code_helper *code1,
> > > +					code_helper *code2,
> > > +					int *multi_step_cvt,
> > > +					vec<tree> *interm_types,
> > 
> > This API is somewhat awkward, as we're inventing a new one I guess we can do
> > better.  I think we want
> > 
> >       vec<std::pair<tree, tree_code> > *converts,
> > 
> > covering all code1, code2, multi_step_cvt and interm_types with the conversion
> > sequence being
> > 
> >       converts[0].first tem0 = converts[0].second op0;
> >       converts[1].first tem1 = converts[1].second tem;
> > 
> 
> That's great, this really makes the function work better.
> 
> >
> > ... while converts.length () determines the length of the chain, one being a direct
> > conversion where then converts[0].first is vectype_out.  That would allow
> > double -> char to go double -> float -> int -> short -> char for example.
> >
> 
> I'm trying to determine the requirements, do you want this function to 
> support multiple conversions (the current implementation just does a 
> two-step conversion, like double -> char, which becomes double -> int -> 
> char). Actually we should be able to do all conversions in two steps, if 
> we have some suitable instructions. I can't think of a scenario where 
> multiple conversions are needed yet. Could you give me some examples? Of 
> course, I could tweak this feature in advance if it is for future 
> consideration.

I think the API should support multi-level, not only two levels.  The
implementation doesn't need to cover that case unless we run into
such a requirement.  Usually vector ISAs allow 2x integer 
widening/shortening but not 4x, so a VnDImode -> VnQImode conversion
would need to go via VnSImode and VnHImode (of course some targets
might "help" the vectorizer by providing a VnDImode -> VnQImode
pattern that does the intermediate conversions behind the vectorizers
back).  But yes, the original motivation for the vectorizer code
was that float<->int conversions are limited.

Thanks,
Richard.

> 
> >
> > > +					tree op0,
> > > +					slp_tree slp_node)
> > 
> > I would like to avoid passing VINFO and SLP_NODE here, see below.
> > The same is true for OP0 where the existing use is wrong for SLP already, but I
> > guess that can stay for now (I opened PR115538 about the wrong-code issue).
> >
> 
> Thanks, I have removed them. 
>  
> >
> > > +{
> > > +  bool found_mode = false;
> > > +  scalar_mode lhs_mode = GET_MODE_INNER (TYPE_MODE (vectype_out));
> > > +  scalar_mode rhs_mode = GET_MODE_INNER (TYPE_MODE (vectype_in));
> > > +  opt_scalar_mode mode_iter;
> > > +  tree_code tc1, tc2;
> > > +
> > > +  tree cvt_type = NULL_TREE;
> > > +  poly_uint64 nelts = TYPE_VECTOR_SUBPARTS (vectype_in);
> > > +
> > > +  (*multi_step_cvt) = 0;
> > > +  /* For conversions between float and integer types try whether
> > > +     we can use intermediate signed integer types to support the
> > > +     conversion.  */
> > > +  if (GET_MODE_SIZE (lhs_mode) != GET_MODE_SIZE (rhs_mode)
> > > +      && (code == FLOAT_EXPR
> > > +	  || (code == FIX_TRUNC_EXPR && !flag_trapping_math)))
> > > +    {
> > > +      bool demotion = GET_MODE_SIZE (rhs_mode) > GET_MODE_SIZE
> > (lhs_mode);
> > > +      bool float_expr_p = code == FLOAT_EXPR;
> > > +      unsigned short target_size;
> > > +      scalar_mode intermediate_mode;
> > > +      if (demotion)
> > > +	{
> > > +	  intermediate_mode = lhs_mode;
> > > +	  target_size = GET_MODE_SIZE (rhs_mode);
> > > +	}
> > > +      else
> > > +	{
> > > +	  target_size = GET_MODE_SIZE (lhs_mode);
> > > +	  if (!int_mode_for_size
> > > +	      (GET_MODE_BITSIZE (rhs_mode), 0).exists (&intermediate_mode))
> > > +	    return false;
> > > +	}
> > > +      *code1 = float_expr_p ? code : NOP_EXPR;
> > > +      *code2 = float_expr_p ? NOP_EXPR : code;
> > > +      opt_scalar_mode mode_iter;
> > > +      FOR_EACH_2XWIDER_MODE (mode_iter, intermediate_mode)
> > > +	{
> > > +	  intermediate_mode = mode_iter.require ();
> > > +
> > > +	  if (GET_MODE_SIZE (intermediate_mode) > target_size)
> > > +	    break;
> > > +
> > > +	  scalar_mode cvt_mode;
> > > +	  if (!int_mode_for_size
> > > +	      (GET_MODE_BITSIZE (intermediate_mode), 0).exists (&cvt_mode))
> > > +	    break;
> > > +
> > > +	  cvt_type = build_nonstandard_integer_type
> > > +	    (GET_MODE_BITSIZE (cvt_mode), 0);
> > > +
> > > +	  /* Check if the intermediate type can hold OP0's range.
> > > +	     When converting from float to integer this is not necessary
> > > +	     because values that do not fit the (smaller) target type are
> > > +	     unspecified anyway.  */
> > > +	  if (demotion && float_expr_p)
> > > +	    {
> > > +	      wide_int op_min_value, op_max_value;
> > > +	      /* For vector form, it looks like op0 doesn't have RANGE_INFO.
> > > +		 In the future, if it is supported, changes may need to be made
> > > +		 to this part, such as checking the RANGE of each element
> > > +		 in the vector.  */
> > > +	      if (!SSA_NAME_RANGE_INFO (op0)
> > > +		  || !vect_get_range_info (op0, &op_min_value,
> > &op_max_value))
> > > +		break;
> > > +
> > > +	      if (cvt_type == NULL_TREE
> > > +		  || (wi::min_precision (op_max_value, SIGNED)
> > > +		      > TYPE_PRECISION (cvt_type))
> > > +		  || (wi::min_precision (op_min_value, SIGNED)
> > > +		      > TYPE_PRECISION (cvt_type)))
> > > +		continue;
> > > +	    }
> > > +
> > > +	  if (vinfo != NULL && slp_node != NULL)
> > > +	    cvt_type = get_vectype_for_scalar_type (vinfo, cvt_type, slp_node);
> > > +	  else
> > > +	    {
> > > +	      bool uns = TYPE_UNSIGNED (TREE_TYPE (vectype_out))
> > > +			 || TYPE_UNSIGNED (TREE_TYPE (vectype_in));
> > > +	      cvt_type = build_nonstandard_integer_type
> > > +		(GET_MODE_BITSIZE (cvt_mode), uns);
> > > +	      cvt_type = build_vector_type (cvt_type, nelts);
> > > +	    }
> > 
> > So this would then become
> > 
> >           cvt_type = get_related_vectype_for_scalar_type (TYPE_MODE
> > (vectype_in), cvt_type, TYPE_VECTOR_SUBPARTS (vectype_in));
> > 
> > > +	  /* This should only happened for SLP as long as loop vectorizer
> > > +	     only supports same-sized vector.  */
> > > +	  if (cvt_type == NULL_TREE
> > > +	      || maybe_ne (TYPE_VECTOR_SUBPARTS (cvt_type), nelts)
> > > +	      || !supportable_convert_operation ((tree_code) *code1,
> > > +						 vectype_out,
> > > +						 cvt_type, &tc1)
> > > +	      || !supportable_convert_operation ((tree_code) *code2,
> > > +						 cvt_type,
> > > +						 vectype_in, &tc2))
> > > +	    continue;
> > > +
> > > +	  found_mode = true;
> > > +	  break;
> > > +	}
> > > +
> > > +      if (found_mode)
> > > +	{
> > > +	  (*multi_step_cvt)++;
> > > +	  interm_types->safe_push (cvt_type);
> > > +	  cvt_type = NULL_TREE;
> > > +	  *code1 = tc1;
> > > +	  *code2 = tc2;
> > > +	  return true;
> > > +	}
> > > +    }
> > > +  interm_types->release ();
> > 
> > Hmm, ownership of interm_types is somewhat unclear here - the caller should
> > release it, or is the situation that the caller is confused by stray elements in it?  In
> > that case I'd suggest to instead do interm_types->truncate (0).
> >
> 
> It's my fault, I just imitate supportable_narrowing/widening_operation, 
> I think for this function, interm_types->release() is not needed.