From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi1-x22b.google.com (mail-oi1-x22b.google.com [IPv6:2607:f8b0:4864:20::22b]) by sourceware.org (Postfix) with ESMTPS id A6E383857342 for ; Thu, 15 Jun 2023 17:35:28 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org A6E383857342 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-oi1-x22b.google.com with SMTP id 5614622812f47-39cc64e4a44so3110377b6e.0 for ; Thu, 15 Jun 2023 10:35:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=vrull.eu; s=google; t=1686850528; x=1689442528; 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=3AvscaEYCtIQoaXPG63QKk+np6wq1KELoA1YDWVMdzw=; b=VFYjCQXEVdT0zqnirxxoMKF8aevhMSABXIyrVW5qRIj7Gp/+pVtyhb1jRO+mPPFRlj 5lOTuG/GH4Qn68UcYnlJA8ND5bUI5tj9+DjXymC65+5d4rYf86WVT4xRvJHBCx9VGev1 /uBiGUInqPRwhamRsS/41JUD6uhf4GSzVKCOWGvX1OExjxXh43Qixkz96jbJnOeIxUur VgUTH+QHzv9Fo9hqbPo/ENuFcWUSYKQMV58lYhz2rNyI63pLWbx87B3vh8Q7j9mjYQf2 q6C6Pone6Rso2/Nw4oBtDAxb6LFm52YQNe5oXLUlLqB7uHjbhch0UH3Cy6m10/TujRkV 90iw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686850528; x=1689442528; 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=3AvscaEYCtIQoaXPG63QKk+np6wq1KELoA1YDWVMdzw=; b=ZrSzZeB3QM8pd1LgyUZhWa1J12eqmB0RcX9RPsa4QNqxWS0PECECEJWXz7NsB/gT3P KhDyQmgbD0I4tuAkF12lMqwKYKhsgxgLgHybBxd87R8apI/RFwhAepmbLK7OqEdjltNV UTc55ywQEykjUtON0YNBF6Mf1y9374zJblFPnET847FMLnOgVKfnYH9BjLCnUItSp4rd WrY3aivKodDratbp1qeUti1bhGKBGEUBMBpBC+HSd7uWtNRVeXmOthvPVQEeJGqoG1ZM kevClGNfp+2IY18VZ4x+c1O3cIkkVyLeTeNMNcDBCj0zMRZ+5Guqo7QxbtRi83ubMiGO bsqw== X-Gm-Message-State: AC+VfDzgwwlB3UiNY+7br3SVi+bE/yqjGOuR9bErVd01mP1TqsBS+VrH aBKkaLDMAseCCjYZ7J+Pa3QfeDfwDL5hSMYQHP2lTA== X-Google-Smtp-Source: ACHHUZ4gq8aQ0e0etNW+7o1M4wKxm6GN1grrWiXoTbZQb1VeOczv8IBXHWD+Z1+yPzBD+BNBgUEkQbpSy9deINqbaf8= X-Received: by 2002:a05:6808:1808:b0:398:2483:ef4e with SMTP id bh8-20020a056808180800b003982483ef4emr14795950oib.3.1686850527736; Thu, 15 Jun 2023 10:35:27 -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: From: Manolis Tsamis Date: Thu, 15 Jun 2023 20:34:51 +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.1 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: The new target-independant implementation of fold-mem-offsets pass can be found in the list (link is https://gcc.gnu.org/pipermail/gcc-patches/2023-June/621920.html) Aside from now being target independent, I have fixed a number of new bugs that emerged when running this on other targets and a minor memory leak. I have also improved the propagation logic in fold_offsets to work with more patterns found in other targets (e.g. LEA instructions in x86). Finally I improved the naming of things (e.g. replaced uses of 'delete'/'remove' with 'fold', made bitmap names more meaningful) and reduced unnecessary verbosity in some comments. Thanks, Manolis On Tue, Jun 13, 2023 at 12:58=E2=80=AFAM Jeff Law w= rote: > > > > On 6/12/23 01:32, Manolis Tsamis wrote: > > >> > >>> + 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 exp= lains 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? > Ah. So I think the code is meant to filter out things that DF will say > are set vs those which are actually exposed explicitly in the RTL (and > which REE might be able to modify). So we're probably good. > > Those routines are are pretty close to each other in implementation. I > bet we could take everything up to the loop over the ref links and > factor that into a common function. Both your function and get_defs > would be able to use that and then do bit of processing afterwards. > > > > > >> > >>> + > >>> + unsigned int dest_regno =3D REGNO (dest); > >>> + > >>> + /* We don't want to fold offsets from instructions that change som= e > >>> + 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_POINT= ER_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 you want to verify it's a general register, then you have to ask if > the regno is in the GENERAL_REGS class. Something like: > > TEST_HARD_REG_BIT (reg_class_contents[GENERAL_REGS], dest_regno) > > GP_REG_P is a risc-v specific macro, so we can't use it here. > > So something like > if (fixed_regs[dest_regno] > || !TEST_HARD_REG_BIT (reg_class_contents[GENERAL_REGS], dest_regn= o)) > > > > >> 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: ..."? > Works for me. > > > > >> > >> > >>> + > >>> + /* 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. > It's pretty obscure and I probably would have missed it if I hadn't > debugged a problem related to this just a few months back. It shouldn't > be necessary due to rules about how the movXX patterns are supposed to > work. But I've seen it mucked up enough that it's the right thing to do. > > Essentially when you call into recog, if the pattern is recognized, then > a cookie is cached so that we know what pattern was recognized within > the backend. > > As far as generalizing to a target independent pass. You'll need to > declare the new pass in tree-pass.h, add it to passes.def, wire up the > option in common.opt, document it in invoke.texi and turn it on for -O2 > and above. WRT the interaction with shorten-memrefs, I think that can > be handled in override-options. I think we want to have shorten-memrefs > override. So if shorten-memrefs is on, then we turn off f-o-m in the RV > backend. This should probably be documented in invoke.texi as well. > > It sounds like a lot, but I think each of those is a relatively simple > change. It'd be best if you could tackle those changes. I think with > that done and bootstrap/regression test round we'd be ready to integrate > your work. > > Thanks! > > jeff