From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) by sourceware.org (Postfix) with ESMTPS id D93163858C60 for ; Mon, 3 Jan 2022 12:18:20 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org D93163858C60 Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out1.suse.de (Postfix) with ESMTP id A11B121155; Mon, 3 Jan 2022 12:18:19 +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 72DA1A3B83; Mon, 3 Jan 2022 12:18:19 +0000 (UTC) Date: Mon, 3 Jan 2022 13:18:19 +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: <9d3755df-6c41-20e4-31fb-811e5cd9182a@arm.com> Message-ID: <231396s0-2756-q51s-q55-o8npqo91on32@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> MIME-Version: 1.0 X-Spam-Status: No, score=-11.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, 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, 03 Jan 2022 12:18:22 -0000 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.