diff mbox

[ARM,PR65768] Keep constants in register when expanding

Message ID 553D6FCD.8010808@linaro.org
State New
Headers show

Commit Message

Kugan Vivekanandarajah April 26, 2015, 11:07 p.m. UTC 
> While I support your goals, I think your approach needs some refinement.
> 
> In particular, we DO NOT want another function that starts looking at
> constant values and tries to decide, on a case by case basis, what to do
> with that value.  We need to keep the logic for that, as much as
> possible, in one small set of functions so that the compiler cannot end
> up with conflicting decisions coming from different bits of code.
> 
> So const_ok_for_split has to go.  Instead you should be using
> const_ok_for op (an existing routine) and a simple macro that
> encapsulates "optimize >= 2 && can_create_pseudo_p ()" as the gate for
> when to use a separate scratch register.
> 

Thanks for the review. Please find the patch attached that does this.
Regression tested on qemu for arm-none-linux-gnueab with no new
regressions. Bootstrapped successfully on chromebook. Is this OK for trunk?

Thanks,
Kugan


gcc/ChangeLog:

2015-04-26  Kugan Vivekanandarajah  <kuganv@linaro.org>
	    Zhenqiang Chen  <zhenqiang.chen@linaro.org>

	PR target/65768
	* config/arm/arm.h (CONST_OK_FOR_SPLIT): New macro.
	* config/arm/arm.md (subsi3, andsi3, iorsi3, xorsi3, movsi): Keep some
	 large constants in register instead of splitting them.

gcc/testsuite/ChangeLog:

2015-04-26  Kugan Vivekanandarajah  <kuganv@linaro.org>
	    Zhenqiang Chen  <zhenqiang.chen@linaro.org>

	PR target/65768
	* gcc.target/arm/maskdata.c: New test.

Comments

Kugan Vivekanandarajah May 14, 2015, 2:17 a.m. UTC | #1 
Ping ?

Thanks,
Kugan

On 27/04/15 09:07, Kugan wrote:
> 
>> While I support your goals, I think your approach needs some refinement.
>>
>> In particular, we DO NOT want another function that starts looking at
>> constant values and tries to decide, on a case by case basis, what to do
>> with that value.  We need to keep the logic for that, as much as
>> possible, in one small set of functions so that the compiler cannot end
>> up with conflicting decisions coming from different bits of code.
>>
>> So const_ok_for_split has to go.  Instead you should be using
>> const_ok_for op (an existing routine) and a simple macro that
>> encapsulates "optimize >= 2 && can_create_pseudo_p ()" as the gate for
>> when to use a separate scratch register.
>>
> 
> Thanks for the review. Please find the patch attached that does this.
> Regression tested on qemu for arm-none-linux-gnueab with no new
> regressions. Bootstrapped successfully on chromebook. Is this OK for trunk?
> 
> Thanks,
> Kugan
> 
> 
> gcc/ChangeLog:
> 
> 2015-04-26  Kugan Vivekanandarajah  <kuganv@linaro.org>
> 	    Zhenqiang Chen  <zhenqiang.chen@linaro.org>
> 
> 	PR target/65768
> 	* config/arm/arm.h (CONST_OK_FOR_SPLIT): New macro.
> 	* config/arm/arm.md (subsi3, andsi3, iorsi3, xorsi3, movsi): Keep some
> 	 large constants in register instead of splitting them.
> 
> gcc/testsuite/ChangeLog:
> 
> 2015-04-26  Kugan Vivekanandarajah  <kuganv@linaro.org>
> 	    Zhenqiang Chen  <zhenqiang.chen@linaro.org>
> 
> 	PR target/65768
> 	* gcc.target/arm/maskdata.c: New test.
>
diff mbox

Patch

diff --git a/gcc/config/arm/arm.h b/gcc/config/arm/arm.h
index 8c10ea3..89f0a52 100644
--- a/gcc/config/arm/arm.h
+++ b/gcc/config/arm/arm.h
@@ -389,6 +389,11 @@  extern void (*arm_lang_output_object_attributes_hook)(void);
 /* Should NEON be used for 64-bits bitops.  */
 #define TARGET_PREFER_NEON_64BITS (prefer_neon_for_64bits)
 
+/* Should constant I be slplit for OP.  */
+#define CONST_OK_FOR_SPLIT(i, op)	(optimize < 2 \
+					 || !can_create_pseudo_p () \
+					 || const_ok_for_op (i, op))
+
 /* True iff the full BPABI is being used.  If TARGET_BPABI is true,
    then TARGET_AAPCS_BASED must be true -- but the converse does not
    hold.  TARGET_BPABI implies the use of the BPABI runtime library,
diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
index 164ac13..b499885 100644
--- a/gcc/config/arm/arm.md
+++ b/gcc/config/arm/arm.md
@@ -1164,10 +1164,16 @@ 
     {
       if (TARGET_32BIT)
         {
-          arm_split_constant (MINUS, SImode, NULL_RTX,
-	                      INTVAL (operands[1]), operands[0],
-	  		      operands[2], optimize && can_create_pseudo_p ());
-          DONE;
+	  if (!CONST_OK_FOR_SPLIT (INTVAL (operands[1]), MINUS))
+	    operands[1] = force_reg (SImode, operands[1]);
+	  else
+	    {
+	      arm_split_constant (MINUS, SImode, NULL_RTX,
+				  INTVAL (operands[1]), operands[0],
+				  operands[2],
+				  optimize && can_create_pseudo_p ());
+	      DONE;
+	    }
 	}
       else /* TARGET_THUMB1 */
         operands[1] = force_reg (SImode, operands[1]);
@@ -2078,14 +2084,19 @@ 
 	      operands[1] = convert_to_mode (QImode, operands[1], 1);
 	      emit_insn (gen_thumb2_zero_extendqisi2_v6 (operands[0],
 							 operands[1]));
+	      DONE;
 	    }
+	  else if (!CONST_OK_FOR_SPLIT (INTVAL (operands[2]), AND))
+	    operands[2] = force_reg (SImode, operands[2]);
 	  else
-	    arm_split_constant (AND, SImode, NULL_RTX,
-				INTVAL (operands[2]), operands[0],
-				operands[1],
-				optimize && can_create_pseudo_p ());
+	    {
+	      arm_split_constant (AND, SImode, NULL_RTX,
+				  INTVAL (operands[2]), operands[0],
+				  operands[1],
+				  optimize && can_create_pseudo_p ());
 
-          DONE;
+	      DONE;
+	    }
         }
     }
   else /* TARGET_THUMB1 */
@@ -2884,10 +2895,16 @@ 
     {
       if (TARGET_32BIT)
         {
-          arm_split_constant (IOR, SImode, NULL_RTX,
-	                      INTVAL (operands[2]), operands[0], operands[1],
-			      optimize && can_create_pseudo_p ());
-          DONE;
+	  if (!CONST_OK_FOR_SPLIT (INTVAL (operands[2]), IOR))
+	    operands[2] = force_reg (SImode, operands[2]);
+	  else
+	    {
+	      arm_split_constant (IOR, SImode, NULL_RTX,
+				  INTVAL (operands[2]), operands[0],
+				  operands[1],
+				  optimize && can_create_pseudo_p ());
+	      DONE;
+	    }
 	}
       else /* TARGET_THUMB1 */
         {
@@ -3054,10 +3071,16 @@ 
     {
       if (TARGET_32BIT)
         {
-          arm_split_constant (XOR, SImode, NULL_RTX,
-	                      INTVAL (operands[2]), operands[0], operands[1],
-			      optimize && can_create_pseudo_p ());
-          DONE;
+	  if (!CONST_OK_FOR_SPLIT (INTVAL (operands[2]), XOR))
+	    operands[2] = force_reg (SImode, operands[2]);
+	  else
+	    {
+	      arm_split_constant (XOR, SImode, NULL_RTX,
+				  INTVAL (operands[2]), operands[0],
+				  operands[1],
+				  optimize && can_create_pseudo_p ());
+	      DONE;
+	    }
 	}
       else /* TARGET_THUMB1 */
         {
@@ -5554,10 +5577,18 @@ 
           && !(const_ok_for_arm (INTVAL (operands[1]))
                || const_ok_for_arm (~INTVAL (operands[1]))))
         {
-           arm_split_constant (SET, SImode, NULL_RTX,
-	                       INTVAL (operands[1]), operands[0], NULL_RTX,
-			       optimize && can_create_pseudo_p ());
-          DONE;
+	   if (!CONST_OK_FOR_SPLIT (INTVAL (operands[1]), SET))
+	     {
+		emit_insn (gen_rtx_SET (VOIDmode, operands[0], operands[1]));
+		DONE;
+	     }
+	  else
+	     {
+		arm_split_constant (SET, SImode, NULL_RTX,
+	                            INTVAL (operands[1]), operands[0], NULL_RTX,
+			            optimize && can_create_pseudo_p ());
+		DONE;
+	     }
         }
     }
   else /* TARGET_THUMB1...  */
diff --git a/gcc/testsuite/gcc.target/arm/maskdata.c b/gcc/testsuite/gcc.target/arm/maskdata.c
index e69de29..6d6bb39 100644
--- a/gcc/testsuite/gcc.target/arm/maskdata.c
+++ b/gcc/testsuite/gcc.target/arm/maskdata.c
@@ -0,0 +1,15 @@ 
+/* { dg-do compile } */
+/* { dg-options " -O2 -fno-gcse " }  */
+/* { dg-require-effective-target arm_thumb2_ok } */
+
+#define MASK 0xff00ff
+void maskdata (int * data, int len)
+{
+   int i = len;
+   for (; i > 0; i -= 2)
+    {
+      data[i] &= MASK;
+      data[i + 1] &= MASK;
+    }
+}
+/* { dg-final { scan-assembler-not "65280" } } */