Patchwork [ARM,2/2] Fix DImode addressing

login
register
mail settings
Submitter Jie Zhang
Date Dec. 22, 2010, 6:41 a.m.
Message ID <4D119DA9.4060002@codesourcery.com>
Download mbox | patch
Permalink /patch/76367/
State New
Headers show

Comments

Jie Zhang - Dec. 22, 2010, 6:41 a.m.
Based on Ricard's explanation, it will be good to allow larger index 
range for DImode addressing on modern ARM chips. Like this patch does. I 
use multiple_operation_profitable_p to decide if it's good to use LDM or 
not. Currently multiple_operation_profitable_p is not fine tuned. In 
future the four conditionals about "low" in

+         if ((low != -8 && low != -4 && low != 0 && low != 4)
+             || !multiple_operation_profitable_p (false, 2, val))

should be moved into multiple_operation_profitable_p.

It's still under testing. Is it OK if the result is good?


Regards,
Ramana Radhakrishnan - Dec. 23, 2010, 12:58 p.m.
Hi Jie,

I'm not sure I totally understand your patch. 


On Wed, 2010-12-22 at 14:41 +0800, Jie Zhang wrote: 
> Based on Ricard's explanation, it will be good to allow larger index 
> range for DImode addressing on modern ARM chips. Like this patch does. I 
> use multiple_operation_profitable_p to decide if it's good to use LDM or 
> not. Currently multiple_operation_profitable_p is not fine tuned. In 
> future the four conditionals about "low" in

Is this even going to work ? The patch didn't seem to define
multiple_operation_profitable_p anywhere.


> 
> +         if ((low != -8 && low != -4 && low != 0 && low != 4)
> +             || !multiple_operation_profitable_p (false, 2, val))
> 
> should be moved into multiple_operation_profitable_p.

Since the code is identical - why not move it in there today or is that
what you are saying ? Ideally I think it would be better to move this in
as a flag into the costs structure per core .

> 
> It's still under testing. Is it OK if the result is good?

What is the size or performance impact of this on something like
SPECINT2k ? I thought that vortex or was it chess in there that uses a
bit of DImode arithmetic ?

cheers
Ramana 
> 
> 
> Regards,
Jie Zhang - Dec. 23, 2010, 1:29 p.m.
On 12/23/2010 08:58 PM, Ramana Radhakrishnan wrote:
>
> Hi Jie,
>
> I'm not sure I totally understand your patch.
>
>
> On Wed, 2010-12-22 at 14:41 +0800, Jie Zhang wrote:
>> Based on Ricard's explanation, it will be good to allow larger index
>> range for DImode addressing on modern ARM chips. Like this patch does. I
>> use multiple_operation_profitable_p to decide if it's good to use LDM or
>> not. Currently multiple_operation_profitable_p is not fine tuned. In
>> future the four conditionals about "low" in
>
> Is this even going to work ? The patch didn't seem to define
> multiple_operation_profitable_p anywhere.
>
multiple_operation_profitable_p was added by Bernd 4 months ago.

>
>>
>> +         if ((low != -8&&  low != -4&&  low != 0&&  low != 4)
>> +             || !multiple_operation_profitable_p (false, 2, val))
>>
>> should be moved into multiple_operation_profitable_p.
>
> Since the code is identical - why not move it in there today or is that
> what you are saying ? Ideally I think it would be better to move this in
> as a flag into the costs structure per core .
>
I'm not sure if it's good to move it into 
multiple_operation_profitable_p since it's also called from some places. 
Moving into it but not affecting other code needs more time thinking. 
For now I just try to keep my patch as simple as possible to ease code 
review. We can move it into multiple_operation_profitable_p later when 
multiple_operation_profitable_p will be changed largely.

>>
>> It's still under testing. Is it OK if the result is good?
>
> What is the size or performance impact of this on something like
> SPECINT2k ? I thought that vortex or was it chess in there that uses a
> bit of DImode arithmetic ?
>
I have not done the performance benchmarking. I just did the regression 
testing.

There are no regressions for arm-none-eabi target with c,c++,lto and the 
default multilib. And there are no regressions found with the backported 
patch for our (CodeSourcery now Mentor Graphics) internal 4.5 branch for 
arm-none-linux-gnueabi target with c,c++,lto and "-march=armv7-a 
-mfloat-abi=softfp -mfpu=neon".


Regards,
Ramana Radhakrishnan - Dec. 23, 2010, 4:28 p.m.
On Thu, 2010-12-23 at 21:29 +0800, Jie Zhang wrote: 
> On 12/23/2010 08:58 PM, Ramana Radhakrishnan wrote:
> >
> > Hi Jie,
> >
> > I'm not sure I totally understand your patch.
> >
> >
> > On Wed, 2010-12-22 at 14:41 +0800, Jie Zhang wrote:
> >> Based on Ricard's explanation, it will be good to allow larger index
> >> range for DImode addressing on modern ARM chips. Like this patch does. I
> >> use multiple_operation_profitable_p to decide if it's good to use LDM or
> >> not. Currently multiple_operation_profitable_p is not fine tuned. In
> >> future the four conditionals about "low" in
> >
> > Is this even going to work ? The patch didn't seem to define
> > multiple_operation_profitable_p anywhere.
> >
> multiple_operation_profitable_p was added by Bernd 4 months ago.


