Re: [AArch64] Enable generation of FRINTNZ instructions

["Andre Vieira (lists)" <andre.simoesdiasvieira@arm.com>]

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.

Thanks,
Andre