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
