From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id D4F383858D28 for ; Wed, 29 Dec 2021 15:55:40 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org D4F383858D28 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 726B56D; Wed, 29 Dec 2021 07:55:40 -0800 (PST) Received: from [10.57.6.172] (unknown [10.57.6.172]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id DE2663F774; Wed, 29 Dec 2021 07:55:39 -0800 (PST) Message-ID: <9d3755df-6c41-20e4-31fb-811e5cd9182a@arm.com> Date: Wed, 29 Dec 2021 15:55:41 +0000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.4.1 Subject: Re: [AArch64] Enable generation of FRINTNZ instructions Content-Language: en-US To: "Andre Vieira (lists) via Gcc-patches" , Richard Biener , richard.sandiford@arm.com 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> From: "Andre Vieira (lists)" In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-14.3 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, NICE_REPLY_A, 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 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: Wed, 29 Dec 2021 15:55:42 -0000 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