On Mon, 10 Jan 2022, Andre Vieira (lists) wrote:
> Yeah seems I forgot to send the latest version, my bad.
>
> Bootstrapped on aarch64-none-linux.
>
> OK for trunk?
The match.pd part looks OK to me.
Thanks,
Richard.
> gcc/ChangeLog:
>
> * config/aarch64/aarch64.md (ftrunc<mode><frintnz_mode>2): New
> pattern.
> * config/aarch64/iterators.md (FRINTNZ): New iterator.
> (frintnz_mode): New int attribute.
> (VSFDF): Make iterator conditional.
> * internal-fn.def (FTRUNC_INT): New IFN.
> * internal-fn.c (ftrunc_int_direct): New define.
> (expand_ftrunc_int_optab_fn): New custom expander.
> (direct_ftrunc_int_optab_supported_p): New supported_p.
> * match.pd: Add to the existing TRUNC pattern match.
> * optabs.def (ftrunc_int): New entry.
> * stor-layout.h (element_precision): Moved from here...
> * tree.h (element_precision): ... to here.
> (element_type): New declaration.
> * tree.c (element_type): New function.
> (element_precision): Changed to use element_type.
> * tree-vect-stmts.c (vectorizable_internal_function): Add
> support for
> IFNs with different input types.
> (vectorizable_call): Teach to handle IFN_FTRUNC_INT.
> * doc/md.texi: New entry for ftrunc pattern name.
> * doc/sourcebuild.texi (aarch64_frintzx_ok): New target.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/aarch64/merge_trunc1.c: Adapted to skip if frintNz
> instruction available.
> * lib/target-supports.exp: Added arm_v8_5a_frintnzx_ok target.
> * gcc.target/aarch64/frintnz.c: New test.
> * gcc.target/aarch64/frintnz_vec.c: New test.
>
> On 03/01/2022 12:18, Richard Biener wrote:
> > 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.
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Ivo Totev; HRB 36809 (AG Nuernberg)
