Re: [PATCH][Arm] Remove constraint strings from define_expand constructs in the back end

2019-06-28 Thread Kyrill Tkachov 

Hi Dennis,

On 6/27/19 4:58 PM, Dennis Zhang wrote:

Hi Kyrill,

Thanks for the review!

On 6/24/19 5:27 PM, Kyrill Tkachov wrote:

Hi Dennis,

On 6/24/19 4:13 PM, Dennis Zhang wrote:

Hi,

A number of Arm define_expand patterns have specified constraints for
their operands. But the constraint strings are ignored at expand time
and are therefore redundant/useless. We now avoid specifying constraints
in new define_expands, but we should clean up the existing define_expand
definitions.

For example, the constraint "=r" is removed in the following case:
(define_expand "reload_inhi"
      [(parallel [(match_operand:HI 0 "s_register_operand" "=r")
The "" marks with an empty constraint in define_expand are removed as
well.

The patch is tested with the build configuration of
--target=arm-linux-gnueabi and it passes gcc/testsuite.


Thank you for the patch.

Unfortunately I've hit an ICE building an arm-none-eabi target with your
patch.

This appears to be due to:

@@ -6767,9 +6767,9 @@
   ;; temporary if the address isn't offsettable -- push_reload doesn't seem
   ;; to take any notice of the "o" constraints on reload_memory_operand
operand.
   (define_expand "reload_outhi"
-  [(parallel [(match_operand:HI 0 "arm_reload_memory_operand" "=o")
-      (match_operand:HI 1 "s_register_operand"    "r")
-      (match_operand:DI 2 "s_register_operand" "=")])]
+  [(parallel [(match_operand:HI 0 "arm_reload_memory_operand")
+      (match_operand:HI 1 "s_register_operand")
+      (match_operand:DI 2 "s_register_operand")])]
     "TARGET_EITHER"
     "if (TARGET_ARM)
    arm_reload_out_hi (operands);
@@ -6780,9 +6780,9 @@
   )

   (define_expand "reload_inhi"
-  [(parallel [(match_operand:HI 0 "s_register_operand" "=r")
-      (match_operand:HI 1 "arm_reload_memory_operand" "o")
-      (match_operand:DI 2 "s_register_operand" "=")])]
+  [(parallel [(match_operand:HI 0 "s_register_operand")
+      (match_operand:HI 1 "arm_reload_memory_operand")
+      (match_operand:DI 2 "s_register_operand")])]
     "TARGET_EITHER"
     "
     if (TARGET_ARM)


the reload_in and reload_out patterns are somewhat special:

https://gcc.gnu.org/onlinedocs/gccint/Standard-Names.html#Standard-Names

where the constraints seems to matter.

We should migrate these patterns to the recommended secondary_reload
hook, but that would be a separate patches.

For now, please try removing changes to these patterns and making sure
that the build succeeds.

Thanks,

Kyrill



The special exception, reload_inm and reload_outm patterns are fixed.
Their constraints are left untouched as original in order to work
correctly in default_secondary_reload, gcc/targhook.c.

The updated patch is tested for targets: arm-none-linux-gnueabi,
arm-none-linux-gnueabihf, arm-linux-gnueabi, and arm-linux-gnueabihf.
And it survives in testsuite regression.


Thanks, I've committed this on your behalf with r272779.

Kyrill



gcc/ChangeLog:

2019-06-21  Dennis Zhang  

* config/arm/arm.md: Remove redundant constraints from
define_expand but leave reload_inm and reload_outm patterns
untouched since they need special constraints to work.
* config/arm/arm-fixed.md: Remove redundant constraints from
define_expand.
* config/arm/iwmmxt.md: Likewise.
* config/arm/neon.md: Likewise.
* config/arm/sync.md: Likewise.
* config/arm/thumb1.md: Likewise.
* config/arm/vec-common.md: Likewise.

Re: [PATCH][Arm] Remove constraint strings from define_expand constructs in the back end

2019-06-24 Thread Kyrill Tkachov 

Hi Dennis,

On 6/24/19 4:13 PM, Dennis Zhang wrote:

Hi,

A number of Arm define_expand patterns have specified constraints for
their operands. But the constraint strings are ignored at expand time
and are therefore redundant/useless. We now avoid specifying constraints
in new define_expands, but we should clean up the existing define_expand
definitions.

For example, the constraint "=r" is removed in the following case:
(define_expand "reload_inhi"
     [(parallel [(match_operand:HI 0 "s_register_operand" "=r")
The "" marks with an empty constraint in define_expand are removed as 
well.


The patch is tested with the build configuration of
--target=arm-linux-gnueabi and it passes gcc/testsuite.


Thank you for the patch.

Unfortunately I've hit an ICE building an arm-none-eabi target with your 
patch.


This appears to be due to:

@@ -6767,9 +6767,9 @@
 ;; temporary if the address isn't offsettable -- push_reload doesn't seem
 ;; to take any notice of the "o" constraints on reload_memory_operand 
operand.

 (define_expand "reload_outhi"
-  [(parallel [(match_operand:HI 0 "arm_reload_memory_operand" "=o")
-      (match_operand:HI 1 "s_register_operand"    "r")
-      (match_operand:DI 2 "s_register_operand" "=")])]
+  [(parallel [(match_operand:HI 0 "arm_reload_memory_operand")
+      (match_operand:HI 1 "s_register_operand")
+      (match_operand:DI 2 "s_register_operand")])]
   "TARGET_EITHER"
   "if (TARGET_ARM)
  arm_reload_out_hi (operands);
@@ -6780,9 +6780,9 @@
 )

 (define_expand "reload_inhi"
-  [(parallel [(match_operand:HI 0 "s_register_operand" "=r")
-      (match_operand:HI 1 "arm_reload_memory_operand" "o")
-      (match_operand:DI 2 "s_register_operand" "=")])]
+  [(parallel [(match_operand:HI 0 "s_register_operand")
+      (match_operand:HI 1 "arm_reload_memory_operand")
+      (match_operand:DI 2 "s_register_operand")])]
   "TARGET_EITHER"
   "
   if (TARGET_ARM)


the reload_in and reload_out patterns are somewhat special:

https://gcc.gnu.org/onlinedocs/gccint/Standard-Names.html#Standard-Names

where the constraints seems to matter.

We should migrate these patterns to the recommended secondary_reload 
hook, but that would be a separate patches.


For now, please try removing changes to these patterns and making sure 
that the build succeeds.


Thanks,

Kyrill

[PATCH][Arm] Remove constraint strings from define_expand constructs in the back end

2019-06-24 Thread Dennis Zhang 
Hi,

A number of Arm define_expand patterns have specified constraints for 
their operands. But the constraint strings are ignored at expand time 
and are therefore redundant/useless. We now avoid specifying constraints 
in new define_expands, but we should clean up the existing define_expand 
definitions.

For example, the constraint "=r" is removed in the following case:
(define_expand "reload_inhi"
     [(parallel [(match_operand:HI 0 "s_register_operand" "=r")
The "" marks with an empty constraint in define_expand are removed as well.

The patch is tested with the build configuration of 
--target=arm-linux-gnueabi and it passes gcc/testsuite.

Thanks,
Dennis

gcc/ChangeLog:

2019-06-21  Dennis Zhang  

       * config/arm/arm-fixed.md: Remove redundant constraints from
       define_expand.
       * config/arm/arm.md: Likewise.
       * config/arm/iwmmxt.md: Likewise.
       * config/arm/neon.md: Likewise.
       * config/arm/sync.md: Likewise.
       * config/arm/thumb1.md: Likewise.
       * config/arm/vec-common.md: Likewise.

diff --git a/gcc/config/arm/arm-fixed.md b/gcc/config/arm/arm-fixed.md
index 6534ed41488..fcab40d13f6 100644
--- a/gcc/config/arm/arm-fixed.md
+++ b/gcc/config/arm/arm-fixed.md
@@ -98,9 +98,9 @@
 ; Note: none of these do any rounding.
 
 (define_expand "mulqq3"
-  [(set (match_operand:QQ 0 "s_register_operand" "")
-	(mult:QQ (match_operand:QQ 1 "s_register_operand" "")
-		 (match_operand:QQ 2 "s_register_operand" "")))]
+  [(set (match_operand:QQ 0 "s_register_operand")
+	(mult:QQ (match_operand:QQ 1 "s_register_operand")
+		 (match_operand:QQ 2 "s_register_operand")))]
   "TARGET_DSP_MULTIPLY && arm_arch_thumb2"
 {
   rtx tmp1 = gen_reg_rtx (HImode);
@@ -116,9 +116,9 @@
 })
 
 (define_expand "mulhq3"
-  [(set (match_operand:HQ 0 "s_register_operand" "")
-	(mult:HQ (match_operand:HQ 1 "s_register_operand" "")
-		 (match_operand:HQ 2 "s_register_operand" "")))]
+  [(set (match_operand:HQ 0 "s_register_operand")
+	(mult:HQ (match_operand:HQ 1 "s_register_operand")
+		 (match_operand:HQ 2 "s_register_operand")))]
   "TARGET_DSP_MULTIPLY && arm_arch_thumb2"
 {
   rtx tmp = gen_reg_rtx (SImode);
@@ -134,9 +134,9 @@
 })
 
 (define_expand "mulsq3"
-  [(set (match_operand:SQ 0 "s_register_operand" "")
-	(mult:SQ (match_operand:SQ 1 "s_register_operand" "")
-		 (match_operand:SQ 2 "s_register_operand" "")))]
+  [(set (match_operand:SQ 0 "s_register_operand")
+	(mult:SQ (match_operand:SQ 1 "s_register_operand")
+		 (match_operand:SQ 2 "s_register_operand")))]
   "TARGET_32BIT"
 {
   rtx tmp1 = gen_reg_rtx (DImode);
@@ -156,9 +156,9 @@
 ;; Accumulator multiplies.
 
 (define_expand "mulsa3"
-  [(set (match_operand:SA 0 "s_register_operand" "")
-	(mult:SA (match_operand:SA 1 "s_register_operand" "")
-		 (match_operand:SA 2 "s_register_operand" "")))]
+  [(set (match_operand:SA 0 "s_register_operand")
+	(mult:SA (match_operand:SA 1 "s_register_operand")
+		 (match_operand:SA 2 "s_register_operand")))]
   "TARGET_32BIT"
 {
   rtx tmp1 = gen_reg_rtx (DImode);
@@ -175,9 +175,9 @@
 })
 
 (define_expand "mulusa3"
-  [(set (match_operand:USA 0 "s_register_operand" "")
-	(mult:USA (match_operand:USA 1 "s_register_operand" "")
-		  (match_operand:USA 2 "s_register_operand" "")))]
+  [(set (match_operand:USA 0 "s_register_operand")
+	(mult:USA (match_operand:USA 1 "s_register_operand")
+		  (match_operand:USA 2 "s_register_operand")))]
   "TARGET_32BIT"
 {
   rtx tmp1 = gen_reg_rtx (DImode);
@@ -317,9 +317,9 @@
 		  (const_int 32)))])
 
 (define_expand "mulha3"
-  [(set (match_operand:HA 0 "s_register_operand" "")
-	(mult:HA (match_operand:HA 1 "s_register_operand" "")
-		 (match_operand:HA 2 "s_register_operand" "")))]
+  [(set (match_operand:HA 0 "s_register_operand")
+	(mult:HA (match_operand:HA 1 "s_register_operand")
+		 (match_operand:HA 2 "s_register_operand")))]
   "TARGET_DSP_MULTIPLY && arm_arch_thumb2"
 {
   rtx tmp = gen_reg_rtx (SImode);
@@ -333,9 +333,9 @@
 })
 
 (define_expand "muluha3"
-  [(set (match_operand:UHA 0 "s_register_operand" "")
-	(mult:UHA (match_operand:UHA 1 "s_register_operand" "")
-		  (match_operand:UHA 2 "s_register_operand" "")))]
+  [(set (match_operand:UHA 0 "s_register_operand")
+	(mult:UHA (match_operand:UHA 1 "s_register_operand")
+		  (match_operand:UHA 2 "s_register_operand")))]
   "TARGET_DSP_MULTIPLY"
 {
   rtx tmp1 = gen_reg_rtx (SImode);
@@ -353,9 +353,9 @@
 })
 
 (define_expand "ssmulha3"
-  [(set (match_operand:HA 0 "s_register_operand" "")
-	(ss_mult:HA (match_operand:HA 1 "s_register_operand" "")
-		(match_operand:HA 2 "s_register_operand" "")))]
+  [(set (match_operand:HA 0 "s_register_operand")
+	(ss_mult:HA (match_operand:HA 1 "s_register_operand")
+		(match_operand:HA 2 "s_register_operand")))]
   "TARGET_32BIT && TARGET_DSP_MULTIPLY && arm_arch6"
 {
   rtx tmp = gen_reg_rtx (SImode);
@@ -373,9 +373,9 @@
 })
 
 (define_expand "usmuluha3"
-  [(set (match_operand:UHA 0 "s_register_operand" "")
-	(us_mult:UHA (match_operand:UHA 1