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.