Patchwork RFA: Switching LRA on for s390

login
register
mail settings
Submitter Vladimir Makarov
Date May 31, 2013, 6:11 p.m.
Message ID <51A8E7DA.7080803@redhat.com>
Download mbox | patch
Permalink /patch/247996/
State New
Headers show

Comments

Vladimir Makarov - May 31, 2013, 6:11 p.m.
The following patch switches LRA on for s390.  The patch introduces
a new option -mlra to use LRA instead of reload.  I did not document
the option as I'd like to delete it for gcc4.9.  By default, LRA will
be used for s390 instead of reload for better testing LRA for s390.

   Changes in s390.md are because of define_splits for the last
alternative *movmem_short, *clrmem_short, *cmpmem_short have guard
TARGET_CPU_ZARCH.

   The patch was successfully bootstrapped and tested on s390x.

   Any comments?
   Is it ok to commit the patch into trunk?

   Thanks.


2013-05-31  Vladimir Makarov  <vmakarov@redhat.com>

     * config/s390/s390.opt (mlra): New option.
     * config/s390/s390.c (s390_decompose_address): Check displacement
     for all registers for LRA.
     (s390_secondary_reload): Don't used secondary reloads for LRA.
     (s390_lra_p): New function.
     (TARGET_LRA_P): Define.
     * config/s390/s390.md (*movmem_short, *clrmem_short): Change value
     of attribute cpu_facility to zarch for the last alternative.
     (*cmpmem_short): Ditto.
Andreas Krebbel - June 5, 2013, 1:58 p.m.
On Fri, May 31, 2013 at 02:11:38PM -0400, Vladimir Makarov wrote:
>   The patch was successfully bootstrapped and tested on s390x.
> 
>   Any comments?
>   Is it ok to commit the patch into trunk?
> 
>   Thanks.
> 
> 
> 2013-05-31  Vladimir Makarov  <vmakarov@redhat.com>
> 
>     * config/s390/s390.opt (mlra): New option.
>     * config/s390/s390.c (s390_decompose_address): Check displacement
>     for all registers for LRA.
>     (s390_secondary_reload): Don't used secondary reloads for LRA.
>     (s390_lra_p): New function.
>     (TARGET_LRA_P): Define.
>     * config/s390/s390.md (*movmem_short, *clrmem_short): Change value
>     of attribute cpu_facility to zarch for the last alternative.
>     (*cmpmem_short): Ditto.

In the last chunk z900 can be used instead of zarch.  The splitters
are guarded with TARGET_CPU_ZARCH. This requires a zarch cpu but it
does not need to run in zarch mode.  z900 is the first zarch cpu.

Ok with that change.  Many thanks for doing the migration.  We'll try
to do a performance regression run soon.

Bye,

-Andreas-
Andreas Krebbel - June 7, 2013, 2:57 p.m.
On Fri, May 31, 2013 at 02:11:38PM -0400, Vladimir Makarov wrote:
>   The following patch switches LRA on for s390.  The patch introduces
> a new option -mlra to use LRA instead of reload.  I did not document
> the option as I'd like to delete it for gcc4.9.  By default, LRA will
> be used for s390 instead of reload for better testing LRA for s390.
> 
>   Changes in s390.md are because of define_splits for the last
> alternative *movmem_short, *clrmem_short, *cmpmem_short have guard
> TARGET_CPU_ZARCH.
> 
>   The patch was successfully bootstrapped and tested on s390x.
> 
>   Any comments?
>   Is it ok to commit the patch into trunk?
> 
>   Thanks.
> 
> 
> 2013-05-31  Vladimir Makarov  <vmakarov@redhat.com>
> 
>     * config/s390/s390.opt (mlra): New option.
>     * config/s390/s390.c (s390_decompose_address): Check displacement
>     for all registers for LRA.
>     (s390_secondary_reload): Don't used secondary reloads for LRA.
>     (s390_lra_p): New function.
>     (TARGET_LRA_P): Define.
>     * config/s390/s390.md (*movmem_short, *clrmem_short): Change value
>     of attribute cpu_facility to zarch for the last alternative.
>     (*cmpmem_short): Ditto.

I've applied the attached patch. This helps me getting a little
further when bootstrapping with lra and --with-arch=zEC12.

