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 BAC833858D1E for ; Tue, 25 Apr 2023 09:55:53 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org BAC833858D1E 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 2D87F1FB; Tue, 25 Apr 2023 02:56:37 -0700 (PDT) Received: from [10.57.69.200] (unknown [10.57.69.200]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 828113F5A1; Tue, 25 Apr 2023 02:55:52 -0700 (PDT) Message-ID: Date: Tue, 25 Apr 2023 10:55:46 +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 Cc: Richard Biener , Richard Sandiford , "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: 8bit 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 24/04/2023 12:57, Richard Biener wrote: > 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 ()). > > Maybe the need for the (ugly) safe_as_tree_code/fn_code goes away then. > > Otherwise patch1 looks OK. > > Unfortunately there are no ChangeLog / patch descriptions on the changes. > patch2 has > > - tree_code rhs_code = gimple_assign_rhs_code (assign); > - if (rhs_code != code && rhs_code != widened_code) > + code_helper rhs_code; > + if (is_gimple_assign (stmt)) > + { > + rhs_code = gimple_assign_rhs_code (stmt); > + if (rhs_code.safe_as_tree_code () != code > + && rhs_code.get_rep () != widened_code.get_rep ()) > + return 0; > + } > + else if (is_gimple_call (stmt)) > + { > + rhs_code = gimple_call_combined_fn (stmt); > + if (rhs_code.get_rep () != widened_code.get_rep ()) > + return 0; > + } > > that looks mightly complicated - esp. the use of get_rep () > looks dangerous? What's the intent of this? Not that I > understand the existing code much. A comment would > clearly help (also indicating test coverage). I don't think the use of get_rep here is dangerous, it's meant to avoid having to check whether widened_code is the same 'kind' as rhs_code. With get_rep we don't have to do this check first because tree_codes will have positive reps and combined_fns negative reps. Having said that, this can all be simplified and we don't need to use get_rep either as the == operator has been overloaded to use get_rep and even use the constructor on the rhs of the ==, so I suggest moving the check after assigning rhs_code and just doing: if (rhs_code != code && rhs_code != widened_code) return 0; > > >> Kind regards, >> Andre