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

Reply via email to