From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf1-x430.google.com (mail-pf1-x430.google.com [IPv6:2607:f8b0:4864:20::430]) by sourceware.org (Postfix) with ESMTPS id 834963858C20 for ; Fri, 9 Jun 2023 00:57:29 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 834963858C20 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-pf1-x430.google.com with SMTP id d2e1a72fcca58-654f8b56807so1235959b3a.1 for ; Thu, 08 Jun 2023 17:57:29 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1686272248; x=1688864248; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=W2XNBjc9QErMKhz69Q4ozyCXMZ45GQjUL1swSou5EmE=; b=Z9uRGWVHWmkiFWbflcTGM7ZPq9uSn4eB+hG7koH9uJcPdTjcuoaPUVLqdFtOYvzZqF KXWxcJA8kp0HZX2JBrIZxo8WSqKA/CelzVp27+p3JCmpCf2SFIGv5zcAEHhUU0UH6PeJ Pqt+Tqx1efFnloS+LMGlfwJD7yhPDQrCJqStiVZQEbYqF3611PdZSj6Aal30ZZ+nG9SX Jtx9iz+Ev3ho+uGc+rKGl7ZYAcsT2ShD0s3y1MwfkPEUVCDPDjHYHm2Xx4bO7F40FKZc LmL9A2YqRUqKe46mmoVpohNffLHdAPfddqjW1PCc44Mc/PNaRnG2lzXA1jEPVzeI7/ye fblQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686272248; x=1688864248; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=W2XNBjc9QErMKhz69Q4ozyCXMZ45GQjUL1swSou5EmE=; b=DpTh32Xy02Cke4haAm449wI8jkyX0E1WHfp4jPc71rWG3c+b4wDtlAuhKDcXC5+f3e mtUvnoDBKGeIzwzKLCoC8ZO9PIH2SCmv6cbNZPgzuiNcjYvvqWK+OLLJrh4Hf/F5XK2t wMMsswMCuegr47uA3eQ00LdJsSaemVwWOB9mEN8j1xbpRkmKHRmNjLNik93AaIPMB+CT viZdylfIqZAKdBRvbS2L9Z0Nj7IWKR02cKzlyH3t4XiV52S3mZob+rX0Y/pRtypySzxa SJYMVzz4ZNQulpdMPNmRobU6jsbHuKwX89AkuNJjUt5lfXzjqEqv+OlgmZJOQv+uR+jx ZM3w== X-Gm-Message-State: AC+VfDxXqOpUUrAxorOEIFT9p49HMEvdQB7lnd3DioJDamn77gwVNR0x 0Xr6f3CW2KTcSfumA2j6C7Q= X-Google-Smtp-Source: ACHHUZ7Bj5qPKZ7P84gbv7VHV9AjqM6ByHN9cUXX7qvVhjjJeY8gbVy7mHPpnWkMqWgzlXOLTkQTUQ== X-Received: by 2002:a05:6a21:3391:b0:117:1ffb:a14 with SMTP id yy17-20020a056a21339100b001171ffb0a14mr10190836pzb.13.1686272248140; Thu, 08 Jun 2023 17:57:28 -0700 (PDT) Received: from [172.31.0.109] ([136.36.130.248]) by smtp.gmail.com with ESMTPSA id j11-20020a170902690b00b001b02bd00c61sm1968200plk.237.2023.06.08.17.57.26 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 08 Jun 2023 17:57:27 -0700 (PDT) Message-ID: <0b919289-8b02-1c1a-3d07-0cac3d22cd87@gmail.com> Date: Thu, 8 Jun 2023 18:57:25 -0600 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.9.1 Subject: Re: [PATCH 1/2] Implementation of new RISCV optimizations pass: fold-mem-offsets. Content-Language: en-US To: Manolis Tsamis , gcc-patches@gcc.gnu.org Cc: Richard Biener , Palmer Dabbelt , Philipp Tomsich , Kito Cheng References: <20230525123550.1072506-1-manolis.tsamis@vrull.eu> <20230525123550.1072506-2-manolis.tsamis@vrull.eu> From: Jeff Law In-Reply-To: <20230525123550.1072506-2-manolis.tsamis@vrull.eu> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-2.3 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,KAM_SHORT,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,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 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 new > pass. > * config/riscv/riscv-protos.h (make_pass_fold_mem_offsets): Declare. > * 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 = 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; > + 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. > + > + unsigned int dest_regno = 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 == STACK_POINTER_REGNUM > + || (frame_pointer_needed && dest_regno == HARD_FRAME_POINTER_REGNUM) > + || dest_regno == GP_REGNUM > + || dest_regno == THREAD_POINTER_REGNUM > + || dest_regno == 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. > + else if (( > + GET_CODE (src) == SIGN_EXTEND > + || GET_CODE (src) == ZERO_EXTEND > + ) > + && MEM_P (XEXP (src, 0))) Formatting is goofy above... > + > + if (dump_file) > + { > + fprintf (dump_file, "Instruction deleted from folding:"); > + print_rtl_single (dump_file, insn); > + } > + > + if (REGNO (dest) != REGNO (arg1)) > + { > + /* If the dest register is different than the fisrt argument > + then the addition with constant 0 is equivalent to a move > + instruction. We emit the move and let the subsequent > + pass cprop_hardreg eliminate that if possible. */ > + rtx arg1_reg_rtx = gen_rtx_REG (GET_MODE (dest), REGNO (arg1)); > + rtx mov_rtx = 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. > + > + /* Temporarily change the offset in MEM to test whether > + it results in a valid instruction. */ > + machine_mode mode = GET_MODE (mem_addr); > + XEXP (mem, 0) = gen_rtx_PLUS (mode, reg, GEN_INT (offset)); > + > + bool valid_change = recog (PATTERN (insn), insn, 0) >= 0; > + > + /* Restore the instruction. */ > + XEXP (mem, 0) = 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. Anyway, just wanted to get those issues raised so that you can address them. jeff