[PATCH 1/3] Replace insn_rtx_cost with insn_cost and pattern_cost

[PATCH 1/3] Replace insn_rtx_cost with insn_cost and pattern_cost

[Segher Boessenkool]

This replaces insn_rtx_cost with insn_cost if an insn is readily
available, and with pattern_cost otherwise.

Okayed by Jeff in https://gcc.gnu.org/ml/gcc-patches/2017-08/msg00203.html
and I'll commit it now.


Segher


2017-10-09  Segher Boessenkool  <segher@kernel.crashing.org>

	* cfgrtl.c (rtl_account_profile_record): Replace insn_rtx_cost with
	insn_cost.
	* combine.c (uid_insn_cost): Adjust comment.
	(combine_validate_cost): Adjust comment.  Use pattern_cost instead
	of insn_rtx_cost
	(combine_instructions): Use insn_cost instead of insn_rtx_cost.
	* dse.c (find_shift_sequence): Ditto.
	* ifcvt.c (cheap_bb_rtx_cost_p): Ditto.
	(bb_valid_for_noce_process_p): Use pattern_cost.
	* rtl.h (insn_rtx_cost): Delete.
	(pattern_cost): New prototype.
	(insn_cost): New prototype.
	* rtlanal.c (insn_rtx_cost): Rename to...
	(pattern_cost): ... this.
	(insn_cost): New.

---
 gcc/cfgrtl.c  |  7 +++----
 gcc/combine.c | 17 ++++++++---------
 gcc/dse.c     |  2 +-
 gcc/ifcvt.c   | 12 ++++++------
 gcc/rtl.h     |  3 ++-
 gcc/rtlanal.c | 13 +++++++++++--
 6 files changed, 31 insertions(+), 23 deletions(-)

diff --git a/gcc/cfgrtl.c b/gcc/cfgrtl.c
index 6ef47b7..739d1bb 100644
--- a/gcc/cfgrtl.c
+++ b/gcc/cfgrtl.c
@@ -5039,14 +5039,13 @@ rtl_account_profile_record (basic_block bb, int after_pass,
   FOR_BB_INSNS (bb, insn)
     if (INSN_P (insn))
       {
-	record->size[after_pass]
-	  += insn_rtx_cost (PATTERN (insn), false);
+	record->size[after_pass] += insn_cost (insn, false);
 	if (bb->count.initialized_p ())
 	  record->time[after_pass]
-	    += insn_rtx_cost (PATTERN (insn), true) * bb->count.to_gcov_type ();
+	    += insn_cost (insn, true) * bb->count.to_gcov_type ();
 	else if (profile_status_for_fn (cfun) == PROFILE_GUESSED)
 	  record->time[after_pass]
-	    += insn_rtx_cost (PATTERN (insn), true) * bb->frequency;
+	    += insn_cost (insn, true) * bb->frequency;
       }
 }
 
diff --git a/gcc/combine.c b/gcc/combine.c
index 3b98ff4..c5f6a00 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -311,7 +311,7 @@ static bool optimize_this_for_speed_p;
 
 static int max_uid_known;
 
-/* The following array records the insn_rtx_cost for every insn
+/* The following array records the insn_cost for every insn
    in the instruction stream.  */
 
 static int *uid_insn_cost;
@@ -841,7 +841,7 @@ do_SUBST_LINK (struct insn_link **into, struct insn_link *newval)
 #define SUBST_LINK(oldval, newval) do_SUBST_LINK (&oldval, newval)
 \f
 /* Subroutine of try_combine.  Determine whether the replacement patterns
-   NEWPAT, NEWI2PAT and NEWOTHERPAT are cheaper according to insn_rtx_cost
+   NEWPAT, NEWI2PAT and NEWOTHERPAT are cheaper according to insn_cost
    than the original sequence I0, I1, I2, I3 and undobuf.other_insn.  Note
    that I0, I1 and/or NEWI2PAT may be NULL_RTX.  Similarly, NEWOTHERPAT and
    undobuf.other_insn may also both be NULL_RTX.  Return false if the cost
@@ -888,11 +888,11 @@ combine_validate_cost (rtx_insn *i0, rtx_insn *i1, rtx_insn *i2, rtx_insn *i3,
     old_cost -= i1_cost;
 
 
-  /* Calculate the replacement insn_rtx_costs.  */
-  new_i3_cost = insn_rtx_cost (newpat, optimize_this_for_speed_p);
+  /* Calculate the replacement pattern_costs.  */
+  new_i3_cost = pattern_cost (newpat, optimize_this_for_speed_p);
   if (newi2pat)
     {
-      new_i2_cost = insn_rtx_cost (newi2pat, optimize_this_for_speed_p);
+      new_i2_cost = pattern_cost (newi2pat, optimize_this_for_speed_p);
       new_cost = (new_i2_cost > 0 && new_i3_cost > 0)
 		 ? new_i2_cost + new_i3_cost : 0;
     }
@@ -907,7 +907,7 @@ combine_validate_cost (rtx_insn *i0, rtx_insn *i1, rtx_insn *i2, rtx_insn *i3,
       int old_other_cost, new_other_cost;
 
       old_other_cost = INSN_COST (undobuf.other_insn);
-      new_other_cost = insn_rtx_cost (newotherpat, optimize_this_for_speed_p);
+      new_other_cost = pattern_cost (newotherpat, optimize_this_for_speed_p);
       if (old_other_cost > 0 && new_other_cost > 0)
 	{
 	  old_cost += old_other_cost;
@@ -1208,10 +1208,9 @@ combine_instructions (rtx_insn *f, unsigned int nregs)
 		  set_nonzero_bits_and_sign_copies (XEXP (links, 0), NULL_RTX,
 						    insn);
 
-	    /* Record the current insn_rtx_cost of this instruction.  */
+	    /* Record the current insn_cost of this instruction.  */
 	    if (NONJUMP_INSN_P (insn))
-	      INSN_COST (insn) = insn_rtx_cost (PATTERN (insn),
-	      					optimize_this_for_speed_p);
+	      INSN_COST (insn) = insn_cost (insn, optimize_this_for_speed_p);
 	    if (dump_file)
 	      {
 		fprintf (dump_file, "insn_cost %d for ", INSN_COST (insn));
diff --git a/gcc/dse.c b/gcc/dse.c
index cff3ac4..563ca9f 100644
--- a/gcc/dse.c
+++ b/gcc/dse.c
@@ -1653,7 +1653,7 @@ find_shift_sequence (int access_size,
       cost = 0;
       for (insn = shift_seq; insn != NULL_RTX; insn = NEXT_INSN (insn))
 	if (INSN_P (insn))
-	  cost += insn_rtx_cost (PATTERN (insn), speed);
+	  cost += insn_cost (insn, speed);
 
       /* The computation up to here is essentially independent
 	 of the arguments and could be precomputed.  It may
diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index e1b163c..278d5b24 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -121,7 +121,7 @@ count_bb_insns (const_basic_block bb)
   return count;
 }
 
-/* Determine whether the total insn_rtx_cost on non-jump insns in
+/* Determine whether the total insn_cost on non-jump insns in
    basic block BB is less than MAX_COST.  This function returns
    false if the cost of any instruction could not be estimated. 
 
@@ -140,7 +140,7 @@ cheap_bb_rtx_cost_p (const_basic_block bb,
 	      : REG_BR_PROB_BASE;
 
   /* Set scale to REG_BR_PROB_BASE to void the identical scaling
-     applied to insn_rtx_cost when optimizing for size.  Only do
+     applied to insn_cost when optimizing for size.  Only do
      this after combine because if-conversion might interfere with
      passes before combine.
 
@@ -163,7 +163,7 @@ cheap_bb_rtx_cost_p (const_basic_block bb,
     {
       if (NONJUMP_INSN_P (insn))
 	{
-	  int cost = insn_rtx_cost (PATTERN (insn), speed) * REG_BR_PROB_BASE;
+	  int cost = insn_cost (insn, speed) * REG_BR_PROB_BASE;
 	  if (cost == 0)
 	    return false;
 
@@ -3021,7 +3021,7 @@ bb_valid_for_noce_process_p (basic_block test_bb, rtx cond,
   if (first_insn == last_insn)
     {
       *simple_p = noce_operand_ok (SET_DEST (first_set));
-      *cost += insn_rtx_cost (first_set, speed_p);
+      *cost += pattern_cost (first_set, speed_p);
       return *simple_p;
     }
 
@@ -3037,7 +3037,7 @@ bb_valid_for_noce_process_p (basic_block test_bb, rtx cond,
   /* The regs that are live out of test_bb.  */
   bitmap test_bb_live_out = df_get_live_out (test_bb);
 
-  int potential_cost = insn_rtx_cost (last_set, speed_p);
+  int potential_cost = pattern_cost (last_set, speed_p);
   rtx_insn *insn;
   FOR_BB_INSNS (test_bb, insn)
     {
@@ -3057,7 +3057,7 @@ bb_valid_for_noce_process_p (basic_block test_bb, rtx cond,
 	      || reg_overlap_mentioned_p (SET_DEST (sset), cond))
 	    goto free_bitmap_and_fail;
 
-	  potential_cost += insn_rtx_cost (sset, speed_p);
+	  potential_cost += pattern_cost (sset, speed_p);
 	  bitmap_set_bit (test_bb_temps, REGNO (SET_DEST (sset)));
 	}
     }
diff --git a/gcc/rtl.h b/gcc/rtl.h
index a63f33e..f854550 100644
--- a/gcc/rtl.h
+++ b/gcc/rtl.h
@@ -3203,7 +3203,8 @@ extern int loc_mentioned_in_p (rtx *, const_rtx);
 extern rtx_insn *find_first_parameter_load (rtx_insn *, rtx_insn *);
 extern bool keep_with_call_p (const rtx_insn *);
 extern bool label_is_jump_target_p (const_rtx, const rtx_insn *);
-extern int insn_rtx_cost (rtx, bool);
+extern int pattern_cost (rtx, bool);
+extern int insn_cost (rtx_insn *, bool);
 extern unsigned seq_cost (const rtx_insn *, bool);
 
 /* Given an insn and condition, return a canonical description of
diff --git a/gcc/rtlanal.c b/gcc/rtlanal.c
index b28325e..f01eab5 100644
--- a/gcc/rtlanal.c
+++ b/gcc/rtlanal.c
@@ -5269,11 +5269,11 @@ num_sign_bit_copies1 (const_rtx x, scalar_int_mode mode, const_rtx known_x,
 	 ? 1 : bitwidth - floor_log2 (nonzero) - 1;
 }
 
-/* Calculate the rtx_cost of a single instruction.  A return value of
+/* Calculate the rtx_cost of a single instruction pattern.  A return value of
    zero indicates an instruction pattern without a known cost.  */
 
 int
-insn_rtx_cost (rtx pat, bool speed)
+pattern_cost (rtx pat, bool speed)
 {
   int i, cost;
   rtx set;
@@ -5323,6 +5323,15 @@ insn_rtx_cost (rtx pat, bool speed)
   return cost > 0 ? cost : COSTS_N_INSNS (1);
 }
 
+/* Calculate the cost of a single instruction.  A return value of zero
+   indicates an instruction pattern without a known cost.  */
+
+int
+insn_cost (rtx_insn *insn, bool speed)
+{
+  return pattern_cost (PATTERN (insn), speed);
+}
+
 /* Returns estimate on cost of computing SEQ.  */
 
 unsigned
-- 
1.8.3.1

[PATCH 2/3] combine: Use insn_cost instead of pattern_cost everywhere

[Segher Boessenkool]

Committing to trunk.


Segher


2017-10-09  Segher Boessenkool  <segher@kernel.crashing.org>

	* combine.c (combine_validate_cost): Compute the new insn_cost,
	not just pattern_cost.
	(try_combine): Adjust comment.

---
 gcc/combine.c | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/gcc/combine.c b/gcc/combine.c
index c5f6a00..9a006be 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -856,7 +856,7 @@ combine_validate_cost (rtx_insn *i0, rtx_insn *i1, rtx_insn *i2, rtx_insn *i3,
   int new_i2_cost, new_i3_cost;
   int old_cost, new_cost;
 
-  /* Lookup the original insn_rtx_costs.  */
+  /* Lookup the original insn_costs.  */
   i2_cost = INSN_COST (i2);
   i3_cost = INSN_COST (i3);
 
@@ -888,11 +888,23 @@ combine_validate_cost (rtx_insn *i0, rtx_insn *i1, rtx_insn *i2, rtx_insn *i3,
     old_cost -= i1_cost;
 
 
-  /* Calculate the replacement pattern_costs.  */
-  new_i3_cost = pattern_cost (newpat, optimize_this_for_speed_p);
+  /* Calculate the replacement insn_costs.  */
+  rtx tmp = PATTERN (i3);
+  PATTERN (i3) = newpat;
+  int tmpi = INSN_CODE (i3);
+  INSN_CODE (i3) = -1;
+  new_i3_cost = insn_cost (i3, optimize_this_for_speed_p);
+  PATTERN (i3) = tmp;
+  INSN_CODE (i3) = tmpi;
   if (newi2pat)
     {
-      new_i2_cost = pattern_cost (newi2pat, optimize_this_for_speed_p);
+      tmp = PATTERN (i2);
+      PATTERN (i2) = newi2pat;
+      tmpi = INSN_CODE (i2);
+      INSN_CODE (i2) = -1;
+      new_i2_cost = insn_cost (i2, optimize_this_for_speed_p);
+      PATTERN (i2) = tmp;
+      INSN_CODE (i2) = tmpi;
       new_cost = (new_i2_cost > 0 && new_i3_cost > 0)
 		 ? new_i2_cost + new_i3_cost : 0;
     }
@@ -907,7 +919,14 @@ combine_validate_cost (rtx_insn *i0, rtx_insn *i1, rtx_insn *i2, rtx_insn *i3,
       int old_other_cost, new_other_cost;
 
       old_other_cost = INSN_COST (undobuf.other_insn);
-      new_other_cost = pattern_cost (newotherpat, optimize_this_for_speed_p);
+      tmp = PATTERN (undobuf.other_insn);
+      PATTERN (undobuf.other_insn) = newotherpat;
+      tmpi = INSN_CODE (undobuf.other_insn);
+      INSN_CODE (undobuf.other_insn) = -1;
+      new_other_cost = insn_cost (undobuf.other_insn,
+				  optimize_this_for_speed_p);
+      PATTERN (undobuf.other_insn) = tmp;
+      INSN_CODE (undobuf.other_insn) = tmpi;
       if (old_other_cost > 0 && new_other_cost > 0)
 	{
 	  old_cost += old_other_cost;
@@ -4108,7 +4127,7 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0,
 	}
     }
 
-  /* Only allow this combination if insn_rtx_costs reports that the
+  /* Only allow this combination if insn_cost reports that the
      replacement instructions are cheaper than the originals.  */
   if (!combine_validate_cost (i0, i1, i2, i3, newpat, newi2pat, other_pat))
     {
-- 
1.8.3.1

[PATCH 3/3] Add targetm.insn_cost hook

[Segher Boessenkool]

This adds a new hook that the insn_cost function uses if a target has
implemented it (it uses the old pattern_cost nee insn_rtx_cost if not).

I'll commit this now; it was okayed by Jeff at
https://gcc.gnu.org/ml/gcc-patches/2017-08/msg00204.html .


Segher


2017-10-09  Segher Boessenkool  <segher@kernel.crashing.org>

	* target.def (insn_cost): New hook.
	* doc/tm.texi.in (TARGET_INSN_COST): New hook.
	* doc/tm.texi: Regenerate.
	* rtlanal.c (insn_cost): Use the new hook.

---
 gcc/doc/tm.texi    | 12 ++++++++++++
 gcc/doc/tm.texi.in |  2 ++
 gcc/rtlanal.c      |  3 +++
 gcc/target.def     | 14 ++++++++++++++
 4 files changed, 31 insertions(+)

diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
index 8f503e1..0377217 100644
--- a/gcc/doc/tm.texi
+++ b/gcc/doc/tm.texi
@@ -6652,6 +6652,18 @@ should probably only be given to addresses with different numbers of
 registers on machines with lots of registers.
 @end deftypefn
 
+@deftypefn {Target Hook} int TARGET_INSN_COST (rtx_insn *@var{insn}, bool @var{speed})
+This target hook describes the relative costs of RTL instructions.
+
+In implementing this hook, you can use the construct
+@code{COSTS_N_INSNS (@var{n})} to specify a cost equal to @var{n} fast
+instructions.
+
+When optimizing for code size, i.e.@: when @code{speed} is
+false, this target hook should be used to estimate the relative
+size cost of an expression, again relative to @code{COSTS_N_INSNS}.
+@end deftypefn
+
 @deftypefn {Target Hook} {unsigned int} TARGET_MAX_NOCE_IFCVT_SEQ_COST (edge @var{e})
 This hook returns a value in the same units as @code{TARGET_RTX_COSTS},
 giving the maximum acceptable cost for a sequence generated by the RTL
diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
index 15b3f1f..d2cf68f 100644
--- a/gcc/doc/tm.texi.in
+++ b/gcc/doc/tm.texi.in
@@ -4600,6 +4600,8 @@ Define this macro if a non-short-circuit operation produced by
 
 @hook TARGET_ADDRESS_COST
 
+@hook TARGET_INSN_COST
+
 @hook TARGET_MAX_NOCE_IFCVT_SEQ_COST
 
 @hook TARGET_NOCE_CONVERSION_PROFITABLE_P
diff --git a/gcc/rtlanal.c b/gcc/rtlanal.c
index f01eab5..eadf691 100644
--- a/gcc/rtlanal.c
+++ b/gcc/rtlanal.c
@@ -5329,6 +5329,9 @@ pattern_cost (rtx pat, bool speed)
 int
 insn_cost (rtx_insn *insn, bool speed)
 {
+  if (targetm.insn_cost)
+    return targetm.insn_cost (insn, speed);
+
   return pattern_cost (PATTERN (insn), speed);
 }
 
diff --git a/gcc/target.def b/gcc/target.def
index 80ef746..aac5d27 100644
--- a/gcc/target.def
+++ b/gcc/target.def
@@ -3715,6 +3715,20 @@ registers on machines with lots of registers.",
  int, (rtx address, machine_mode mode, addr_space_t as, bool speed),
  default_address_cost)
 
+/* Compute a cost for INSN.  */
+DEFHOOK
+(insn_cost,
+ "This target hook describes the relative costs of RTL instructions.\n\
+\n\
+In implementing this hook, you can use the construct\n\
+@code{COSTS_N_INSNS (@var{n})} to specify a cost equal to @var{n} fast\n\
+instructions.\n\
+\n\
+When optimizing for code size, i.e.@: when @code{speed} is\n\
+false, this target hook should be used to estimate the relative\n\
+size cost of an expression, again relative to @code{COSTS_N_INSNS}.",
+ int, (rtx_insn *insn, bool speed), NULL)
+
 /* Give a cost, in RTX Costs units, for an edge.  Like BRANCH_COST, but with
    well defined units.  */
 DEFHOOK
-- 
1.8.3.1

Re: [PATCH 3/3] Add targetm.insn_cost hook

[Sandra Loosemore]

On 10/09/2017 01:35 PM, Segher Boessenkool wrote:
> This adds a new hook that the insn_cost function uses if a target has
> implemented it (it uses the old pattern_cost nee insn_rtx_cost if not).
>
> I'll commit this now; it was okayed by Jeff at
> https://gcc.gnu.org/ml/gcc-patches/2017-08/msg00204.html .
>
>
> Segher
>
>
> 2017-10-09  Segher Boessenkool  <segher@kernel.crashing.org>
>
> 	* target.def (insn_cost): New hook.
> 	* doc/tm.texi.in (TARGET_INSN_COST): New hook.
> 	* doc/tm.texi: Regenerate.
> 	* rtlanal.c (insn_cost): Use the new hook.

As a target maintainer, I'm kind of confused by this patch, and I don't 
think the tm.texi change gives sufficient guidance about the default 
hook behavior, how it interacts with TARGET_RTX_COSTS and/or 
TARGET_ADDRESS_COST, or the different contexts the three hooks are used 
in.  Do target maintainers need to do something to define this new hook 
to prevent performance regressions?

I could try to write up some advice about cost models and tuning for the 
internals manual, but at present I don't feel like I have any 
understanding of what motivated this change or how it changed the 
recommended practices for back end tuning.  :-(

-Sandra the missing-some-brain-cells

Re: [PATCH 3/3] Add targetm.insn_cost hook

[Segher Boessenkool]

Hi!

On Wed, Oct 11, 2017 at 05:39:25PM -0600, Sandra Loosemore wrote:
> On 10/09/2017 01:35 PM, Segher Boessenkool wrote:
> >This adds a new hook that the insn_cost function uses if a target has
> >implemented it (it uses the old pattern_cost nee insn_rtx_cost if not).

> As a target maintainer, I'm kind of confused by this patch, and I don't 
> think the tm.texi change gives sufficient guidance about the default 
> hook behavior, how it interacts with TARGET_RTX_COSTS and/or 
> TARGET_ADDRESS_COST, or the different contexts the three hooks are used 
> in.  Do target maintainers need to do something to define this new hook 
> to prevent performance regressions?

No, everything works as it did if you don't do anything.  Should I add
a little bit of documentation what the hook does by default?  It may be
good as a scary story, to get more ports to use their own hook
implementation instead ;-)

> I could try to write up some advice about cost models and tuning for the 
> internals manual, but at present I don't feel like I have any 
> understanding of what motivated this change or how it changed the 
> recommended practices for back end tuning.  :-(

insn_rtx_cost did not work with instructions: it worked with instruction
_patterns_.  It also does not work for anything but single-set and similar
patterns (not the same as single_set, that requires an instruction!)

It also is really hard to write a TARGET_RTX_COSTS that properly describes
the cost of the machine instructions you have (and to keep it updated!)

With these patches combine can make better decisions already (no longer
do many instructions have "unknown" cost).  I am planning to move more
users of TARGET_RTX_COSTS over to insn_cost, or even get the needed info
in a more direct way.  This will take time; all help is welcome :-)


Segher