2013-06-07  Andreas Krebbel  <Andreas.Krebbel@de.ibm.com>

	* config/s390/s390.md (cpu_facility): Add cpu_zarch.
	("*movmem_short", "*clrmem_short", "*cmpmem_short): Use cpu_zarch
	for last alternative in the cpu_facility attribute.

---
 gcc/config/s390/s390.md |   13 ++++!!!!!!!!!
 1 file changed, 4 insertions(+), 9 modifications(!)

Index: gcc/config/s390/s390.md
===================================================================
*** gcc/config/s390/s390.md.orig
--- gcc/config/s390/s390.md
***************
*** 277,283 ****
  (define_attr "cpu" "g5,g6,z900,z990,z9_109,z9_ec,z10,z196,zEC12"
    (const (symbol_ref "s390_tune_attr")))
  
! (define_attr "cpu_facility" "standard,ieee,zarch,longdisp,extimm,dfp,z10,z196,zEC12"
    (const_string "standard"))
  
  (define_attr "enabled" ""
--- 277,284 ----
  (define_attr "cpu" "g5,g6,z900,z990,z9_109,z9_ec,z10,z196,zEC12"
    (const (symbol_ref "s390_tune_attr")))
  
! (define_attr "cpu_facility"
!   "standard,ieee,zarch,cpu_zarch,longdisp,extimm,dfp,z10,z196,zEC12"
    (const_string "standard"))
  
  (define_attr "enabled" ""
***************
*** 304,309 ****
--- 305,314 ----
  	      (match_test "TARGET_DFP"))
  	 (const_int 1)
  
+          (and (eq_attr "cpu_facility" "cpu_zarch")
+               (match_test "TARGET_CPU_ZARCH"))
+ 	 (const_int 1)
+ 
           (and (eq_attr "cpu_facility" "z10")
                (match_test "TARGET_Z10"))
  	 (const_int 1)
***************
*** 2690,2696 ****
    "(GET_MODE (operands[2]) == Pmode || GET_MODE (operands[2]) == VOIDmode)"
    "#"
    [(set_attr "type"         "cs")
!    (set_attr "cpu_facility" "*,*,z10,zarch")])
  
  (define_split
    [(set (match_operand:BLK 0 "memory_operand" "")
--- 2695,2701 ----
    "(GET_MODE (operands[2]) == Pmode || GET_MODE (operands[2]) == VOIDmode)"
    "#"
    [(set_attr "type"         "cs")
!    (set_attr "cpu_facility" "*,*,z10,cpu_zarch")])
  
  (define_split
    [(set (match_operand:BLK 0 "memory_operand" "")
***************
*** 2899,2905 ****
    "(GET_MODE (operands[1]) == Pmode || GET_MODE (operands[1]) == VOIDmode)"
    "#"
    [(set_attr "type" "cs")
!    (set_attr "cpu_facility" "*,*,z10,zarch")])
  
  (define_split
    [(set (match_operand:BLK 0 "memory_operand" "")
--- 2904,2910 ----
    "(GET_MODE (operands[1]) == Pmode || GET_MODE (operands[1]) == VOIDmode)"
    "#"
    [(set_attr "type" "cs")
!    (set_attr "cpu_facility" "*,*,z10,cpu_zarch")])
  
  (define_split
    [(set (match_operand:BLK 0 "memory_operand" "")
***************
*** 3075,3081 ****
    "(GET_MODE (operands[2]) == Pmode || GET_MODE (operands[2]) == VOIDmode)"
    "#"
    [(set_attr "type" "cs")
!    (set_attr "cpu_facility" "*,*,z10,zarch")])
  
  (define_split
    [(set (reg:CCU CC_REGNUM)
--- 3080,3086 ----
    "(GET_MODE (operands[2]) == Pmode || GET_MODE (operands[2]) == VOIDmode)"
    "#"
    [(set_attr "type" "cs")
!    (set_attr "cpu_facility" "*,*,z10,cpu_zarch")])
  
  (define_split
    [(set (reg:CCU CC_REGNUM)
Vladimir Makarov - June 7, 2013, 3:12 p.m.
On 13-06-07 10:57 AM, Andreas Krebbel wrote:
> On Fri, May 31, 2013 at 02:11:38PM -0400, Vladimir Makarov wrote:
>>    The following patch switches LRA on for s390.  The patch introduces
>> a new option -mlra to use LRA instead of reload.  I did not document
>> the option as I'd like to delete it for gcc4.9.  By default, LRA will
>> be used for s390 instead of reload for better testing LRA for s390.
>>
>>    Changes in s390.md are because of define_splits for the last
>> alternative *movmem_short, *clrmem_short, *cmpmem_short have guard
>> TARGET_CPU_ZARCH.
>>
>>    The patch was successfully bootstrapped and tested on s390x.
>>
>>    Any comments?
>>    Is it ok to commit the patch into trunk?
>>
>>    Thanks.
>>
>>
>> 2013-05-31  Vladimir Makarov  <vmakarov@redhat.com>
>>
>>      * config/s390/s390.opt (mlra): New option.
>>      * config/s390/s390.c (s390_decompose_address): Check displacement
>>      for all registers for LRA.
>>      (s390_secondary_reload): Don't used secondary reloads for LRA.
>>      (s390_lra_p): New function.
>>      (TARGET_LRA_P): Define.
>>      * config/s390/s390.md (*movmem_short, *clrmem_short): Change value
>>      of attribute cpu_facility to zarch for the last alternative.
>>      (*cmpmem_short): Ditto.
> I've applied the attached patch. This helps me getting a little
> further when bootstrapping with lra and --with-arch=zEC12.
>
> 2013-06-07  Andreas Krebbel  <Andreas.Krebbel@de.ibm.com>
>
> 	* config/s390/s390.md (cpu_facility): Add cpu_zarch.
> 	("*movmem_short", "*clrmem_short", "*cmpmem_short): Use cpu_zarch
> 	for last alternative in the cpu_facility attribute.
>
> ---
>   gcc/config/s390/s390.md |   13 ++++!!!!!!!!!
>   1 file changed, 4 insertions(+), 9 modifications(!)
>
> Index: gcc/config/s390/s390.md
> ===================================================================
> *** gcc/config/s390/s390.md.orig
> --- gcc/config/s390/s390.md
> ***************
> *** 277,283 ****
>    (define_attr "cpu" "g5,g6,z900,z990,z9_109,z9_ec,z10,z196,zEC12"
>      (const (symbol_ref "s390_tune_attr")))
>    
> ! (define_attr "cpu_facility" "standard,ieee,zarch,longdisp,extimm,dfp,z10,z196,zEC12"
>      (const_string "standard"))
>    
>    (define_attr "enabled" ""
> --- 277,284 ----
>    (define_attr "cpu" "g5,g6,z900,z990,z9_109,z9_ec,z10,z196,zEC12"
>      (const (symbol_ref "s390_tune_attr")))
>    
> ! (define_attr "cpu_facility"
> !   "standard,ieee,zarch,cpu_zarch,longdisp,extimm,dfp,z10,z196,zEC12"
>      (const_string "standard"))
>    
>    (define_attr "enabled" ""
> ***************
> *** 304,309 ****
> --- 305,314 ----
>    	      (match_test "TARGET_DFP"))
>    	 (const_int 1)
>    
> +          (and (eq_attr "cpu_facility" "cpu_zarch")
> +               (match_test "TARGET_CPU_ZARCH"))
> + 	 (const_int 1)
> +
>             (and (eq_attr "cpu_facility" "z10")
>                  (match_test "TARGET_Z10"))
>    	 (const_int 1)
> ***************
> *** 2690,2696 ****
>      "(GET_MODE (operands[2]) == Pmode || GET_MODE (operands[2]) == VOIDmode)"
>      "#"
>      [(set_attr "type"         "cs")
> !    (set_attr "cpu_facility" "*,*,z10,zarch")])
>    
>    (define_split
>      [(set (match_operand:BLK 0 "memory_operand" "")
> --- 2695,2701 ----
>      "(GET_MODE (operands[2]) == Pmode || GET_MODE (operands[2]) == VOIDmode)"
>      "#"
>      [(set_attr "type"         "cs")
> !    (set_attr "cpu_facility" "*,*,z10,cpu_zarch")])
>    
>    (define_split
>      [(set (match_operand:BLK 0 "memory_operand" "")
> ***************
> *** 2899,2905 ****
>      "(GET_MODE (operands[1]) == Pmode || GET_MODE (operands[1]) == VOIDmode)"
>      "#"
>      [(set_attr "type" "cs")
> !    (set_attr "cpu_facility" "*,*,z10,zarch")])
>    
>    (define_split
>      [(set (match_operand:BLK 0 "memory_operand" "")
> --- 2904,2910 ----
>      "(GET_MODE (operands[1]) == Pmode || GET_MODE (operands[1]) == VOIDmode)"
>      "#"
>      [(set_attr "type" "cs")
> !    (set_attr "cpu_facility" "*,*,z10,cpu_zarch")])
>    
>    (define_split
>      [(set (match_operand:BLK 0 "memory_operand" "")
> ***************
> *** 3075,3081 ****
>      "(GET_MODE (operands[2]) == Pmode || GET_MODE (operands[2]) == VOIDmode)"
>      "#"
>      [(set_attr "type" "cs")
> !    (set_attr "cpu_facility" "*,*,z10,zarch")])
>    
>    (define_split
>      [(set (reg:CCU CC_REGNUM)
> --- 3080,3086 ----
>      "(GET_MODE (operands[2]) == Pmode || GET_MODE (operands[2]) == VOIDmode)"
>      "#"
>      [(set_attr "type" "cs")
> !    (set_attr "cpu_facility" "*,*,z10,cpu_zarch")])
>    
>    (define_split
>      [(set (reg:CCU CC_REGNUM)
>
Thanks, Andreas.  I am not a specialist in 390 architecture.  You are 
the expert.  I'll check the bootstrap with this option too on a machine 
available for me.  I've checked the bootstrap without any -with-arch 
option before submitting the patch for approval.  If I find some 
problems, I'll work on them too.  I'll work on PR57599 you just reported 
too.
Vladimir Makarov - June 8, 2013, 6:41 p.m.
On 13-06-07 11:12 AM, Vladimir Makarov wrote:
> On 13-06-07 10:57 AM, Andreas Krebbel wrote:
>> I've applied the attached patch. This helps me getting a little
>> further when bootstrapping with lra and --with-arch=zEC12.
>>
>> 2013-06-07  Andreas Krebbel  <Andreas.Krebbel@de.ibm.com>
>>
>>     * config/s390/s390.md (cpu_facility): Add cpu_zarch.
>>     ("*movmem_short", "*clrmem_short", "*cmpmem_short): Use cpu_zarch
>>     for last alternative in the cpu_facility attribute.
>>
>> ---
>>   gcc/config/s390/s390.md |   13 ++++!!!!!!!!!
>>   1 file changed, 4 insertions(+), 9 modifications(!)
>>
>>
> Thanks, Andreas.  I am not a specialist in 390 architecture.  You are 
> the expert.  I'll check the bootstrap with this option too on a 
> machine available for me.  I've checked the bootstrap without any 
> -with-arch option before submitting the patch for approval. If I find 
> some problems, I'll work on them too.  I'll work on PR57599 you just 
> reported too.
>
I've fixed PR57599.  Unfortunately, I can not bootstrap with 
--with-arch=zEC12 as a machine available for me does not support this 
subtarget.
Andreas Krebbel - June 10, 2013, 8:52 a.m.
On 08/06/13 20:41, Vladimir Makarov wrote:
> On 13-06-07 11:12 AM, Vladimir Makarov wrote:
>> On 13-06-07 10:57 AM, Andreas Krebbel wrote:
>>> I've applied the attached patch. This helps me getting a little
>>> further when bootstrapping with lra and --with-arch=zEC12.
>>>
>>> 2013-06-07  Andreas Krebbel  <Andreas.Krebbel@de.ibm.com>
>>>
>>>     * config/s390/s390.md (cpu_facility): Add cpu_zarch.
>>>     ("*movmem_short", "*clrmem_short", "*cmpmem_short): Use cpu_zarch
>>>     for last alternative in the cpu_facility attribute.
>>>
>>> ---
>>>   gcc/config/s390/s390.md |   13 ++++!!!!!!!!!
>>>   1 file changed, 4 insertions(+), 9 modifications(!)
>>>
>>>
>> Thanks, Andreas.  I am not a specialist in 390 architecture.  You are 
>> the expert.  I'll check the bootstrap with this option too on a 
>> machine available for me.  I've checked the bootstrap without any 
>> -with-arch option before submitting the patch for approval. If I find 
>> some problems, I'll work on them too.  I'll work on PR57599 you just 
>> reported too.
>>
> I've fixed PR57599.  Unfortunately, I can not bootstrap with 
> --with-arch=zEC12 as a machine available for me does not support this 
> subtarget.

Thanks. I would be happy to test the patch for you.

Bye,

-Andreas-

Patch

Index: config/s390/s390.c
===================================================================
--- config/s390/s390.c	(revision 199453)
+++ config/s390/s390.c	(working copy)
@@ -2017,14 +2017,18 @@  s390_decompose_address (rtx addr, struct
 	 Thus we don't check the displacement for validity here.  If after
 	 elimination the displacement turns out to be invalid after all,
 	 this is fixed up by reload in any case.  */
-      if (base != arg_pointer_rtx
-	  && indx != arg_pointer_rtx
-	  && base != return_address_pointer_rtx
-	  && indx != return_address_pointer_rtx
-	  && base != frame_pointer_rtx
-	  && indx != frame_pointer_rtx
-	  && base != virtual_stack_vars_rtx
-	  && indx != virtual_stack_vars_rtx)
+      /* LRA maintains always displacements up to date and we need to
+	 know the displacement is right during all LRA not only at the
+	 final elimination.  */
+      if (lra_in_progress
+	  || (base != arg_pointer_rtx
+	      && indx != arg_pointer_rtx
+	      && base != return_address_pointer_rtx
+	      && indx != return_address_pointer_rtx
+	      && base != frame_pointer_rtx
+	      && indx != frame_pointer_rtx
+	      && base != virtual_stack_vars_rtx
+	      && indx != virtual_stack_vars_rtx))
 	if (!DISP_IN_RANGE (offset))
 	  return false;
     }
@@ -3189,7 +3193,9 @@  s390_secondary_reload (bool in_p, rtx x,
 
   /* We need a scratch register when loading a PLUS expression which
      is not a legitimate operand of the LOAD ADDRESS instruction.  */
-  if (in_p && s390_plus_operand (x, mode))
+  /* LRA can deal with transformation of plus op very well -- so we
+     don't need to prompt LRA in this case.  */
+  if (! lra_in_progress && in_p && s390_plus_operand (x, mode))
     sri->icode = (TARGET_64BIT ?
 		  CODE_FOR_reloaddi_plus : CODE_FOR_reloadsi_plus);
 
@@ -7868,6 +7874,13 @@  s390_class_max_nregs (enum reg_class rcl
   return (GET_MODE_SIZE (mode) + UNITS_PER_WORD - 1) / UNITS_PER_WORD;
 }
 
+/* Return true if we use LRA instead of reload pass.  */
+static bool
+s390_lra_p (void)
+{
+  return s390_lra_flag;
+}
+
 /* Return true if register FROM can be eliminated via register TO.  */
 
 static bool
@@ -11105,6 +11118,9 @@  s390_loop_unroll_adjust (unsigned nunrol
 #undef TARGET_LEGITIMATE_CONSTANT_P
 #define TARGET_LEGITIMATE_CONSTANT_P s390_legitimate_constant_p
 
+#undef TARGET_LRA_P
+#define TARGET_LRA_P s390_lra_p
+
 #undef TARGET_CAN_ELIMINATE
 #define TARGET_CAN_ELIMINATE s390_can_eliminate
 
Index: config/s390/s390.md
===================================================================
--- config/s390/s390.md	(revision 199453)
+++ config/s390/s390.md	(working copy)
@@ -2690,7 +2690,7 @@ 
   "(GET_MODE (operands[2]) == Pmode || GET_MODE (operands[2]) == VOIDmode)"
   "#"
   [(set_attr "type"         "cs")
-   (set_attr "cpu_facility" "*,*,z10,*")])
+   (set_attr "cpu_facility" "*,*,z10,zarch")])
 
 (define_split
   [(set (match_operand:BLK 0 "memory_operand" "")
@@ -2899,7 +2899,7 @@ 
   "(GET_MODE (operands[1]) == Pmode || GET_MODE (operands[1]) == VOIDmode)"
   "#"
   [(set_attr "type" "cs")
-   (set_attr "cpu_facility" "*,*,z10,*")])
+   (set_attr "cpu_facility" "*,*,z10,zarch")])
 
 (define_split
   [(set (match_operand:BLK 0 "memory_operand" "")
@@ -3075,7 +3075,7 @@ 
   "(GET_MODE (operands[2]) == Pmode || GET_MODE (operands[2]) == VOIDmode)"
   "#"
   [(set_attr "type" "cs")
-   (set_attr "cpu_facility" "*,*,z10,*")])
+   (set_attr "cpu_facility" "*,*,z10,zarch")])
 
 (define_split
   [(set (reg:CCU CC_REGNUM)
Index: config/s390/s390.opt
===================================================================
--- config/s390/s390.opt	(revision 199453)
+++ config/s390/s390.opt	(working copy)
@@ -149,3 +149,7 @@  Target Report Joined RejectNegative UInt
 Set the branch costs for conditional branch instructions.  Reasonable
 values are small, non-negative integers.  The default branch cost is
 1.
+
+mlra
+Target Report Var(s390_lra_flag) Init(1) Save
+Use LRA instead of reload