I suppose I was confused by the additional declaration in your patch and
probably was looking in the wrong buffer yesteray. Sorry about that. 

> 
> >
> >>
> >> +         if ((low != -8&&  low != -4&&  low != 0&&  low != 4)
> >> +             || !multiple_operation_profitable_p (false, 2, val))
> >>
> >> should be moved into multiple_operation_profitable_p.
> >
> > Since the code is identical - why not move it in there today or is that
> > what you are saying ? Ideally I think it would be better to move this in
> > as a flag into the costs structure per core .
> >
> I'm not sure if it's good to move it into 
> multiple_operation_profitable_p since it's also called from some places. 
> Moving into it but not affecting other code needs more time thinking. 
> For now I just try to keep my patch as simple as possible to ease code 
> review. We can move it into multiple_operation_profitable_p later when 
> multiple_operation_profitable_p will be changed largely.

Then I would suggest that you add a comment or a FIXME: that marks this
as something that has to be looked at when we look at tuning
multiple_operation_profitable_p . 



cheers 
Ramana

Patch


	* config/arm/arm.c (multiple_operation_profitable_p): Declare.
	(arm_legitimize_address): Adjust handling for DImode and DFmode.
	(arm_legitimize_reload_address): Likewise.

diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index 95bde4a..c926e17 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -250,6 +250,7 @@  static bool arm_builtin_support_vector_misalignment (enum machine_mode mode,
 						     bool is_packed);
 static void arm_conditional_register_usage (void);
 static reg_class_t arm_preferred_rename_class (reg_class_t rclass);
+static bool multiple_operation_profitable_p (bool, int, HOST_WIDE_INT);
 
 
 /* Table of machine attributes.  */
@@ -6230,11 +6231,14 @@  arm_legitimize_address (rtx x, rtx orig_x, enum machine_mode mode)
 	  rtx base_reg, val;
 	  n = INTVAL (xop1);
 
-	  /* VFP addressing modes actually allow greater offsets, but for
-	     now we just stick with the lowest common denominator.  */
 	  if (mode == DImode
 	      || ((TARGET_SOFT_FLOAT || TARGET_VFP) && mode == DFmode))
 	    {
+	      HOST_WIDE_INT orig_n = n;
+
+	      /* We first see if LDM is profitable.  If not, we will use
+		 the lowest common denominator suitable for two LDRs
+		 and VLDR, and for LDRD if TARGET_LDRD.  */
 	      low_n = n & 0x0f;
 	      n &= ~0x0f;
 	      if (low_n > 4)
@@ -6242,6 +6246,14 @@  arm_legitimize_address (rtx x, rtx orig_x, enum machine_mode mode)
 		  n += 16;
 		  low_n -= 16;
 		}
+	      if ((low_n != -8 && low_n != -4 && low_n != 0 && low_n != 4)
+		  || !multiple_operation_profitable_p (false, 2, orig_n))
+		{
+		  HOST_WIDE_INT mask = (TARGET_LDRD ? 0xfc : 0x3fc);
+		  n = orig_n;
+		  low_n = (n >= 0 ? (n & mask) : -((-n) & mask));
+		  n -= low_n;
+		}
 	    }
 	  else
 	    {
@@ -6404,7 +6416,18 @@  arm_legitimize_reload_address (rtx *p,
       HOST_WIDE_INT low, high;
 
       if (mode == DImode || (mode == DFmode && TARGET_SOFT_FLOAT))
-	low = ((val & 0xf) ^ 0x8) - 0x8;
+	{
+	  /* We first see if LDM is profitable.  If not, we will use
+	     the lowest common denominator suitable for two LDRs
+	     and VLDR, and for LDRD if TARGET_LDRD.  */
+	  low = ((val & 0xf) ^ 0x8) - 0x8;
+	  if ((low != -8 && low != -4 && low != 0 && low != 4)
+	      || !multiple_operation_profitable_p (false, 2, val))
+	    {
+	      HOST_WIDE_INT mask = (TARGET_LDRD ? 0xfc : 0x3fc);
+	      low = (val >= 0 ? (val & mask) : -((-val) & mask));
+	    }
+	}
       else if (TARGET_MAVERICK && TARGET_HARD_FLOAT)
 	/* Need to be careful, -256 is not a valid offset.  */
 	low = val >= 0 ? (val & 0xff) : -((-val) & 0xff);