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 E983C3858425 for ; Fri, 28 Apr 2023 16:06:18 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org E983C3858425 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=arm.com 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 6940AC14; Fri, 28 Apr 2023 09:07:02 -0700 (PDT) Received: from [10.57.70.10] (unknown [10.57.70.10]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id A1DEC3F64C; Fri, 28 Apr 2023 09:06:17 -0700 (PDT) Message-ID: Date: Fri, 28 Apr 2023 17:06:12 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.10.0 Subject: Re: [ping][vect-patterns] Refactor widen_plus/widen_minus as internal_fns Content-Language: en-US To: Richard Biener , Richard Sandiford Cc: Richard Biener , "gcc-patches@gcc.gnu.org" References: <51ce8969-3130-452e-092e-f9d91eff2dad@arm.com> From: "Andre Vieira (lists)" In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-9.7 required=5.0 tests=BAYES_00,KAM_DMARC_NONE,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,NICE_REPLY_A,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 25/04/2023 13:30, Richard Biener wrote: > On Mon, 24 Apr 2023, Richard Sandiford wrote: > >> Richard Biener writes: >>> On Thu, Apr 20, 2023 at 3:24?PM Andre Vieira (lists) via Gcc-patches >>> wrote: >>>> >>>> Rebased all three patches and made some small changes to the second one: >>>> - removed sub and abd optabs from commutative_optab_p, I suspect this >>>> was a copy paste mistake, >>>> - removed what I believe to be a superfluous switch case in vectorizable >>>> conversion, the one that was here: >>>> + if (code.is_fn_code ()) >>>> + { >>>> + internal_fn ifn = as_internal_fn (code.as_fn_code ()); >>>> + int ecf_flags = internal_fn_flags (ifn); >>>> + gcc_assert (ecf_flags & ECF_MULTI); >>>> + >>>> + switch (code.as_fn_code ()) >>>> + { >>>> + case CFN_VEC_WIDEN_PLUS: >>>> + break; >>>> + case CFN_VEC_WIDEN_MINUS: >>>> + break; >>>> + case CFN_LAST: >>>> + default: >>>> + return false; >>>> + } >>>> + >>>> + internal_fn lo, hi; >>>> + lookup_multi_internal_fn (ifn, &lo, &hi); >>>> + *code1 = as_combined_fn (lo); >>>> + *code2 = as_combined_fn (hi); >>>> + optab1 = lookup_multi_ifn_optab (lo, !TYPE_UNSIGNED (vectype)); >>>> + optab2 = lookup_multi_ifn_optab (hi, !TYPE_UNSIGNED (vectype)); >>>> } >>>> >>>> I don't think we need to check they are a specfic fn code, as we look-up >>>> optabs and if they succeed then surely we can vectorize? >>>> >>>> OK for trunk? >>> >>> In the first patch I see some uses of safe_as_tree_code like >>> >>> + if (ch.is_tree_code ()) >>> + return op1 == NULL_TREE ? gimple_build_assign (lhs, >>> ch.safe_as_tree_code (), >>> + op0) : >>> + gimple_build_assign (lhs, ch.safe_as_tree_code (), >>> + op0, op1); >>> + else >>> + { >>> + internal_fn fn = as_internal_fn (ch.safe_as_fn_code ()); >>> + gimple* stmt; >>> >>> where the context actually requires a valid tree code. Please change those >>> to force to tree code / ifn code. Just use explicit casts here and the other >>> places that are similar. Before the as_internal_fn just put a >>> gcc_assert (ch.is_internal_fn ()). >> >> Also, doesn't the above ?: simplify to the "else" arm? Null trailing >> arguments would be ignored for unary operators. >> >> I wasn't sure what to make of the op0 handling: >> >>> +/* Build a GIMPLE_ASSIGN or GIMPLE_CALL with the tree_code, >>> + or internal_fn contained in ch, respectively. */ >>> +gimple * >>> +vect_gimple_build (tree lhs, code_helper ch, tree op0, tree op1) >>> +{ >>> + if (op0 == NULL_TREE) >>> + return NULL; >> >> Can that happen, and if so, does returning null make sense? >> Maybe an assert would be safer. > > Yeah, I was hoping to have a look whether the new gimple_build > overloads could be used to make this all better (but hoped we can > finally get this series in in some way). > > Richard. Yeah, in the newest version of the first patch of the series I found that most of the time I can get away with only really needing to distinguish between tree_code and internal_fn when building gimple, for which it currently uses vect_gimple_build, but it does feel like that could easily be a gimple function. Having said that, as I partially mention in the patch, I didn't rewrite the optabs-tree supportable_half_widening and supportable_conversion (or whatever they are called) because those also at some point need to access the stmt and there is a massive difference in how we handle gassigns and gcall's from that perspective, but maybe we can generalize that too somehow... Anyway have a look at the new versions (posted just some minutes after the email I'm replying too haha! timing :P)