From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf1-x436.google.com (mail-pf1-x436.google.com [IPv6:2607:f8b0:4864:20::436]) by sourceware.org (Postfix) with ESMTPS id 0A4663858D33 for ; Mon, 12 Jun 2023 07:32:40 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 0A4663858D33 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=vrull.eu Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=vrull.eu Received: by mail-pf1-x436.google.com with SMTP id d2e1a72fcca58-655d1fc8ad8so3280418b3a.1 for ; Mon, 12 Jun 2023 00:32:39 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=vrull.eu; s=google; t=1686555159; x=1689147159; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=bdjRGrswDfLV0z3txtLgAfW8Ga9dFSA0mGE5Git4Sx0=; b=fpd2yjy9eMIQ+PHEzgdP3rJX3AnDaGIwcopaKndBUzNeHlzNJ7OOqTAKCM3EBJsWip kt1zb/RCxEo7P9v2rMRoiPGz3cSla5Bbh573Vh4XsAKEdMW+q+8WDKgSSl+1Io9PfuZN AZ4lt3qBRW2DZFqP0oPn9TDS/od6wQ9AsuHHPZEydSP/cDuVJoJk6exgYPV1ksYYdaV7 Qu9i3l3Ee9rtUWWUGJ9VI2TrGRukyNaGKyeiULNBFE0b9KOyKlFzad6Zs/IvdQcOpsBn 7zeh/r9M+Uy1/9Soqrjf5VqIWr4/jL18MQLEfSGNl0CWyLOligpb9XdUHMEPr/WChvU6 icXA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686555159; x=1689147159; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=bdjRGrswDfLV0z3txtLgAfW8Ga9dFSA0mGE5Git4Sx0=; b=d918cj286BGKVrnNPpvQ55Nx9XaVkwh2q6yIysY83qpjxHVJMXZ6ZsefRyOpIrx/EQ PWuJc7yNjjK+kZ9G/+xWgzgSs9ta5JuY8UMDRvyRCfnJedskJzEHXuqg8UR4CJvmo0RT jX7NupwXguVoQ8NKqQMWKVfBUplXCPpLQzK1T3FoM7XLJPMR31EUXZvYjWdXPWlHqW67 7gqZ58LX2ErCyIEiZ1oRd6NxN2T9XVq/8SUaX8Tl6VblUQHDxup70APAmAONY6c7D/uN 6y0VNpcxW+Egr3n/llCkug4JuI7rFzTjf3jsRl6+8xELpiGGvGLrRAiIT1alzH+Q1NBd qPEA== X-Gm-Message-State: AC+VfDzgkT0XH0yuOOGqks5LNyt+phspWJ0FT60I6GNuGSKftIf3dops WRugqihsXWd0ekGto6o5qThQs25VcI2KsyVRTkCdSA== X-Google-Smtp-Source: ACHHUZ6VBGS5KF8mf41gqZk8mMlG66nhO0F9fDcwXDrXzFb1IqwSTaKqJDXRHSnq2NH0ow0WDJ3g6mijzCO5prft3Oo= X-Received: by 2002:a17:90a:3fc7:b0:25b:c19c:925b with SMTP id u7-20020a17090a3fc700b0025bc19c925bmr4256200pjm.39.1686555158569; Mon, 12 Jun 2023 00:32:38 -0700 (PDT) MIME-Version: 1.0 References: <20230525123550.1072506-1-manolis.tsamis@vrull.eu> <20230525123550.1072506-2-manolis.tsamis@vrull.eu> <0b919289-8b02-1c1a-3d07-0cac3d22cd87@gmail.com> In-Reply-To: <0b919289-8b02-1c1a-3d07-0cac3d22cd87@gmail.com> From: Manolis Tsamis Date: Mon, 12 Jun 2023 10:32:02 +0300 Message-ID: Subject: Re: [PATCH 1/2] Implementation of new RISCV optimizations pass: fold-mem-offsets. To: Jeff Law Cc: gcc-patches@gcc.gnu.org, Richard Biener , Palmer Dabbelt , Philipp Tomsich , Kito Cheng Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-3.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,JMQ_SPF_NEUTRAL,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE 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: On Fri, Jun 9, 2023 at 3:57=E2=80=AFAM Jeff Law wro= te: > > > > On 5/25/23 06:35, Manolis Tsamis wrote: > > Implementation of the new RISC-V optimization pass for memory offset > > calculations, documentation and testcases. > > > > gcc/ChangeLog: > > > > * config.gcc: Add riscv-fold-mem-offsets.o to extra_objs. > > * config/riscv/riscv-passes.def (INSERT_PASS_AFTER): Schedule a n= ew > > pass. > > * config/riscv/riscv-protos.h (make_pass_fold_mem_offsets): Decla= re. > > * config/riscv/riscv.opt: New options. > > * config/riscv/t-riscv: New build rule. > > * doc/invoke.texi: Document new option. > > * config/riscv/riscv-fold-mem-offsets.cc: New file. > > > > gcc/testsuite/ChangeLog: > > > > * gcc.target/riscv/fold-mem-offsets-1.c: New test. > > * gcc.target/riscv/fold-mem-offsets-2.c: New test. > > * gcc.target/riscv/fold-mem-offsets-3.c: New test. > So a followup. > > While I think we probably could create a variety of backend patterns, > perhaps disallow the frame pointer as the addend argument to a shadd > pattern and the like and capture the key cases from mcf and probably > deepsjeng it's probably not the best direction. > > What I suspect would ultimately happen is we'd be presented with > additional cases over time that would require an ever increasing number > of patterns. sign vs zero extension, increasing depth of search space > to find reassociation opportunities, different variants with and without > shadd/zbb, etc etc. > > So with that space explored a bit the next big question is target > specific or generic. I'll poke in there a it over the coming days. In > the mean time I do have some questions/comments on the code itself. > There may be more over time.. > > > > > +static rtx_insn* > > +get_single_def_in_bb (rtx_insn *insn, rtx reg) > [ ... ] > > > > + for (ref_link =3D ref_chain; ref_link; ref_link =3D ref_link->next) > > + { > > + /* Problem getting some definition for this instruction. */ > > + if (ref_link->ref =3D=3D NULL) > > + return NULL; > > + if (DF_REF_INSN_INFO (ref_link->ref) =3D=3D NULL) > > + return NULL; > > + if (global_regs[REGNO (reg)] > > + && !set_of (reg, DF_REF_INSN (ref_link->ref))) > > + return NULL; > > + } > That last condition feels a bit odd. It would seem that you wanted an > OR boolean rather than AND. > Most of this function I didn't write by myself, I used existing code to get definitions taken from REE's get_defs. In the original there's a comment about this line this comment that explain= s it: 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. This function is the only one I copied without changing much (because I didn't quite understand it), so I don't know if that condition is any useful for f-m-o. Also the code duplication here is a bit unfortunate, maybe it would be preferred to create a generic version that can be used in both? > > > + > > + unsigned int dest_regno =3D REGNO (dest); > > + > > + /* We don't want to fold offsets from instructions that change some > > + particular registers with potentially global side effects. */ > > + if (!GP_REG_P (dest_regno) > > + || dest_regno =3D=3D STACK_POINTER_REGNUM > > + || (frame_pointer_needed && dest_regno =3D=3D HARD_FRAME_POINTER= _REGNUM) > > + || dest_regno =3D=3D GP_REGNUM > > + || dest_regno =3D=3D THREAD_POINTER_REGNUM > > + || dest_regno =3D=3D RETURN_ADDR_REGNUM) > > + return 0; > I'd think most of this would be captured by testing fixed_registers > rather than trying to list each register individually. In fact, if we > need to generalize this to work on other targets we almost certainly > want a more general test. > Thanks, I knew there would be some proper way to test this but wasn't aware which is the correct one. Should this look like below? Or is the GP_REG_P redundant and just fixed_regs will do? if (!GP_REG_P (dest_regno) || fixed_regs[dest_regno]) return 0; > > > + else if (( > > + GET_CODE (src) =3D=3D SIGN_EXTEND > > + || GET_CODE (src) =3D=3D ZERO_EXTEND > > + ) > > + && MEM_P (XEXP (src, 0))) > Formatting is goofy above... > Noted. > > > > + > > + if (dump_file) > > + { > > + fprintf (dump_file, "Instruction deleted from folding:"); > > + print_rtl_single (dump_file, insn); > > + } > > + > > + if (REGNO (dest) !=3D REGNO (arg1)) > > + { > > + /* If the dest register is different than the fisrt argumen= t > > + then the addition with constant 0 is equivalent to a mov= e > > + instruction. We emit the move and let the subsequent > > + pass cprop_hardreg eliminate that if possible. */ > > + rtx arg1_reg_rtx =3D gen_rtx_REG (GET_MODE (dest), REGNO (a= rg1)); > > + rtx mov_rtx =3D gen_move_insn (dest, arg1_reg_rtx); > > + df_insn_rescan (emit_insn_after (mov_rtx, insn)); > > + } > > + > > + /* If the dest register is the same with the first argument > > + then the addition with constant 0 is a no-op. > > + We can now delete the original add immidiate instruction. *= / > > + delete_insn (insn); > The debugging message is a bit misleading. Yea, we always delete > something here, but in one case we end up emitting a copy. > Indeed. Maybe "Instruction reduced to move: ..."? > > > > + > > + /* Temporarily change the offset in MEM to test whether > > + it results in a valid instruction. */ > > + machine_mode mode =3D GET_MODE (mem_addr); > > + XEXP (mem, 0) =3D gen_rtx_PLUS (mode, reg, GEN_INT (offset)); > > + > > + bool valid_change =3D recog (PATTERN (insn), insn, 0) >=3D 0; > > + > > + /* Restore the instruction. */ > > + XEXP (mem, 0) =3D mem_addr; > You need to reset the INSN_CODE after restoring the instruction. That's > generally a bad thing to do, but I've seen it done enough (and been > guilty myself in the past) that we should just assume some ports are > broken in this regard. > Ok thanks, I didn't knew that one. > > Anyway, just wanted to get those issues raised so that you can address th= em. > > jeff