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 33F2E3858D28 for ; Wed, 3 May 2023 12:11:43 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 33F2E3858D28 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.de Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out1.suse.de (Postfix) with ESMTP id 59CE122747; Wed, 3 May 2023 12:11:42 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1683115902; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=dwP415ou+in5MBmu2XaZkhxVCRkalJYFrl5PzA16aHo=; b=izJcxL2km2hS3ZQoYCgbfV4umu6Ge6DvtxDAHlHvFxkLdTzQ0BREzi3DyLAF5Jap1y473B SlsHiYJ/qKBFvxX9fe23up4lKx5EwJ/NT9jjxSP5DGw0ylEAxNM+aqj6LY7qT3XBWMlxcu N7NZ7eKGVZ4KmtgWVdKQzLDd2QImWP0= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1683115902; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=dwP415ou+in5MBmu2XaZkhxVCRkalJYFrl5PzA16aHo=; b=66BbZRFt1Wqb8buYhUIc1J/7ZOtw9P02CtoI3i1D3ALmFBzCDmy2cWmcrzAmtJe+QvoPLE 7GKYzEzaVoaZG3DQ== Received: from wotan.suse.de (wotan.suse.de [10.160.0.1]) (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 402272C141; Wed, 3 May 2023 12:11:42 +0000 (UTC) Date: Wed, 3 May 2023 12:11:42 +0000 (UTC) From: Richard Biener To: "Andre Vieira (lists)" cc: Richard Biener , Richard Sandiford , "gcc-patches@gcc.gnu.org" Subject: Re: [PATCH 2/3] Refactor widen_plus as internal_fn In-Reply-To: Message-ID: References: <51ce8969-3130-452e-092e-f9d91eff2dad@arm.com> User-Agent: Alpine 2.22 (LSU 394 2020-01-19) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-5.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,KAM_SHORT,SPF_HELO_NONE,SPF_PASS,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 Fri, 28 Apr 2023, Andre Vieira (lists) wrote: > This patch replaces the existing tree_code widen_plus and widen_minus > patterns with internal_fn versions. > > DEF_INTERNAL_OPTAB_HILO_FN is like DEF_INTERNAL_OPTAB_FN except it provides > convenience wrappers for defining conversions that require a hi/lo split, like > widening and narrowing operations. Each definition for will require an > optab named and two other optabs that you specify for signed and > unsigned. The hi/lo pair is necessary because the widening operations take n > narrow elements as inputs and return n/2 wide elements as outputs. The 'lo' > operation operates on the first n/2 elements of input. The 'hi' operation > operates on the second n/2 elements of input. Defining an internal_fn along > with hi/lo variations allows a single internal function to be returned from a > vect_recog function that will later be expanded to hi/lo. > > DEF_INTERNAL_OPTAB_HILO_FN is used in internal-fn.def to register a widening > internal_fn. It is defined differently in different places and internal-fn.def > is sourced from those places so the parameters given can be reused. > internal-fn.c: defined to expand to hi/lo signed/unsigned optabs, later > defined to generate the 'expand_' functions for the hi/lo versions of the fn. > internal-fn.def: defined to invoke DEF_INTERNAL_OPTAB_FN for the original > and hi/lo variants of the internal_fn > > For example: > IFN_VEC_WIDEN_PLUS -> IFN_VEC_WIDEN_PLUS_HI, IFN_VEC_WIDEN_PLUS_LO > for aarch64: IFN_VEC_WIDEN_PLUS_HI -> vec_widen_addl_hi_ -> > (u/s)addl2 > IFN_VEC_WIDEN_PLUS_LO -> vec_widen_addl_lo_ > -> (u/s)addl > > This gives the same functionality as the previous WIDEN_PLUS/WIDEN_MINUS tree > codes which are expanded into VEC_WIDEN_PLUS_LO, VEC_WIDEN_PLUS_HI. I'll note that it's interesting we have widen multiplication as the only existing example where we have both HI/LO and EVEN/ODD cases. I think we want to share as much of the infrastructure to eventually support targets doing even/odd (I guess all VLA vector targets will be even/odd?). DEF_INTERNAL_OPTAB_HILO_FN also looks to be implicitely directed to widening operations (otherwise no signed/unsigned variants would be necessary). What I don't understand is why we need an optab without _hi/_lo but in that case no signed/unsigned variant? Looks like all plus, plus_lo and plus_hi are commutative but only plus is widening?! So is the setup that the vectorizer doesn't know about the split and uses 'plus' but then the expander performs the split? It does look a bit awkward here (the plain 'plus' is just used for the scalar case during pattern recog it seems). I'd rather have DEF_INTERNAL_OPTAB_HILO_FN split up, declaring the hi/lo pairs and the scalar variant separately using DEF_INTERNAL_FN without expander for that, and having DEF_INTERNAL_HILO_WIDEN_OPTAB_FN and DEF_INTERNAL_EVENODD_WIDEN_OPTAB_FN for the signed/unsigned pairs? (if we need that helper at all) Targets shouldn't need to implement the plain optab (it shouldn't exist) and the vectorizer should query the hi/lo or even/odd optabs for support instead. The vectorizer parts look OK to me, I'd like Richard to chime in on the optab parts as well. Thanks, Richard. > gcc/ChangeLog: > > 2023-04-28 Andre Vieira > Joel Hutton > Tamar Christina > > * internal-fn.cc (INCLUDE_MAP): Include maps for use in optab > lookup. > (DEF_INTERNAL_OPTAB_HILO_FN): Macro to define an internal_fn that > expands into multiple internal_fns (for widening). > (ifn_cmp): Function to compare ifn's for sorting/searching. > (lookup_hilo_ifn_optab): Add lookup function. > (lookup_hilo_internal_fn): Add lookup function. > (commutative_binary_fn_p): Add widen_plus fn's. > (widening_fn_p): New function. > (decomposes_to_hilo_fn_p): New function. > * internal-fn.def (DEF_INTERNAL_OPTAB_HILO_FN): Define widening > plus,minus functions. > (VEC_WIDEN_PLUS): Replacement for VEC_WIDEN_PLUS tree code. > (VEC_WIDEN_MINUS): Replacement for VEC_WIDEN_MINUS tree code. > * internal-fn.h (GCC_INTERNAL_FN_H): Add headers. > (lookup_hilo_ifn_optab): Add prototype. > (lookup_hilo_internal_fn): Likewise. > (widening_fn_p): Likewise. > (decomposes_to_hilo_fn_p): Likewise. > * optabs.cc (commutative_optab_p): Add widening plus, minus optabs. > * optabs.def (OPTAB_CD): widen add, sub optabs > * tree-vect-patterns.cc (vect_recog_widen_op_pattern): Support > patterns with a hi/lo split. > (vect_recog_widen_plus_pattern): Refactor to return > IFN_VECT_WIDEN_PLUS. > (vect_recog_widen_minus_pattern): Refactor to return new > IFN_VEC_WIDEN_MINUS. > * tree-vect-stmts.cc (vectorizable_conversion): Add widen plus/minus > ifn > support. > (supportable_widening_operation): Add widen plus/minus ifn support. > > gcc/testsuite/ChangeLog: > > * gcc.target/aarch64/vect-widen-add.c: Test that new > IFN_VEC_WIDEN_PLUS is being used. > * gcc.target/aarch64/vect-widen-sub.c: Test that new > IFN_VEC_WIDEN_MINUS is being used. > -- Richard Biener SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman; HRB 36809 (AG Nuernberg)