From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate.crashing.org (gate.crashing.org [63.228.1.57]) by sourceware.org (Postfix) with ESMTP id 25BE2385559E for ; Thu, 30 Mar 2023 13:42:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 25BE2385559E Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=kernel.crashing.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=kernel.crashing.org Received: from gate.crashing.org (localhost.localdomain [127.0.0.1]) by gate.crashing.org (8.14.1/8.14.1) with ESMTP id 32UDfFpw007668; Thu, 30 Mar 2023 08:41:15 -0500 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id 32UDfE92007667; Thu, 30 Mar 2023 08:41:14 -0500 X-Authentication-Warning: gate.crashing.org: segher set sender to segher@kernel.crashing.org using -f Date: Thu, 30 Mar 2023 08:41:13 -0500 From: Segher Boessenkool To: Ajit Agarwal Cc: gcc-patches , Peter Bergner , Richard Biener , Jeff Law , Raphael Zinsly Subject: Re: [PATCH v2] rtl-optimization: ppc backend generates unnecessary extension. Message-ID: <20230330134113.GO25951@gate.crashing.org> References: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.4.2.3i X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,JMQ_SPF_NEUTRAL,KAM_DMARC_STATUS,SPF_HELO_PASS,SPF_PASS,TXREP autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Hi! On Tue, Mar 28, 2023 at 10:19:27PM +0530, Ajit Agarwal wrote: > This patch makes REE pass as a default pass in rs6000 target. And > add necessary subroutines to eliminate extensions across basic blocks. Please wait with this until stage 1? Some comments: > rtl-optimization: ppc backend generates unnecessary > extension. Subject can start with "ree: ", but not "rtl-optimization: ". Subject should start with a capital (the part after the colon, that is). There should not be a full stop in the subject. Subjects should be in the mail subject, not elsewhere in the patch. > Eliminate unnecessary redundant zero extension across basic > blocks. Not sure where you want this? It doesn't belong in the changelog. > * ree.cc(insn_s_zext_p): New function. Space before (. > * ree.cc(is_feasible_elim_across_basic_blocks): > New function. This is the same file as the one before this. You write that as (is_feasible_elim_across_basic_blocks): New function. (and no early line breaks please, certainly not after a colon). > * common/config/rs6000/rs6000-common.cc: Add free pass > as default pass in rs6000 target. Changelog lines are 80 positions. The leading tab is 8 positions. > @@ -30,6 +30,8 @@ > /* Implement TARGET_OPTION_OPTIMIZATION_TABLE. */ > static const struct default_options rs6000_option_optimization_table[] = > { > + /* Enable -free for zero extension and sign extension elimination.*/ > + { OPT_LEVELS_2_PLUS, OPT_free, NULL, 1 }, > /* Split multi-word types early. */ > { OPT_LEVELS_ALL, OPT_fsplit_wide_types_early, NULL, 1 }, > /* Enable -fsched-pressure for first pass instruction scheduling. */ > @@ -42,7 +44,6 @@ static const struct default_options rs6000_option_optimization_table[] = > /* -frename-registers leads to non-optimal codegen and performance > on rs6000, turn it off by default. */ > { OPT_LEVELS_ALL, OPT_frename_registers, NULL, 0 }, > - Don't remove random lines please. Never do this as part of unrelated patches, too. > +/* Identify instruction AND with identical zero extension. */ What does that mean? > + > +static unsigned int > +insn_is_zext_p(rtx insn) Space before (. If it is an insn, you probably want rtx_insn? > +{ > + if (GET_CODE (insn) == AND) > + { Wrong indentation. > + rtx set = XEXP (insn, 0); > + if (REG_P(set)) > + { > + if (CONST_INT_P (XEXP (insn, 1)) You can see that here: all indentations are a multiple of two, so a single space indent is always wrong. > + && INTVAL (XEXP (insn, 1)) == 1) This easily fits on the previous line. > + return 1; > + } > + else > + return 0; > + } > + > + return 0; > +} Don't "else return 0" if you have a catch-all anyway? > +/* Identify instruction AND with identical zero extension. */ > + > +static unsigned int > +insn_is_zext_p(rtx_insn * insn) No space after * for a pointer. > +{ > + rtx body = PATTERN (insn); > + > + if (GET_CODE (body) == PARALLEL) return 0; Always a new line before an "if" body. > + if (GET_CODE(body) == SET && GET_CODE (SET_SRC (body)) == AND) > + { > + rtx set = XEXP (SET_SRC(body), 0); > + > + if (REG_P(set) && GET_MODE(SET_DEST(body)) > + == GET_MODE(set)) What is this for? > + { > + if (CONST_INT_P (XEXP (SET_SRC (body), 1)) > + && INTVAL (XEXP (SET_SRC (body), 1)) == 1) The && should align with the previous "C". You could just say if (XEXP (SET_SRC (body), 1) == const1_rtx) . > + if (insn_is_zext_p(cand->insn) > + && CONST_INT_P (orig_src) && INTVAL (orig_src) != 0) > + return false; if (insn_is_zext_p (cand->insn) && orig_src != const0_rtx) return false; So what is this patch doing? At least two things, right? It aims to improve what REE does, and also enables it by default for rs6000? So make it two patches, please. Can you talk a bit about what improvements to generated code you see? Segher