Re: [AArch64] Enable generation of FRINTNZ instructions

[Richard Biener <rguenther@suse.de>]

On Wed, 29 Dec 2021, Andre Vieira (lists) wrote:

> Hi Richard,
> 
> Thank you for the review, I've adopted all above suggestions downstream, I am
> still surprised how many style things I still miss after years of gcc
> development :(
> 
> On 17/12/2021 12:44, Richard Sandiford wrote:
> >
> >> @@ -3252,16 +3257,31 @@ vectorizable_call (vec_info *vinfo,
> >>         rhs_type = unsigned_type_node;
> >>       }
> >>   -  int mask_opno = -1;
> >> +  /* The argument that is not of the same type as the others.  */
> >> +  int diff_opno = -1;
> >> +  bool masked = false;
> >>     if (internal_fn_p (cfn))
> >> -    mask_opno = internal_fn_mask_index (as_internal_fn (cfn));
> >> +    {
> >> +      if (cfn == CFN_FTRUNC_INT)
> >> +	/* For FTRUNC this represents the argument that carries the type of
> >> the
> >> +	   intermediate signed integer.  */
> >> +	diff_opno = 1;
> >> +      else
> >> +	{
> >> +	  /* For masked operations this represents the argument that carries
> >> the
> >> +	     mask.  */
> >> +	  diff_opno = internal_fn_mask_index (as_internal_fn (cfn));
> >> +	  masked = diff_opno >=  0;
> >> +	}
> >> +    }
> > I think it would be cleaner not to process argument 1 at all for
> > CFN_FTRUNC_INT.  There's no particular need to vectorise it.
> 
> I agree with this,  will change the loop to continue for argument 1 when
> dealing with non-masked CFN's.
> 
> >>   	}
> >> […]
> >> diff --git a/gcc/tree.c b/gcc/tree.c
> >> index
> >> 845228a055b2cfac0c9ca8c0cda1b9df4b0095c6..f1e9a1eb48769cb11aa69730e2480ed5522f78c1
> >> 100644
> >> --- a/gcc/tree.c
> >> +++ b/gcc/tree.c
> >> @@ -6645,11 +6645,11 @@ valid_constant_size_p (const_tree size,
> >> cst_size_error *perr /* = NULL */)
> >>     return true;
> >>   }
> >>   
> >> -/* Return the precision of the type, or for a complex or vector type the
> >> -   precision of the type of its elements.  */
> >> +/* Return the type, or for a complex or vector type the type of its
> >> +   elements.  */
> >>   -unsigned int
> >> -element_precision (const_tree type)
> >> +tree
> >> +element_type (const_tree type)
> >>   {
> >>     if (!TYPE_P (type))
> >>       type = TREE_TYPE (type);
> >> @@ -6657,7 +6657,16 @@ element_precision (const_tree type)
> >>     if (code == COMPLEX_TYPE || code == VECTOR_TYPE)
> >>       type = TREE_TYPE (type);
> >>   -  return TYPE_PRECISION (type);
> >> +  return (tree) type;
> > I think we should stick a const_cast in element_precision and make
> > element_type take a plain “type”.  As it stands element_type is an
> > implicit const_cast for many cases.
> >
> > Thanks,
> Was just curious about something here, I thought the purpose of having
> element_precision (before) and element_type (now) take a const_tree as an
> argument was to make it clear we aren't changing the input type. I understand
> that as it stands element_type could be an implicit const_cast (which I should
> be using rather than the '(tree)' cast), but that's only if 'type' is a type
> that isn't complex/vector, either way, we are conforming to the promise that
> we aren't changing the incoming type, what the caller then does with the
> result is up to them no?
> 
> I don't mind making the changes, just trying to understand the reasoning
> behind it.
> 
> I'll send in a new patch with all the changes after the review on the match.pd
> stuff.

I'm missing an updated patch after my initial review of the match.pd
stuff so not sure what to review.  Can you re-post and updated patch?

Thanks,
Richard.