Re: [PATCH v2 2/2] sched-deps.c: Avoid replacing address if it increases address cost

[Craig Blackmore <craig.blackmore@embecosm.com>]

Hi Jim,

On 31/10/2019 00:03, Jim Wilson wrote:
> On Fri, Oct 25, 2019 at 10:40 AM Craig Blackmore
> <craig.blackmore@embecosm.com> wrote:
>> The sched2 pass undoes some of the addresses generated by the RISC-V
>> shorten_memrefs code size optimization (patch 1/2) and consequently increases
>> code size. This patch prevents sched-deps.c from changing an address if it is
>> expected to increase address cost.
>
> This should be rewritten as a target hook, and then we can define the
> hook to do what we want for RISC-V.  It isn't OK to make this change
> for other targets without testing them.  If the default hook does
> nothing, then other targets won't be affected.
>

Thanks for the review, here is an updated patch rewriting this as a target hook.

Thanks,
Craig

---
gcc/ChangeLog:

	* config/riscv/riscv.c (riscv_new_address_profitable_p): New function.
	(TARGET_NEW_ADDRESS_PROFITABLE_P): Define.
	* doc/tm.texi: Regenerate.
	* doc/tm.texi.in (TARGET_NEW_ADDRESS_PROFITABLE_P):  New hook.
	* sched-deps.c (attempt_change): Use old address if it is cheaper than
	new address.
	* target.def (new_address_profitable_p): New hook.
	* targhooks.c (default_new_address_profitable_p): New function.
	* targhooks.h (default_new_address_profitable_p): Declare.
---
 gcc/config/riscv/riscv.c | 16 ++++++++++++++++
 gcc/doc/tm.texi          |  7 +++++++
 gcc/doc/tm.texi.in       |  2 ++
 gcc/sched-deps.c         |  3 +++
 gcc/target.def           | 11 +++++++++++
 gcc/targhooks.c          | 13 +++++++++++++
 gcc/targhooks.h          |  1 +
 7 files changed, 53 insertions(+)

diff --git a/gcc/config/riscv/riscv.c b/gcc/config/riscv/riscv.c
index 1d6d2f89f7d..a508894d16e 100644
--- a/gcc/config/riscv/riscv.c
+++ b/gcc/config/riscv/riscv.c
@@ -5095,6 +5095,19 @@ riscv_reorg (void)
     riscv_remove_unneeded_save_restore_calls ();
 }
 
+/* Implement TARGET_NEW_ADDRESS_PROFITABLE_P.  */
+
+bool
+riscv_new_address_profitable_p (rtx memref, rtx_insn *insn, rtx new_addr)
+{
+  /* Prefer old address if it is less expensive.  */
+  addr_space_t as = MEM_ADDR_SPACE (memref);
+  bool speed = optimize_bb_for_speed_p (BLOCK_FOR_INSN (insn));
+  int old_cost = address_cost (XEXP (memref, 0), GET_MODE (memref), as, speed);
+  int new_cost = address_cost (new_addr, GET_MODE (memref), as, speed);
+  return new_cost <= old_cost;
+}
+
 /* Initialize the GCC target structure.  */
 #undef TARGET_ASM_ALIGNED_HI_OP
 #define TARGET_ASM_ALIGNED_HI_OP "\t.half\t"
@@ -5272,6 +5285,9 @@ riscv_reorg (void)
 #undef TARGET_MACHINE_DEPENDENT_REORG
 #define TARGET_MACHINE_DEPENDENT_REORG riscv_reorg
 
+#undef TARGET_NEW_ADDRESS_PROFITABLE_P
+#define TARGET_NEW_ADDRESS_PROFITABLE_P riscv_new_address_profitable_p
+
 struct gcc_target targetm = TARGET_INITIALIZER;
 
 #include "gt-riscv.h"
diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
index 5b8b68bd710..85470c8afa0 100644
--- a/gcc/doc/tm.texi
+++ b/gcc/doc/tm.texi
@@ -6944,6 +6944,13 @@ candidate as a replacement for the if-convertible sequence described in
 @code{if_info}.
 @end deftypefn
 
+@deftypefn {Target Hook} bool TARGET_NEW_ADDRESS_PROFITABLE_P (rtx @var{memref}, rtx_insn * @var{insn}, rtx @var{new_addr})
+Return @code{true} if it is profitable to replace the address in
+@var{memref} with @var{new_addr}.  This allows targets to prevent the
+scheduler from undoing address optimizations.  The instruction containing the
+memref is @var{insn}.  The default implementation returns @code{true}.
+@end deftypefn
+
 @deftypefn {Target Hook} bool TARGET_NO_SPECULATION_IN_DELAY_SLOTS_P (void)
 This predicate controls the use of the eager delay slot filler to disallow
 speculatively executed instructions being placed in delay slots.  Targets
diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
index 1b061d70127..b5c80ce2fdc 100644
--- a/gcc/doc/tm.texi.in
+++ b/gcc/doc/tm.texi.in
@@ -4695,6 +4695,8 @@ Define this macro if a non-short-circuit operation produced by
 
 @hook TARGET_NOCE_CONVERSION_PROFITABLE_P
 
+@hook TARGET_NEW_ADDRESS_PROFITABLE_P
+
 @hook TARGET_NO_SPECULATION_IN_DELAY_SLOTS_P
 
 @hook TARGET_ESTIMATED_POLY_VALUE
diff --git a/gcc/sched-deps.c b/gcc/sched-deps.c
index a9e73fc6044..8f56594b745 100644
--- a/gcc/sched-deps.c
+++ b/gcc/sched-deps.c
@@ -4693,6 +4693,9 @@ attempt_change (struct mem_inc_info *mii, rtx new_addr)
   rtx mem = *mii->mem_loc;
   rtx new_mem;
 
+  if (!targetm.new_address_profitable_p (mem, mii->mem_insn, new_addr))
+    return NULL_RTX;
+
   /* Jump through a lot of hoops to keep the attributes up to date.  We
      do not want to call one of the change address variants that take
      an offset even though we know the offset in many cases.  These
diff --git a/gcc/target.def b/gcc/target.def
index e0e856979a9..81c78d8ffea 100644
--- a/gcc/target.def
+++ b/gcc/target.def
@@ -3824,6 +3824,17 @@ candidate as a replacement for the if-convertible sequence described in\n\
 bool, (rtx_insn *seq, struct noce_if_info *if_info),
 default_noce_conversion_profitable_p)
 
+/* Return true if new_addr should be preferred over the existing address used by
+   memref in insn.  */
+DEFHOOK
+(new_address_profitable_p,
+ "Return @code{true} if it is profitable to replace the address in\n\
+@var{memref} with @var{new_addr}.  This allows targets to prevent the\n\
+scheduler from undoing address optimizations.  The instruction containing the\n\
+memref is @var{insn}.  The default implementation returns @code{true}.",
+bool, (rtx memref, rtx_insn * insn, rtx new_addr),
+default_new_address_profitable_p)
+
 DEFHOOK
 (estimated_poly_value,
  "Return an estimate of the runtime value of @var{val}, for use in\n\
diff --git a/gcc/targhooks.c b/gcc/targhooks.c
index 2d3bcbf3e89..8e3ad63f1c1 100644
--- a/gcc/targhooks.c
+++ b/gcc/targhooks.c
@@ -1564,6 +1564,19 @@ default_mode_dependent_address_p (const_rtx addr ATTRIBUTE_UNUSED,
   return false;
 }
 
+extern bool default_new_address_profitable_p (rtx, rtx);
+
+
+/* The default implementation of TARGET_NEW_ADDRESS_PROFITABLE_P.  */
+
+bool
+default_new_address_profitable_p (rtx memref ATTRIBUTE_UNUSED,
+				  rtx_insn *insn ATTRIBUTE_UNUSED,
+				  rtx new_addr ATTRIBUTE_UNUSED)
+{
+  return true;
+}
+
 bool
 default_target_option_valid_attribute_p (tree ARG_UNUSED (fndecl),
 					 tree ARG_UNUSED (name),
diff --git a/gcc/targhooks.h b/gcc/targhooks.h
index 34987bbcdb3..76a55428851 100644
--- a/gcc/targhooks.h
+++ b/gcc/targhooks.h
@@ -185,6 +185,7 @@ extern tree default_emutls_var_init (tree, tree, tree);
 extern unsigned int default_hard_regno_nregs (unsigned int, machine_mode);
 extern bool default_hard_regno_scratch_ok (unsigned int);
 extern bool default_mode_dependent_address_p (const_rtx, addr_space_t);
+extern bool default_new_address_profitable_p (rtx, rtx_insn *, rtx);
 extern bool default_target_option_valid_attribute_p (tree, tree, tree, int);
 extern bool default_target_option_pragma_parse (tree, tree);
 extern bool default_target_can_inline_p (tree, tree);
-- 
2.17.1