patch for PR61578

patch for PR61578

[Vladimir Makarov]

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

   The following patch solves the 2nd case of

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61578

   I did a lot of benchmarking of different heuristics in hard reg cost 
propagation in IRA.  This is the best what I found.  The patch improves 
stably code size of SPEC2000 and its score although it is not that 
significant.

   The patch was tested and bootstrapped on x86-64.

   Committed as rev. 228097.

2015-09-24  Vladimir Makarov  <vmakarov@redhat.com>

         PR target/61578
         * ira-color.c (update_allocno_cost): Add parameter.
         (update_costs_from_allocno): Decrease conflict cost.  Pass the new
         parameter.


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

Index: ira-color.c
===================================================================
--- ira-color.c	(revision 227495)
+++ ira-color.c	(working copy)
@@ -1311,10 +1311,12 @@ get_next_update_cost (ira_allocno_t *all
   return true;
 }
 
-/* Increase costs of HARD_REGNO by UPDATE_COST for ALLOCNO.  Return
-   true if we really modified the cost.  */
+/* Increase costs of HARD_REGNO by UPDATE_COST and conflict cost by
+   UPDATE_CONFLICT_COST for ALLOCNO.  Return true if we really
+   modified the cost.  */
 static bool
-update_allocno_cost (ira_allocno_t allocno, int hard_regno, int update_cost)
+update_allocno_cost (ira_allocno_t allocno, int hard_regno,
+		     int update_cost, int update_conflict_cost)
 {
   int i;
   enum reg_class aclass = ALLOCNO_CLASS (allocno);
@@ -1330,7 +1332,7 @@ update_allocno_cost (ira_allocno_t alloc
     (&ALLOCNO_UPDATED_CONFLICT_HARD_REG_COSTS (allocno),
      aclass, 0, ALLOCNO_CONFLICT_HARD_REG_COSTS (allocno));
   ALLOCNO_UPDATED_HARD_REG_COSTS (allocno)[i] += update_cost;
-  ALLOCNO_UPDATED_CONFLICT_HARD_REG_COSTS (allocno)[i] += update_cost;
+  ALLOCNO_UPDATED_CONFLICT_HARD_REG_COSTS (allocno)[i] += update_conflict_cost;
   return true;
 }
 
@@ -1342,7 +1344,7 @@ static void
 update_costs_from_allocno (ira_allocno_t allocno, int hard_regno,
 			   int divisor, bool decr_p, bool record_p)
 {
-  int cost, update_cost;
+  int cost, update_cost, update_conflict_cost;
   machine_mode mode;
   enum reg_class rclass, aclass;
   ira_allocno_t another_allocno, from = NULL;
@@ -1383,11 +1385,20 @@ update_costs_from_allocno (ira_allocno_t
 	  if (decr_p)
 	    cost = -cost;
 
-	  update_cost = cp->freq * cost / divisor;
+	  update_conflict_cost = update_cost = cp->freq * cost / divisor;
+
+	  if (ALLOCNO_COLOR_DATA (another_allocno) != NULL
+	      && (ALLOCNO_COLOR_DATA (allocno)->first_thread_allocno
+		  != ALLOCNO_COLOR_DATA (another_allocno)->first_thread_allocno))
+	    /* Decrease conflict cost of ANOTHER_ALLOCNO if it is not
+	       in the same allocation thread.  */
+	    update_conflict_cost /= COST_HOP_DIVISOR;
+
 	  if (update_cost == 0)
 	    continue;
 
-	  if (! update_allocno_cost (another_allocno, hard_regno, update_cost))
+	  if (! update_allocno_cost (another_allocno, hard_regno,
+				     update_cost, update_conflict_cost))
 	    continue;
 	  queue_update_cost (another_allocno, allocno, divisor * COST_HOP_DIVISOR);
 	  if (record_p && ALLOCNO_COLOR_DATA (another_allocno) != NULL)

Re: patch for PR61578

[Jeff Law]

On 09/24/2015 02:41 PM, Vladimir Makarov wrote:
>    The following patch solves the 2nd case of
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61578
>
>    I did a lot of benchmarking of different heuristics in hard reg cost
> propagation in IRA.  This is the best what I found.  The patch improves
> stably code size of SPEC2000 and its score although it is not that
> significant.
Didn't we have a handful of missed optimization BZs that we speculated 
might be helped by propagation of hard register costs?  Might not be a 
bad idea to review those and see if any are magically fixed now.

jeff

Re: patch for PR61578

[Jeff Law]

On 09/24/2015 02:41 PM, Vladimir Makarov wrote:
>    The following patch solves the 2nd case of
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61578
>
>    I did a lot of benchmarking of different heuristics in hard reg cost
> propagation in IRA.  This is the best what I found.  The patch improves
> stably code size of SPEC2000 and its score although it is not that
> significant.
>
>    The patch was tested and bootstrapped on x86-64.
>
>    Committed as rev. 228097.
>
> 2015-09-24  Vladimir Makarov  <vmakarov@redhat.com>
>
>          PR target/61578
>          * ira-color.c (update_allocno_cost): Add parameter.
>          (update_costs_from_allocno): Decrease conflict cost.  Pass the new
>          parameter.
FWIW, I did a quick scan for some of those old BZs.  None were improved. 
  Sigh.

jeff

Re: patch for PR61578

[Vladimir Makarov]

On 09/25/2015 12:57 PM, Jeff Law wrote:
> On 09/24/2015 02:41 PM, Vladimir Makarov wrote:
>>    The following patch solves the 2nd case of
>>
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61578
>>
>>    I did a lot of benchmarking of different heuristics in hard reg cost
>> propagation in IRA.  This is the best what I found.  The patch improves
>> stably code size of SPEC2000 and its score although it is not that
>> significant.
>>
>>    The patch was tested and bootstrapped on x86-64.
>>
>>    Committed as rev. 228097.
>>
>> 2015-09-24  Vladimir Makarov  <vmakarov@redhat.com>
>>
>>          PR target/61578
>>          * ira-color.c (update_allocno_cost): Add parameter.
>>          (update_costs_from_allocno): Decrease conflict cost. Pass 
>> the new
>>          parameter.
> FWIW, I did a quick scan for some of those old BZs.  None were 
> improved.  Sigh.
>
Thanks for checking them, Jeff.  I don't think the work on PR61578 has 
been finished.  I guess people will present other cases which should be 
improved and may be their solutions will solve the other PRs.

patch for PR61578

[Vladimir Makarov]

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

   The following patch is for

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61578

   The patch was bootstrapped and tested on x86 and x86-64.

   Committed as rev. 227382.

2015-09-01  Vladimir Makarov  <vmakarov@redhat.com>

         PR target/61578
         * lra-lives.c (process_bb_lives): Process move pseudos with the
         same value for copies and preferences
         * lra-constraints.c (match_reload): Create match reload pseudo
         with the same value from single dying input pseudo.


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

Index: lra-constraints.c
===================================================================
--- lra-constraints.c	(revision 227381)
+++ lra-constraints.c	(working copy)
@@ -928,10 +928,12 @@ match_reload (signed char out, signed ch
 	 they live in the same place.  When we create a pseudo we
 	 assign value of original pseudo (if any) from which we
 	 created the new pseudo.  If we create the pseudo from the
-	 input pseudo, the new pseudo will no conflict with the input
-	 pseudo which is wrong when the input pseudo lives after the
-	 insn and as the new pseudo value is changed by the insn
-	 output.  Therefore we create the new pseudo from the output.
+	 input pseudo, the new pseudo will have no conflict with the
+	 input pseudo which is wrong when the input pseudo lives after
+	 the insn and as the new pseudo value is changed by the insn
+	 output.  Therefore we create the new pseudo from the output
+	 except the case when we have single matched dying input
+	 pseudo.
 
 	 We cannot reuse the current output register because we might
 	 have a situation like "a <- a op b", where the constraints
@@ -940,8 +942,12 @@ match_reload (signed char out, signed ch
 	 so that it doesn't clobber the current value of "a".  */
 
       new_in_reg = new_out_reg
-	= lra_create_new_reg_with_unique_value (outmode, out_rtx,
-						goal_class, "");
+	= (ins[1] < 0 && REG_P (in_rtx)
+	   && (int) REGNO (in_rtx) < lra_new_regno_start
+	   && find_regno_note (curr_insn, REG_DEAD, REGNO (in_rtx))
+	   ? lra_create_new_reg (inmode, in_rtx, goal_class, "")
+	   : lra_create_new_reg_with_unique_value (outmode, out_rtx,
+						   goal_class, ""));
     }
   /* In operand can be got from transformations before processing insn
      constraints.  One example of such transformations is subreg
Index: lra-lives.c
===================================================================
--- lra-lives.c	(revision 227381)
+++ lra-lives.c	(working copy)
@@ -726,28 +726,33 @@ process_bb_lives (basic_block bb, int &c
 	  lra_hard_reg_usage[reg->regno] += freq;
 
       call_p = CALL_P (curr_insn);
+      src_regno = (set != NULL_RTX && REG_P (SET_SRC (set))
+		   ? REGNO (SET_SRC (set)) : -1);
+      dst_regno = (set != NULL_RTX && REG_P (SET_DEST (set))
+		   ? REGNO (SET_DEST (set)) : -1);
       if (complete_info_p
-	  && set != NULL_RTX
-	  && REG_P (SET_DEST (set)) && REG_P (SET_SRC (set))
+	  && src_regno >= 0 && dst_regno >= 0
 	  /* Check that source regno does not conflict with
 	     destination regno to exclude most impossible
 	     preferences.  */
-	  && ((((src_regno = REGNO (SET_SRC (set))) >= FIRST_PSEUDO_REGISTER
-		&& ! sparseset_bit_p (pseudos_live, src_regno))
+	  && (((src_regno >= FIRST_PSEUDO_REGISTER
+		&& (! sparseset_bit_p (pseudos_live, src_regno)
+		    || (dst_regno >= FIRST_PSEUDO_REGISTER
+			&& lra_reg_val_equal_p (src_regno,
+						lra_reg_info[dst_regno].val,
+						lra_reg_info[dst_regno].offset))))
 	       || (src_regno < FIRST_PSEUDO_REGISTER
 		   && ! TEST_HARD_REG_BIT (hard_regs_live, src_regno)))
 	      /* It might be 'inheritance pseudo <- reload pseudo'.  */
 	      || (src_regno >= lra_constraint_new_regno_start
-		  && ((int) REGNO (SET_DEST (set))
-		      >= lra_constraint_new_regno_start)
+		  && dst_regno >= lra_constraint_new_regno_start
 		  /* Remember to skip special cases where src/dest regnos are
 		     the same, e.g. insn SET pattern has matching constraints
 		     like =r,0.  */
-		  && src_regno != (int) REGNO (SET_DEST (set)))))
+		  && src_regno != dst_regno)))
 	{
 	  int hard_regno = -1, regno = -1;
 
-	  dst_regno = REGNO (SET_DEST (set));
 	  if (dst_regno >= lra_constraint_new_regno_start
 	      && src_regno >= lra_constraint_new_regno_start)
 	    {

Re: patch for PR61578

[Christophe Lyon]

Hi Vladimir,



On 1 September 2015 at 21:39, Vladimir Makarov <vmakarov@redhat.com> wrote:
>   The following patch is for
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61578
>
>   The patch was bootstrapped and tested on x86 and x86-64.
>
>   Committed as rev. 227382.
>

Since this patch, I can see:
  gcc.dg/vect/slp-perm-5.c (internal compiler error)
  gcc.dg/vect/slp-perm-5.c -flto -ffat-lto-objects (internal compiler error)

on arm* targets.

Can you have a look?

Thanks,

Christophe.


> 2015-09-01  Vladimir Makarov  <vmakarov@redhat.com>
>
>         PR target/61578
>         * lra-lives.c (process_bb_lives): Process move pseudos with the
>         same value for copies and preferences
>         * lra-constraints.c (match_reload): Create match reload pseudo
>         with the same value from single dying input pseudo.
>

Re: patch for PR61578

[Vladimir Makarov]

On 09/02/2015 11:32 AM, Christophe Lyon wrote:
> Hi Vladimir,
>
>
>
> On 1 September 2015 at 21:39, Vladimir Makarov <vmakarov@redhat.com> wrote:
>>    The following patch is for
>>
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61578
>>
>>    The patch was bootstrapped and tested on x86 and x86-64.
>>
>>    Committed as rev. 227382.
>>
> Since this patch, I can see:
>    gcc.dg/vect/slp-perm-5.c (internal compiler error)
>    gcc.dg/vect/slp-perm-5.c -flto -ffat-lto-objects (internal compiler error)
>
> on arm* targets.
>
> Can you have a look?
>
>
Thanks for reporting this, Christophe.

Sure, I'll investigate it.  It is my fault I should fix it.

Re: patch for PR61578

[Vladimir Makarov]

On 09/03/2015 11:00 AM, Vladimir Makarov wrote:
> On 09/02/2015 11:32 AM, Christophe Lyon wrote:
>> Hi Vladimir,
>>
>>
>>
>> On 1 September 2015 at 21:39, Vladimir Makarov <vmakarov@redhat.com> 
>> wrote:
>>>    The following patch is for
>>>
>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61578
>>>
>>>    The patch was bootstrapped and tested on x86 and x86-64.
>>>
>>>    Committed as rev. 227382.
>>>
>> Since this patch, I can see:
>>    gcc.dg/vect/slp-perm-5.c (internal compiler error)
>>    gcc.dg/vect/slp-perm-5.c -flto -ffat-lto-objects (internal 
>> compiler error)
>>
>> on arm* targets.
>>
Christophe, I can not reproduce it on my arm board (odroid xu4). Could 
you provide more info (target, configure options).  Thanks.

Re: patch for PR61578

[Christophe Lyon]

On 3 September 2015 at 20:28, Vladimir Makarov <vmakarov@redhat.com> wrote:
> On 09/03/2015 11:00 AM, Vladimir Makarov wrote:
>>
>> On 09/02/2015 11:32 AM, Christophe Lyon wrote:
>>>
>>> Hi Vladimir,
>>>
>>>
>>>
>>> On 1 September 2015 at 21:39, Vladimir Makarov <vmakarov@redhat.com>
>>> wrote:
>>>>
>>>>    The following patch is for
>>>>
>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61578
>>>>
>>>>    The patch was bootstrapped and tested on x86 and x86-64.
>>>>
>>>>    Committed as rev. 227382.
>>>>
>>> Since this patch, I can see:
>>>    gcc.dg/vect/slp-perm-5.c (internal compiler error)
>>>    gcc.dg/vect/slp-perm-5.c -flto -ffat-lto-objects (internal compiler
>>> error)
>>>
>>> on arm* targets.
>>>
> Christophe, I can not reproduce it on my arm board (odroid xu4). Could you
> provide more info (target, configure options).  Thanks.
>
For instance, you can try
--target=arm-none-linux-gnueabihf --with-mode=arm --with-cpu=cortex-a9
--with-fpu=neon-fp16

Re: patch for PR61578

[Dominik Vogt]

On Tue, Sep 01, 2015 at 03:39:19PM -0400, Vladimir Makarov wrote:
>   The following patch is for
> 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61578
> 
>   The patch was bootstrapped and tested on x86 and x86-64.
> 
>   Committed as rev. 227382.
> 
> 2015-09-01  Vladimir Makarov  <vmakarov@redhat.com>
> 
>         PR target/61578
>         * lra-lives.c (process_bb_lives): Process move pseudos with the
>         same value for copies and preferences
>         * lra-constraints.c (match_reload): Create match reload pseudo
>         with the same value from single dying input pseudo.

This check-in caused a regression on s390, please see
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61578 for details.

Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany