[PATCH] RX TARGET_RTX_COSTS function

[PATCH] RX TARGET_RTX_COSTS function

[Sebastian Perta]

Hello,

DJ has posted a patch a few years ago which implements TARGET_RTX_COSTS for
RX:
https://gcc.gnu.org/ml/gcc-patches/2012-05/msg00008.html

Nick has accepted the patch:
https://gcc.gnu.org/ml/gcc-patches/2012-05/msg00012.html

But it was never made it into the trunk.

The patch required some changes (the prototype, second param more exactly,
has changed) in order to compile in the trunk.
So I updated this and I also same a further change to the patch: I disabled
the replacement of the division with multiplication of reciprocals on -Os
because it increases code size for example for the following division:
int a;
void foo()
{
  a = a / 24;
}

The output code without this patch or with my updated patch it looks like:
_foo:
	mov.L	#_a, r4
	mov.L	[r4], r14
	div	#24, r14
	mov.L	r14, [r4]
	rts

While with DJ's original patch:

_foo:
	pushm	r7-r8
	mov.L	#_a, r3
	mov.L	[r3], r14
	mov.L	#0x2aaaaaab, r7
	emul	r14, r7
	shar	#2, r8, r4
	shar	#31, r14
	sub	r14, r4, r14
	mov.L	r14, [r3]
	rtsd	#8, r7-r8

Regression test is OK, I tested with the following command:
"make -k check-gcc RUNTESTFLAGS=--target_board=rx-sim"

Please let me know if this OK to check-in.

Best Regards,
Sebastian

Index: ChangeLog
===================================================================
--- ChangeLog	(revision 257628)
+++ ChangeLog	(working copy)
@@ -1,3 +1,8 @@
+2018-02-13  Sebastian Perta  <sebastian.perta@renesas.com>
+
+	* config/rx/rx.c (TARGET_RTX_COSTS): Define.
+	* config/rx/rx.c (rx_rtx_costs): New function.
+
 2018-02-13  Paolo Bonzini <bonzini@gnu.org>
 
 	PR sanitizer/84340



Index: rx.c
===================================================================
--- rx.c	(revision 257412)
+++ rx.c	(working copy)
@@ -2975,7 +2975,73 @@
   return COSTS_N_INSNS (1);
 }
 
+#undef  TARGET_RTX_COSTS
+#define TARGET_RTX_COSTS rx_rtx_costs
+
 static bool
+rx_rtx_costs (rtx		x,
+		machine_mode mode,
+		int          outer_code ATTRIBUTE_UNUSED,
+		int          opno ATTRIBUTE_UNUSED,
+		int *        total,
+		bool         speed)
+{
+  int code = GET_CODE (x);
+
+  if((GET_CODE (x) == CONST_INT) && (INTVAL (x) == 0))
+	{
+		*total = 0;
+		return true;
+	}
+  switch (code)
+    {
+    case MULT:
+      if (mode == DImode)
+	{
+	  *total = COSTS_N_INSNS (2);
+	  return true;
+	}
+      /* fall through */
+    case PLUS:
+    case MINUS:
+    case AND:
+    case COMPARE:
+    case IOR:
+    case XOR:
+      if (GET_CODE (XEXP (x, 0)) == MEM
+	  || GET_CODE (XEXP (x, 1)) == MEM)
+	*total = COSTS_N_INSNS (3);
+      else
+	*total = COSTS_N_INSNS (1);
+      return true;
+
+    case DIV:
+      if(speed)
+		  /* This is worst case.  */
+		  *total = COSTS_N_INSNS (20);
+      else
+		  *total = COSTS_N_INSNS (3);
+      return true;
+
+    case UDIV:
+      if(speed)
+		  /* This is worst case.  */
+		  *total = COSTS_N_INSNS (18);
+      else
+		  *total = COSTS_N_INSNS (3);
+      return true;
+
+    case IF_THEN_ELSE:
+      *total = COSTS_N_INSNS (3);
+      return true;
+
+    default:
+      break;
+    }
+  return false;
+}
+
+static bool
 rx_can_eliminate (const int from ATTRIBUTE_UNUSED, const int to)
 {
   /* We can always eliminate to the frame pointer.

Re: [PATCH] RX TARGET_RTX_COSTS function

[Oleg Endo]

Hi,

On Tue, 2018-02-13 at 15:54 +0000, Sebastian Perta wrote:
> 
> The patch required some changes (the prototype, second param more
> exactly, has changed) in order to compile in the trunk.

Could you please send the patch as an attachment?  The formatting looks
a bit off (tabs spaces etc).

> So I updated this and I also same a further change to the patch: I
> disabled the replacement of the division with multiplication of
> reciprocals on -Os because it increases code size for example for the
> following division:

Do you happen to have any other numbers on the resulting code
size/speed?  Looking at the new costs that the patch introduces, I'd
expect there to be some more changes than just the 1/x...

Cheers,
Oleg

Re: [PATCH] RX TARGET_RTX_COSTS function

[Oleg Endo]

[-- Attachment #1: Type: text/plain, Size: 1363 bytes --]

On Wed, 2018-02-14 at 01:06 +0900, Oleg Endo wrote:
> 
> Do you happen to have any other numbers on the resulting code
> size/speed?  Looking at the new costs that the patch introduces, I'd
> expect there to be some more changes than just the 1/x...
> 

I've checked your proposed patch with the CSiBE set for code size
changes.

With your patch, the code size of the whole set:
sum:  2806044 -> 2801346    -4698 / -0.167424 %


Taking out this piece

    case IF_THEN_ELSE:
      *total = COSTS_N_INSNS (3);
      return true;

from the rx_rtx_costs results in:
sum:  2806044 -> 2801099    -4945 / -0.176227 %


Taking out another piece 

      if (GET_CODE (XEXP (x, 0)) == MEM
         || GET_CODE (XEXP (x, 1)) == MEM)
       *total = COSTS_N_INSNS (3);
      else

results in:
sum:  2806044 -> 2800315    -5729 / -0.204166 %

So I'd like to propose the attached patch instead, as it eliminates 1
KByte of code more from the whole set.

Just in case, I'm testing it now with
  "make -k check" on rx-sim for c and c++

OK for trunk if it passes?

Cheers,
Oleg

gcc/ChangeLog:
	* config/rx/rx.c (rx_rtx_costs): New function.
	(TARGET_RTX_COSTS): Override to use rx_rtx_costs.

[-- Attachment #2: rx_rtx_costs.patch --]
[-- Type: text/x-patch, Size: 1780 bytes --]

Index: gcc/config/rx/rx.c
===================================================================
--- gcc/config/rx/rx.c	(revision 257655)
+++ gcc/config/rx/rx.c	(working copy)
@@ -2976,6 +2976,62 @@
 }
 
 static bool
+rx_rtx_costs (rtx x, machine_mode mode, int outer_code ATTRIBUTE_UNUSED,
+	      int opno ATTRIBUTE_UNUSED, int* total, bool speed)
+{
+  if (x == const0_rtx)
+    {
+      *total = 0;
+      return true;
+    }
+
+  switch (GET_CODE (x))
+    {
+    case MULT:
+      if (mode == DImode)
+	{
+	  *total = COSTS_N_INSNS (2);
+	  return true;
+	}
+      /* fall through */
+
+    case PLUS:
+    case MINUS:
+    case AND:
+    case COMPARE:
+    case IOR:
+    case XOR:
+      *total = COSTS_N_INSNS (1);
+      return true;
+
+    case DIV:
+      if (speed)
+	/* This is the worst case for a division.  Pessimize divisions when
+	   not optimizing for size and allow reciprocal optimizations which
+	   produce bigger code.  */
+	*total = COSTS_N_INSNS (20);
+      else
+	*total = COSTS_N_INSNS (3);
+      return true;
+
+    case UDIV:
+      if (speed)
+	/* This is the worst case for a division.  Pessimize divisions when
+	   not optimizing for size and allow reciprocal optimizations which
+	   produce bigger code.  */
+	*total = COSTS_N_INSNS (18);
+      else
+	*total = COSTS_N_INSNS (3);
+      return true;
+
+    default:
+      break;
+    }
+
+  return false;
+}
+
+static bool
 rx_can_eliminate (const int from ATTRIBUTE_UNUSED, const int to)
 {
   /* We can always eliminate to the frame pointer.
@@ -3709,6 +3765,9 @@
 #undef  TARGET_MODES_TIEABLE_P
 #define TARGET_MODES_TIEABLE_P			rx_modes_tieable_p
 
+#undef  TARGET_RTX_COSTS
+#define TARGET_RTX_COSTS rx_rtx_costs
+
 struct gcc_target targetm = TARGET_INITIALIZER;
 
 #include "gt-rx.h"

Re: [PATCH] RX TARGET_RTX_COSTS function

[Oleg Endo]

Ping.

On Thu, 2018-02-15 at 23:07 +0900, Oleg Endo wrote:
> On Wed, 2018-02-14 at 01:06 +0900, Oleg Endo wrote:
> > 
> >  
> > Do you happen to have any other numbers on the resulting code
> > size/speed?  Looking at the new costs that the patch introduces,
> > I'd
> > expect there to be some more changes than just the 1/x...
> > 
> I've checked your proposed patch with the CSiBE set for code size
> changes.
> 
> With your patch, the code size of the whole set:
> sum:  2806044 -> 2801346    -4698 / -0.167424 %
> 
> 
> Taking out this piece
> 
>     case IF_THEN_ELSE:
>       *total = COSTS_N_INSNS (3);
>       return true;
> 
> from the rx_rtx_costs results in:
> sum:  2806044 -> 2801099    -4945 / -0.176227 %
> 
> 
> Taking out another piece 
> 
>       if (GET_CODE (XEXP (x, 0)) == MEM
>          || GET_CODE (XEXP (x, 1)) == MEM)
>        *total = COSTS_N_INSNS (3);
>       else
> 
> results in:
> sum:  2806044 -> 2800315    -5729 / -0.204166 %
> 
> So I'd like to propose the attached patch instead, as it eliminates 1
> KByte of code more from the whole set.
> 
> Just in case, I'm testing it now with
>   "make -k check" on rx-sim for c and c++
> 
> OK for trunk if it passes?
> 
> Cheers,
> Oleg
> 
> gcc/ChangeLog:
> 	* config/rx/rx.c (rx_rtx_costs): New function.
> 	(TARGET_RTX_COSTS): Override to use rx_rtx_costs.

Re: [PATCH] RX TARGET_RTX_COSTS function

[Nick Clifton]

Hi Oleg,

> Ping.

Sorry - I am not very good at spotting RX bugs on the gcc-patches list. :-(

>> gcc/ChangeLog:
>> 	* config/rx/rx.c (rx_rtx_costs): New function.
>> 	(TARGET_RTX_COSTS): Override to use rx_rtx_costs.

Approved - please apply.

Cheers
  Nick

RE: [PATCH] RX TARGET_RTX_COSTS function

[Sebastian Perta]

Hi Oleg,

Sorry, for some reason your emails did not ended up in Inbox, I was quite surprized when Nick's email started with Hi Oleg.

>>Do you happen to have any other numbers on the resulting code
>>size/speed?  
The original patch from DJ was present in my local sources since 4.7 so all the benchmarks (CSIBE etc.) were done on that version (when the patch was initially applied)
Of course I re-tested the patch when I applied it to the trunk (but I didn't re-tested CSIBE) the only problem I spotted was the multiplication with the reciprocal.

Best Regards,
Sebastian

> -----Original Message-----
> From: Nick Clifton [mailto:nickc@redhat.com]
> Sent: 22 February 2018 15:41
> To: Oleg Endo <oleg.endo@t-online.de>; gcc-patches@gcc.gnu.org
> Cc: Sebastian Perta <Sebastian.Perta@renesas.com>; 'DJ Delorie'
> <dj@redhat.com>
> Subject: Re: [PATCH] RX TARGET_RTX_COSTS function
> 
> Hi Oleg,
> 
> > Ping.
> 
> Sorry - I am not very good at spotting RX bugs on the gcc-patches list. :-(
> 
> >> gcc/ChangeLog:
> >> 	* config/rx/rx.c (rx_rtx_costs): New function.
> >> 	(TARGET_RTX_COSTS): Override to use rx_rtx_costs.
> 
> Approved - please apply.
> 
> Cheers
>   Nick
> 

Re: [PATCH] RX TARGET_RTX_COSTS function

[Oleg Endo]

On Thu, 2018-02-22 at 15:41 +0000, Nick Clifton wrote:


> > > gcc/ChangeLog:
> > > 	* config/rx/rx.c (rx_rtx_costs): New function.
> > > 	(TARGET_RTX_COSTS): Override to use rx_rtx_costs.
> Approved - please apply.
> 

Thanks.  Committed as r257905.

Cheers,
Oleg