From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ed1-x52c.google.com (mail-ed1-x52c.google.com [IPv6:2a00:1450:4864:20::52c]) by sourceware.org (Postfix) with ESMTPS id D8E773858CDA for ; Thu, 30 Mar 2023 14:28:56 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org D8E773858CDA Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-ed1-x52c.google.com with SMTP id eg48so77160818edb.13 for ; Thu, 30 Mar 2023 07:28:56 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; t=1680186535; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:from:to:cc:subject:date :message-id:reply-to; bh=+mqYCV3tdm7GENxuneIzT4jGs/j/asqOYgxwhPkfE/4=; b=LoOKeyaV29SZGsOZotoSjE0HHBsC8ve9sBm60p2EBypHQ3yNRXk09m0Keae63NBaIW K0Di0bAP/oNauxlCGJ2MiS0NEXOA1aBZj9eCqvIcywqpyeZUQutB4qUIWgYzHd78CMA8 aQ6sEO2WoB8stMw1ytpJySaUmRK3Plt89PYjSMq8GWVo/207lSQw+Bt2zPI6vZ3B+6A6 c9M955Fhecbehy2eUpzO+FNrNtWerXxhrw6R9V9Cp+lHE7NoEZPbjBxoe08Za9Yk6MvD u6Hd1DLHlzxyb46FuDQN1t9PHP6LmvyPinKo3id9v+MMLLNJoWvknksDcxX64vuh+Zxz j+iQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1680186535; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=+mqYCV3tdm7GENxuneIzT4jGs/j/asqOYgxwhPkfE/4=; b=0CMozgnL3HTOhrWJFjWjRBjeS4rCXY4YuHox7RPH/athk92ULH0TGcVeHlUM6e3oOj CemdRW6zp0tSVXshuMAGUjAwHDwjDCdyynPrr1U939+3BrMfUs/l9GEBokwCJnGNCzHC L+h78sk+WoqgMOXg83vgDtQCn7vt2pcOIxUeIGAAW4Uo6q3Q0nUm/+Np6zwxPJskB1y+ X+lESXdPt3eo8eXGe7m5fHrJkIPzSljGhs7XTZRFJGVjkwZ3fSY9lnGs/aaz3xzrJF15 rqe5ZYcZITGBmiAToGGVTWT1HkgXDVYpRfHvcpkHpyMcKcKLDrAAhETaVaOh66cOc0LY 5J9Q== X-Gm-Message-State: AAQBX9fvTb6z16fyfuO0yGoV0M2wucOmyan/+Vbn/TwtlwzwPz0KySl1 4BkRk1v17mKj4JtOqF8YzJOR+KZjmDk= X-Google-Smtp-Source: AKy350YviRsrDl3Ht8Ad0uzJ+CXk8yCi1hsWZUCpx0xlxpxtFunLeBa9vCi0387ZX/YxsttL1AxWwg== X-Received: by 2002:aa7:c042:0:b0:4fe:38b1:e405 with SMTP id k2-20020aa7c042000000b004fe38b1e405mr20819185edo.34.1680186535336; Thu, 30 Mar 2023 07:28:55 -0700 (PDT) Received: from nbbrfq (80-110-214-113.static.upcbusiness.at. [80.110.214.113]) by smtp.gmail.com with ESMTPSA id z23-20020a50cd17000000b004bd1fe2cc02sm18302237edi.16.2023.03.30.07.28.54 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 30 Mar 2023 07:28:55 -0700 (PDT) Date: Thu, 30 Mar 2023 16:28:51 +0200 From: Bernhard Reutner-Fischer To: Ajit Agarwal via Gcc-patches Cc: rep.dot.nop@gmail.com, Ajit Agarwal , Segher Boessenkool , Peter Bergner , Richard Biener , Jeff Law Subject: Re: [PATCH v2] rtl-optimization: ppc backend generates unnecessary extension. Message-ID: <20230330162851.6ac61e1f@nbbrfq> In-Reply-To: <05b5dcba-9343-0b6c-44a7-d4a6d128e7aa@linux.ibm.com> References: <05b5dcba-9343-0b6c-44a7-d4a6d128e7aa@linux.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-9.6 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP 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 Thu, 30 Mar 2023 17:30:43 +0530 Ajit Agarwal via Gcc-patches wrote: > Hello All: Just some nits. And this seems to be an incremental diff ontop of your v2. You might want check for coding-style errors by running: contrib/check_GNU_style.py /tmp/ree-v2bis.patch > > This patch added enhancement to extimination elimination for rs6000 target. I think REE means redundant extension elimination. > gcc/ChangeLog: > > * ree.cc(is_feasible_elim_across_basic_blocks): We often use the lispy _p suffix for predicates. Maybe eliminate_across_bbs_p would be shorter. > Add checks to enable extension elimination.. > * ree.cc(combine_reaching_defs): Add checks to enable > extension elimination. > --- > gcc/ree.cc | 121 ++++++++++++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 116 insertions(+), 5 deletions(-) > > diff --git a/gcc/ree.cc b/gcc/ree.cc > index d05d37f9a23..7e4cce5cee4 100644 > --- a/gcc/ree.cc > +++ b/gcc/ree.cc > @@ -253,6 +253,56 @@ struct ext_cand > > static int max_insn_uid; > > +/* Get all the reaching definitions of an instruction. The definitions are > + desired for REG used in INSN. Return the definition list or NULL if a > + definition is missing. If DEST is non-NULL, additionally push the INSN > + of the definitions onto DEST. */ > + > +static struct df_link * > +get_defs (rtx_insn *insn, rtx reg, vec *dest) > +{ > + df_ref use; > + struct df_link *ref_chain, *ref_link; > + > + FOR_EACH_INSN_USE (use, insn) > + { > + if (GET_CODE (DF_REF_REG (use)) == SUBREG) > + return NULL; > + if (REGNO (DF_REF_REG (use)) == REGNO (reg)) > + break; > + } > + > + if (use == NULL) return NULL; Missing newline before return. > + > + gcc_assert (use != NULL); Either the assert or the if but both is a bit much. > + > + ref_chain = DF_REF_CHAIN (use); > + > + for (ref_link = ref_chain; ref_link; ref_link = ref_link->next) > + { > + /* Problem getting some definition for this instruction. */ > + if (ref_link->ref == NULL) > + return NULL; > + if (DF_REF_INSN_INFO (ref_link->ref) == NULL) > + return NULL; > + /* As global regs are assumed to be defined at each function call > + dataflow can report a call_insn as being a definition of REG. > + But we can't do anything with that in this pass so proceed only > + if the instruction really sets REG in a way that can be deduced > + from the RTL structure. */ > + if (global_regs[REGNO (reg)] > + && !set_of (reg, DF_REF_INSN (ref_link->ref))) > + return NULL; > + } > + > + if (dest) > + for (ref_link = ref_chain; ref_link; ref_link = ref_link->next) > + dest->safe_push (DF_REF_INSN (ref_link->ref)); > + > + return ref_chain; > +} > + > + > /* Identify instruction AND with identical zero extension. */ > > static unsigned int > @@ -393,6 +443,7 @@ combine_set_extension (ext_cand *cand, rtx_insn *curr_insn, rtx *orig_set) > machine_mode orig_mode = GET_MODE (SET_DEST (*orig_set)); > rtx new_set = NULL_RTX; > rtx cand_pat = single_set (cand->insn); > + superfluous whitespace change. > if (insn_is_zext_p(cand->insn) > && CONST_INT_P (orig_src) && INTVAL (orig_src) != 0) > return false; > @@ -401,6 +452,7 @@ combine_set_extension (ext_cand *cand, rtx_insn *curr_insn, rtx *orig_set) > then we need to change the original load to reference the destination > of the extension. Then we need to emit a copy from that destination > to the original destination of the load. */ > + // print_rtl_single(stdout, cand_pat); Please remove that debug comment. > rtx new_reg; > bool copy_needed > = (REGNO (SET_DEST (cand_pat)) != REGNO (XEXP (SET_SRC (cand_pat), 0))); > @@ -547,6 +599,7 @@ transform_ifelse (ext_cand *cand, rtx_insn *def_insn) > return false; > } > > +#if 0 > /* Get all the reaching definitions of an instruction. The definitions are > desired for REG used in INSN. Return the definition list or NULL if a > definition is missing. If DEST is non-NULL, additionally push the INSN > @@ -593,7 +646,7 @@ get_defs (rtx_insn *insn, rtx reg, vec *dest) > > return ref_chain; > } > - > +#endif Why did you move get_defs? > /* Get all the reaching uses of an instruction. The uses are desired for REG > set in INSN. Return use list or NULL if a use is missing or irregular. */ > > @@ -848,6 +901,36 @@ is_feasible_elim_across_basic_blocks (ext_cand *cand, > && REGNO (XEXP (PATTERN (BB_END (bb)), 0)) != REGNO (SET_DEST (cand->expr))) > return false; > > + if (cand->code == ZERO_EXTEND > + && GET_CODE ((PATTERN (def_insn))) == PARALLEL) > + return false; Excess braces above. > + > + if (cand->code == ZERO_EXTEND > + && GET_CODE ((PATTERN (def_insn))) == SET Excess braces above. > + && GET_CODE (SET_SRC (PATTERN (def_insn))) != XOR) > + return false; > + > + if (cand->code == ZERO_EXTEND > + && GET_CODE ((PATTERN (def_insn))) == SET > + && GET_CODE (SET_SRC (PATTERN (def_insn))) == XOR) > + { > + vec *dest = XCNEWVEC (vec, 4); > + if (!get_defs (def_insn, XEXP (SET_SRC (PATTERN(def_insn)), 0), dest)) > + return false; missing space between PATTERN and the brace This leaks dest. The above looks a bit redundant. I'd have a single test for cand->code == ZERO_EXTEND and then rtx def = PATTERN (def_insn); if (def == PARALLEL) return false; if (def == SET) { rtx src = SET_SRC (def); if (GET_CODE (src) != XOR) return false; /* maybe use an auto_vec here */ ... > + > + int i; > + rtx_insn *def_insn; > + > + FOR_EACH_VEC_ELT (*dest, i, def_insn) > + { > + if ((GET_CODE (PATTERN (def_insn)) == SET > + && GET_CODE (SET_SRC (PATTERN (def_insn))) == ASHIFT) > + || GET_CODE (PATTERN (def_insn)) == PARALLEL) leaks dest. > + return false; > + } > + XDELETEVEC (dest); > + } > + > return true; > } > > @@ -873,7 +956,8 @@ merge_def_and_ext (ext_cand *cand, rtx_insn *def_insn, ext_state *state) > > if (!feasible) return false; > > - if (((!copy_needed && (insn_is_zext_p (cand->insn)) > + if (((!copy_needed && (insn_is_zext_p (cand->insn) > + || (cand->code == ZERO_EXTEND && ext_src_mode == QImode)) > && (GET_MODE (SET_DEST (*sub_rtx)) != ext_src_mode > && state->modified[INSN_UID (def_insn)].kind == EXT_MODIFIED_NONE)) > || ((state->modified[INSN_UID (def_insn)].kind > @@ -1211,15 +1295,22 @@ combine_reaching_defs (ext_cand *cand, const_rtx set_pat, ext_state *state) > definitions could be merged. */ > if (apply_change_group ()) > { > - if (dump_file) > - fprintf (dump_file, "All merges were successful.\n"); > + if (state->modified_list.length() == 0) return false; Missing space after length before the opening brace, and missing newline before return. > + > + if (cand->code == ZERO_EXTEND > + && GET_CODE (PATTERN (state->modified_list[0])) == SET > + && GET_CODE (SET_SRC (PATTERN (state->modified_list[0]))) != XOR) > + return false; > + > + if (dump_file) > + fprintf (dump_file, "All merges were successful.\n"); > > FOR_EACH_VEC_ELT (state->modified_list, i, def_insn) > { > ext_modified *modified = &state->modified[INSN_UID (def_insn)]; > if (modified->kind == EXT_MODIFIED_NONE) > modified->kind = (cand->code == ZERO_EXTEND ? EXT_MODIFIED_ZEXT > - : EXT_MODIFIED_SEXT); > + : EXT_MODIFIED_SEXT); > > if (copy_needed) > modified->do_not_reextend = 1; > @@ -1228,6 +1319,26 @@ combine_reaching_defs (ext_cand *cand, const_rtx set_pat, ext_state *state) > } > else > { > + if (state->modified_list.length() == 0) return false; same comment as above > + > + if (cand->code == ZERO_EXTEND > + && GET_CODE (PATTERN(state->modified_list[0])) == SET > + && GET_CODE (SET_SRC (PATTERN (state->modified_list[0]))) != XOR) > + return false; Missing space before '(' > + > + if (cand->code == ZERO_EXTEND) > + { > + FOR_EACH_VEC_ELT (state->modified_list, i, def_insn) > + { > + ext_modified *modified = &state->modified[INSN_UID (def_insn)]; > + if (modified->kind == EXT_MODIFIED_NONE) > + modified->kind = (cand->code == ZERO_EXTEND ? EXT_MODIFIED_ZEXT > + : EXT_MODIFIED_SEXT); The whole loop is guarded by code == ZERO_EXTEND, so the rhs condition above is redundant. thanks, > + > + modified->do_not_reextend = 1; > + } > + return true; > + } > /* Changes need not be cancelled explicitly as apply_change_group > does it. Print list of definitions in the dump_file for debug > purposes. This extension cannot be deleted. */