From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.220.29]) by sourceware.org (Postfix) with ESMTPS id 36ECE3858C60 for ; Mon, 10 Jan 2022 14:45:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 36ECE3858C60 Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id 115A51F383; Mon, 10 Jan 2022 14:45:24 +0000 (UTC) Received: from murzim.suse.de (murzim.suse.de [10.160.4.192]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id 0B272A3B87; Mon, 10 Jan 2022 14:45:24 +0000 (UTC) Date: Mon, 10 Jan 2022 15:45:23 +0100 (CET) From: Richard Biener To: "Andre Vieira (lists)" cc: "Andre Vieira (lists) via Gcc-patches" , richard.sandiford@arm.com Subject: Re: [AArch64] Enable generation of FRINTNZ instructions In-Reply-To: <5d7bb7af-b09e-cb91-b457-c6148f65028e@arm.com> Message-ID: <498n407n-s4p3-1472-s98q-rq10s8o55165@fhfr.qr> References: <8225375c-eb9e-f9b3-6bcd-9fbccf2fc87b@arm.com> <70s9nn94-452-5rrr-4458-q6n3qp563652@fhfr.qr> <36e3469a-3922-d49e-4006-0088eac29157@arm.com> <653o8886-3p5n-sr82-9n83-71q33o8824@fhfr.qr> <6c730f35-10b1-2843-cffc-4ed0851380be@arm.com> <85sr96q-o3s-602o-3436-40713n68pp84@fhfr.qr> <8d593d5f-41a0-6051-0ce0-d72834ecfa25@arm.com> <9d3755df-6c41-20e4-31fb-811e5cd9182a@arm.com> <231396s0-2756-q51s-q55-o8npqo91on32@fhfr.qr> <5d7bb7af-b09e-cb91-b457-c6148f65028e@arm.com> MIME-Version: 1.0 X-Spam-Status: No, score=-9.5 required=5.0 tests=BAYES_00, BODY_8BITS, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT X-Content-Filtered-By: Mailman/MimeDel 2.1.29 X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 10 Jan 2022 14:45:26 -0000 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 (ftrunc2): 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 SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany; GF: Ivo Totev; HRB 36809 (AG Nuernberg)