public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* IRA costs tweaks, PR 56069
       [not found] <56D71977.5040204@t-online.de>
@ 2016-03-02 17:30 ` Bernd Schmidt
  2016-03-02 21:53   ` Vladimir Makarov
  0 siblings, 1 reply; 9+ messages in thread
From: Bernd Schmidt @ 2016-03-02 17:30 UTC (permalink / raw)
  To: GCC Patches, Vladimir Makarov

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

I had a look at the costs issues in this PR. I think I've found a fair 
number of bugs, but fixing those alone doesn't solve the issue; one 
additional tweak is needed.

As for the bugs, they are primarily in the mechanism of recording cost 
updates and restoring them. When adjusting costs for preferences and 
copies, we create records of the adjustments we made. At the end of 
assign_hard_reg, when we have assigned a hard register, we try to undo 
these changes (through restore_costs_from_copies), and then call 
update_costs_from_copies again with the real assigned hard register 
number so that future allocations take that into account.

The issues I can see are:

1. restore_costs_from_copies passes true for decr_p, which means costs 
aren't restored, but doubled instead.

2. update_costs_from_allocno records a cost update not just for the 
initial allocno, but for each of the visited ones. I can sort of see an 
argument for doing that (let's say if you assign an allocno in the 
middle of a copy chain you'd want the tail end of the chain to be 
reset), but in practice I don't think the present algorithm can work at 
all. In the case of an allocno in the middle of a copy chain the restore 
would progress in both directions, and in any case it looks like this 
approach can end up double-counting things when restoring costs.

3. As far as I can tell, in update_costs_from_prefs, we adjust costs for 
all allocnos connected to the one with the pref, but not for the initial 
one.

The patch below corrects these. Issue #2 has no clearly best solution; 
in this patch I've just moved the recording of the cost out of the loop 
so it's done only for the initial allocno. The code is a little 
convoluted so as to prevent skipping restorations if allocnos in a copy 
chain have already been allocated. I have an alternative patch that more 
directly records actual cost updates caused to other allocnos for a 
given one. That variant is a little more clear, but consumes a bit more 
memory. I can post that as well if desired.

As for the cost tweak, Vlad mentioned the effect of copies for 
commutative operands in the PR. I ended up dividing the frequency of 
such copies by two. (Another way to solve the PR is to reduce the 
initial divisor for preference updates, but that seems a little more 
hackish).

The overall effect of this patch seems to be fairly minimal in practice, 
which is slightly disappointing. On the whole, it seems to be a very 
slight net positive, with few instances where we generate additional 
instructions.

Ok (in full or in parts, now or for stage1)?


Bernd

[-- Attachment #2: ira-costs-v5.diff --]
[-- Type: text/x-patch, Size: 5804 bytes --]

	* ira-color.c (record_update_cost): New function, replacing...
	(get_update_cost_record): ... this, removed.
	(update_allocno_cost): Return false if allocno has been assigned.
	(update_costs_from_allocno): Call record_udpate_cost once for
	the initial allocno. Don't record update costs for allocnos
	found while following copies.
	(update_costs_from_prefs): Adjust costs of the initial allocno.
	(restore_costs_from_copies): Fix call to update_costs_from_allocno
	to undo previous changes.
	* ira-conflicts.c (add_insn_allocno_copies): Divide freq by two
	for commutative operands.

Index: gcc/ira-color.c
===================================================================
--- gcc/ira-color.c	(revision 233451)
+++ gcc/ira-color.c	(working copy)
@@ -1146,18 +1146,17 @@ setup_profitable_hard_regs (void)
 static object_allocator<update_cost_record> update_cost_record_pool
   ("update cost records");
 
-/* Return new update cost record with given params.  */
-static struct update_cost_record *
-get_update_cost_record (int hard_regno, int divisor,
-			struct update_cost_record *next)
+/* Create a new update cost record in DATA with given params.  */
+static void
+record_update_cost (allocno_color_data_t data, int hard_regno, int divisor)
 {
   struct update_cost_record *record;
 
   record = update_cost_record_pool.allocate ();
   record->hard_regno = hard_regno;
   record->divisor = divisor;
-  record->next = next;
-  return record;
+  record->next = data->update_cost_records;
+  data->update_cost_records = record;
 }
 
 /* Free memory for all records in LIST.  */
@@ -1305,6 +1304,9 @@ static bool
 update_allocno_cost (ira_allocno_t allocno, int hard_regno,
 		     int update_cost, int update_conflict_cost)
 {
+  if (ALLOCNO_ASSIGNED_P (allocno))
+    return false;
+
   int i;
   enum reg_class aclass = ALLOCNO_CLASS (allocno);
 
@@ -1337,6 +1339,9 @@ update_costs_from_allocno (ira_allocno_t
   ira_allocno_t another_allocno, from = NULL;
   ira_copy_t cp, next_cp;
 
+  if (record_p && ALLOCNO_COLOR_DATA (allocno) != NULL)
+    record_update_cost (ALLOCNO_COLOR_DATA (allocno), hard_regno, divisor);
+
   rclass = REGNO_REG_CLASS (hard_regno);
   do
     {
@@ -1361,9 +1366,11 @@ update_costs_from_allocno (ira_allocno_t
 	    continue;
 
 	  aclass = ALLOCNO_CLASS (another_allocno);
-	  if (! TEST_HARD_REG_BIT (reg_class_contents[aclass],
-				   hard_regno)
-	      || ALLOCNO_ASSIGNED_P (another_allocno))
+	  if (!TEST_HARD_REG_BIT (reg_class_contents[aclass], hard_regno)
+	      || (ALLOCNO_ASSIGNED_P (another_allocno)
+		  /* When restoring, don't let assigned allocnos block us
+		     from undoing the entire set of changes.  */
+		  && decr_p))
 	    continue;
 
 	  cost = (cp->second == allocno
@@ -1384,15 +1391,16 @@ update_costs_from_allocno (ira_allocno_t
 	  if (update_cost == 0)
 	    continue;
 
-	  if (! update_allocno_cost (another_allocno, hard_regno,
-				     update_cost, update_conflict_cost))
+	  /* Update the cost, stopping at this point if that fails.
+	     As above, don't stop if incrementing for restoring costs,
+	     thus we want to keep going if the allocno was already
+	     assigned and we're incrementing costs.  */
+	  if (!update_allocno_cost (another_allocno, hard_regno,
+				    update_cost, update_conflict_cost)
+	      && !(ALLOCNO_ASSIGNED_P (another_allocno) && !decr_p))
 	    continue;
-	  queue_update_cost (another_allocno, allocno, divisor * COST_HOP_DIVISOR);
-	  if (record_p && ALLOCNO_COLOR_DATA (another_allocno) != NULL)
-	    ALLOCNO_COLOR_DATA (another_allocno)->update_cost_records
-	      = get_update_cost_record (hard_regno, divisor,
-					ALLOCNO_COLOR_DATA (another_allocno)
-					->update_cost_records);
+	  queue_update_cost (another_allocno, allocno,
+			     divisor * COST_HOP_DIVISOR);
 	}
     }
   while (get_next_update_cost (&allocno, &from, &divisor));
@@ -1407,8 +1415,16 @@ update_costs_from_prefs (ira_allocno_t a
 
   start_update_cost ();
   for (pref = ALLOCNO_PREFS (allocno); pref != NULL; pref = pref->next_pref)
-    update_costs_from_allocno (allocno, pref->hard_regno,
-			       COST_HOP_DIVISOR, true, true);
+    {
+      reg_class rclass = REGNO_REG_CLASS (pref->hard_regno);
+      reg_class aclass = ALLOCNO_CLASS (allocno);
+      machine_mode mode = ALLOCNO_MODE (allocno);
+      int cost = ira_register_move_cost[mode][rclass][aclass];
+      cost *= pref->freq * -1;
+      if (update_allocno_cost (allocno, pref->hard_regno, cost, true))
+	update_costs_from_allocno (allocno, pref->hard_regno, COST_HOP_DIVISOR,
+				   true, true);
+    }
 }
 
 /* Update (decrease if DECR_P) the cost of allocnos connected to
@@ -1443,7 +1459,7 @@ restore_costs_from_copies (ira_allocno_t
   start_update_cost ();
   for (curr = records; curr != NULL; curr = curr->next)
     update_costs_from_allocno (allocno, curr->hard_regno,
-			       curr->divisor, true, false);
+			       curr->divisor, false, false);
   free_update_cost_record_list (records);
   ALLOCNO_COLOR_DATA (allocno)->update_cost_records = NULL;
 }
Index: gcc/ira-conflicts.c
===================================================================
--- gcc/ira-conflicts.c	(revision 233451)
+++ gcc/ira-conflicts.c	(working copy)
@@ -383,6 +383,9 @@ add_insn_allocno_copies (rtx_insn *insn)
       operand = recog_data.operand[i];
       if (! REG_SUBREG_P (operand))
 	continue;
+      int adj_freq = freq;
+      if (recog_data.constraints[i][0] == '%')
+	adj_freq /= 2;
       if ((n = ira_get_dup_out_num (i, alts)) >= 0)
 	{
 	  bound_p[n] = true;
@@ -393,7 +396,7 @@ add_insn_allocno_copies (rtx_insn *insn)
 				? operand
 				: SUBREG_REG (operand)) != NULL_RTX)
 	    process_regs_for_copy (operand, dup, true, NULL,
-				   freq);
+				   adj_freq);
 	}
     }
   for (i = 0; i < recog_data.n_operands; i++)


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: IRA costs tweaks, PR 56069
  2016-03-02 17:30 ` IRA costs tweaks, PR 56069 Bernd Schmidt
@ 2016-03-02 21:53   ` Vladimir Makarov
  2016-03-03 17:45     ` Bernd Schmidt
                       ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Vladimir Makarov @ 2016-03-02 21:53 UTC (permalink / raw)
  To: Bernd Schmidt, GCC Patches

On 03/02/2016 12:29 PM, Bernd Schmidt wrote:
> I had a look at the costs issues in this PR. I think I've found a fair 
> number of bugs, but fixing those alone doesn't solve the issue; one 
> additional tweak is needed.
>
> As for the bugs, they are primarily in the mechanism of recording cost 
> updates and restoring them. When adjusting costs for preferences and 
> copies, we create records of the adjustments we made. At the end of 
> assign_hard_reg, when we have assigned a hard register, we try to undo 
> these changes (through restore_costs_from_copies), and then call 
> update_costs_from_copies again with the real assigned hard register 
> number so that future allocations take that into account.
>
> The issues I can see are:
>
> 1. restore_costs_from_copies passes true for decr_p, which means costs 
> aren't restored, but doubled instead.
>
> 2. update_costs_from_allocno records a cost update not just for the 
> initial allocno, but for each of the visited ones. I can sort of see 
> an argument for doing that (let's say if you assign an allocno in the 
> middle of a copy chain you'd want the tail end of the chain to be 
> reset), but in practice I don't think the present algorithm can work 
> at all. In the case of an allocno in the middle of a copy chain the 
> restore would progress in both directions, and in any case it looks 
> like this approach can end up double-counting things when restoring 
> costs.
>
It is just a heuristic.  Richard Sandiford proposed this update 
approach.  Before it we had only updates of allocnos directly connected 
to allocno in question.  Richard's approach helped to improve code in 
some cases.  If something works better we should use.  The bechmarking 
is the best criterium.
> 3. As far as I can tell, in update_costs_from_prefs, we adjust costs 
> for all allocnos connected to the one with the pref, but not for the 
> initial one.
>
> The patch below corrects these. Issue #2 has no clearly best solution; 
> in this patch I've just moved the recording of the cost out of the 
> loop so it's done only for the initial allocno. The code is a little 
> convoluted so as to prevent skipping restorations if allocnos in a 
> copy chain have already been allocated. I have an alternative patch 
> that more directly records actual cost updates caused to other 
> allocnos for a given one. That variant is a little more clear, but 
> consumes a bit more memory. I can post that as well if desired.
>
> As for the cost tweak, Vlad mentioned the effect of copies for 
> commutative operands in the PR. I ended up dividing the frequency of 
> such copies by two. (Another way to solve the PR is to reduce the 
> initial divisor for preference updates, but that seems a little more 
> hackish).
>
> The overall effect of this patch seems to be fairly minimal in 
> practice, which is slightly disappointing. On the whole, it seems to 
> be a very slight net positive, with few instances where we generate 
> additional instructions.
>
I am just speculating may be the most important thing for performance is 
updating costs not their exact values (although the values can be 
important in some cases too).
> Ok (in full or in parts, now or for stage1)?
>
The patch is ok for me.  But I'd wait for GCC7.  People are sensitive to 
their code performance degradation.  Even in most cases the patch 
improves performance, in some case it can worsen code and people might 
fill new PRs after such change.

Bernd, thanks for working on it and providing a fresh view of the code.  
For me especially valuable when people benchmark their patches.  
Sometimes I have to do it by myself.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: IRA costs tweaks, PR 56069
  2016-03-02 21:53   ` Vladimir Makarov
@ 2016-03-03 17:45     ` Bernd Schmidt
  2016-03-07 20:29       ` Richard Sandiford
  2016-04-25  9:11     ` Bernd Schmidt
  2016-04-27  3:57     ` Jeff Law
  2 siblings, 1 reply; 9+ messages in thread
From: Bernd Schmidt @ 2016-03-03 17:45 UTC (permalink / raw)
  To: Vladimir Makarov, GCC Patches, Richard Sandiford

On 03/02/2016 10:53 PM, Vladimir Makarov wrote:
>> 2. update_costs_from_allocno records a cost update not just for the
>> initial allocno, but for each of the visited ones. I can sort of see
>> an argument for doing that (let's say if you assign an allocno in the
>> middle of a copy chain you'd want the tail end of the chain to be
>> reset), but in practice I don't think the present algorithm can work
>> at all. In the case of an allocno in the middle of a copy chain the
>> restore would progress in both directions, and in any case it looks
>> like this approach can end up double-counting things when restoring
>> costs.
>>
> It is just a heuristic.  Richard Sandiford proposed this update
> approach.  Before it we had only updates of allocnos directly connected
> to allocno in question.  Richard's approach helped to improve code in
> some cases.  If something works better we should use.  The bechmarking
> is the best criterium.

Ccing Richard in case he has comments.

> The patch is ok for me.  But I'd wait for GCC7.  People are sensitive to
> their code performance degradation.  Even in most cases the patch
> improves performance, in some case it can worsen code and people might
> fill new PRs after such change.
>
> Bernd, thanks for working on it and providing a fresh view of the code.
> For me especially valuable when people benchmark their patches.
> Sometimes I have to do it by myself.

Ok, I'll wait. I did not actually run any benchmarks either, but I 
looked at generated code for a large number of examples. Based on that I 
suspect the effect would be too small to detect with benchmark runs.


Bernd

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: IRA costs tweaks, PR 56069
  2016-03-03 17:45     ` Bernd Schmidt
@ 2016-03-07 20:29       ` Richard Sandiford
  0 siblings, 0 replies; 9+ messages in thread
From: Richard Sandiford @ 2016-03-07 20:29 UTC (permalink / raw)
  To: Bernd Schmidt; +Cc: Vladimir Makarov, GCC Patches

Bernd Schmidt <bschmidt@redhat.com> writes:
> On 03/02/2016 10:53 PM, Vladimir Makarov wrote:
>>> 2. update_costs_from_allocno records a cost update not just for the
>>> initial allocno, but for each of the visited ones. I can sort of see
>>> an argument for doing that (let's say if you assign an allocno in the
>>> middle of a copy chain you'd want the tail end of the chain to be
>>> reset), but in practice I don't think the present algorithm can work
>>> at all. In the case of an allocno in the middle of a copy chain the
>>> restore would progress in both directions, and in any case it looks
>>> like this approach can end up double-counting things when restoring
>>> costs.
>>>
>> It is just a heuristic.  Richard Sandiford proposed this update
>> approach.  Before it we had only updates of allocnos directly connected
>> to allocno in question.  Richard's approach helped to improve code in
>> some cases.  If something works better we should use.  The bechmarking
>> is the best criterium.
>
> Ccing Richard in case he has comments.

TBH I don't remember anything about this now.  Is it:

  https://gcc.gnu.org/ml/gcc-patches/2008-09/msg00541.html

?  I think that was just tweaking the traversal order in an existing
cost update, rather than adding a new one.

You might be talking about a different patch though, sorry.

Richard

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: IRA costs tweaks, PR 56069
  2016-03-02 21:53   ` Vladimir Makarov
  2016-03-03 17:45     ` Bernd Schmidt
@ 2016-04-25  9:11     ` Bernd Schmidt
  2016-04-27  4:02       ` Jeff Law
  2016-04-27  3:57     ` Jeff Law
  2 siblings, 1 reply; 9+ messages in thread
From: Bernd Schmidt @ 2016-04-25  9:11 UTC (permalink / raw)
  To: Vladimir Makarov, GCC Patches; +Cc: Jakub Jelinek

On 03/02/2016 10:53 PM, Vladimir Makarov wrote:
>>
> The patch is ok for me.  But I'd wait for GCC7.  People are sensitive to
> their code performance degradation.  Even in most cases the patch
> improves performance, in some case it can worsen code and people might
> fill new PRs after such change.

I've retested now, and it turns out that it turns two guality/sra-1.c 
tests into UNSUPPORTED. Apparently a variable is now destructively 
overwritten. Is that something we worry about (I'm inclined to think no)?


Bernd

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: IRA costs tweaks, PR 56069
  2016-03-02 21:53   ` Vladimir Makarov
  2016-03-03 17:45     ` Bernd Schmidt
  2016-04-25  9:11     ` Bernd Schmidt
@ 2016-04-27  3:57     ` Jeff Law
  2 siblings, 0 replies; 9+ messages in thread
From: Jeff Law @ 2016-04-27  3:57 UTC (permalink / raw)
  To: Vladimir Makarov, Bernd Schmidt, GCC Patches

On 03/02/2016 02:53 PM, Vladimir Makarov wrote:
>>
>> The overall effect of this patch seems to be fairly minimal in
>> practice, which is slightly disappointing. On the whole, it seems to
>> be a very slight net positive, with few instances where we generate
>> additional instructions.
>>
> I am just speculating may be the most important thing for performance is
> updating costs not their exact values (although the values can be
> important in some cases too).
I suspect you're right -- when I was looking at handling reload via 
splitting and calling back into IRA, cost updates were key to getting 
good code for the newly created pseudos.   It wasn't a matter of getting 
them perfect, but a matter of getting them "reasonable".

jeff

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: IRA costs tweaks, PR 56069
  2016-04-25  9:11     ` Bernd Schmidt
@ 2016-04-27  4:02       ` Jeff Law
  2016-04-27 11:33         ` Bernd Schmidt
  0 siblings, 1 reply; 9+ messages in thread
From: Jeff Law @ 2016-04-27  4:02 UTC (permalink / raw)
  To: Bernd Schmidt, Vladimir Makarov, GCC Patches; +Cc: Jakub Jelinek

On 04/25/2016 03:11 AM, Bernd Schmidt wrote:
> On 03/02/2016 10:53 PM, Vladimir Makarov wrote:
>>>
>> The patch is ok for me.  But I'd wait for GCC7.  People are sensitive to
>> their code performance degradation.  Even in most cases the patch
>> improves performance, in some case it can worsen code and people might
>> fill new PRs after such change.
>
> I've retested now, and it turns out that it turns two guality/sra-1.c
> tests into UNSUPPORTED. Apparently a variable is now destructively
> overwritten. Is that something we worry about (I'm inclined to think no)?
We'd like to not go backwards if we can.

AFAICT the sra-1.c expects to see the incremented value and I'm at a 
loss to understand what's really going on here.  Can you give more details?

jeff

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: IRA costs tweaks, PR 56069
  2016-04-27  4:02       ` Jeff Law
@ 2016-04-27 11:33         ` Bernd Schmidt
  2016-06-23 15:51           ` Jeff Law
  0 siblings, 1 reply; 9+ messages in thread
From: Bernd Schmidt @ 2016-04-27 11:33 UTC (permalink / raw)
  To: Jeff Law, Vladimir Makarov, GCC Patches; +Cc: Jakub Jelinek

On 04/27/2016 06:02 AM, Jeff Law wrote:
> AFAICT the sra-1.c expects to see the incremented value and I'm at a
> loss to understand what's really going on here.  Can you give more details?

Yeah, maybe my first impression wasn't very accurate.

When I try to run gdb manually, it just crashes:

(gdb) show version
GNU gdb (Gentoo 7.10.1 vanilla) 7.10.1
(gdb) b 43
Breakpoint 1 at 0x40059b: file sra-1.c, line 43.
(gdb) run
Starting program: /local/src/egcs/bscommit/gcc/a.out

Breakpoint 1, f3 (k=<optimized out>) at sra-1.c:43
43	  bar (a.j);		/* { dg-final { gdb-test 43 "a.j" "14" } } */
(gdb) p a.j
Segmentation fault (core dumped)

Here's rtl from the final dump (reg notes and insn codes etc. removed 
where it seemed to help readability):

(note 49 21 39 2 (var_location a$i (const_int 4 [0x4])) 
NOTE_INSN_VAR_LOCATION)
(insn:TI 39 49 2 2 (set (reg:HI 0 ax [orig:97 a$i ] [97])
         (const_int 4 [0x4])) sra-1.c:40

(insn 2 39 50 2 (set (reg/v:SI 1 dx [orig:96 k ] [96])
         (reg:SI 5 di [ k ])) sra-1.c:38

(note 50 2 12 2 (var_location a$j (plus:HI (reg:HI 1 dx [orig:96 k ] [96])
     (const_int 6 [0x6]))) NOTE_INSN_VAR_LOCATION)

(insn:TI 12 50 51 2 (parallel [
             (set (reg:HI 0 ax [orig:97 a$i ] [97])
                 (asm_operands:HI ("") ("=r") 0 [
                         (reg:HI 0 ax [orig:97 a$i ] [97])
                     ]
                      [
                         (asm_input:HI ("0") sra-1.c:40)
                     ]
                      [] sra-1.c:40))
             (clobber (reg:CCFP 18 fpsr))
             (clobber (reg:CC 17 flags))
         ]) sra-1.c:40 -1

(note 51 12 52 2 (var_location a$i (reg:HI 0 ax [orig:97 a$i ] [97])) 
NOTE_INSN_VAR_LOCATION)
(note 52 51 15 2 (var_location a$j (plus:HI (reg:HI 1 dx [orig:96 k ] [96])
     (const_int 7 [0x7]))) NOTE_INSN_VAR_LOCATION)
(insn:TI 15 52 16 2 (set (reg:SI 2 cx [orig:92 _10 ] [92])
         (sign_extend:SI (reg:HI 0 ax [orig:97 a$i ] [97]))) sra-1.c:42

(insn:TI 16 15 53 2 (set (reg:SI 5 di)
         (reg:SI 2 cx [orig:92 _10 ] [92])) sra-1.c:42 86

(note 53 16 17 2 (var_location k (reg/v:SI 1 dx [orig:96 k ] [96])) 
NOTE_INSN_VAR_LOCATION)

(call_insn:TI 17 53 54 2 (call (mem:QI (symbol_ref:DI ("bar")))

(note 54 17 41 2 (expr_list:REG_DEP_TRUE (concat:SI (reg:SI 5 di)
         (reg:SI 2 cx [orig:92 _10 ] [92]))
     (nil)) NOTE_INSN_CALL_ARG_LOCATION)
(insn:TI 41 54 55 2 (parallel [
             (set (reg:SI 1 dx [101])
                 (plus:SI (reg:SI 1 dx [orig:96 k ] [96])
                     (const_int 7 [0x7])))
             (clobber (reg:CC 17 flags))
         ]) sra-1.c:41 218 {*addsi_1}
(note 55 41 56 2 (var_location k (plus:SI (reg:SI 1 dx [101])
     (const_int -7 [0xfffffffffffffff9]))) NOTE_INSN_VAR_LOCATION)
(note 56 55 42 2 (var_location a$j (reg:HI 1 dx [101])) 
NOTE_INSN_VAR_LOCATION)
(insn:TI 42 56 57 2 (parallel [
             (set (reg:SI 1 dx [103])
                 (ashift:SI (reg:SI 1 dx [101])
                     (const_int 4 [0x4])))
             (clobber (reg:CC 17 flags))
         ]) sra-1.c:41

(note 57 42 58 2 (var_location k (entry_value:SI (reg:SI 5 di [ k ]))) 
NOTE_INSN_VAR_LOCATION)
(note 58 57 23 2 (var_location a$j (plus:HI (subreg:HI (entry_value:SI 
(reg:SI 5 di [ k ])) 0)
     (const_int 7 [0x7]))) NOTE_INSN_VAR_LOCATION)

(insn:TI 23 58 24 2 (parallel [
             (set (reg:HI 1 dx [104])
                 (ashiftrt:HI (reg:HI 1 dx [103])
                     (const_int 4 [0x4])))
             (clobber (reg:CC 17 flags))
         ]) sra-1.c:41
(insn:TI 24 23 59 2 (set (reg:SI 0 ax [orig:93 _12 ] [93])
         (sign_extend:SI (reg:HI 1 dx [104]))) sra-1.c:43

(note 59 24 25 2 (var_location a$i (reg:HI 2 cx [orig:92 _10 ] [92])) 
NOTE_INSN_VAR_LOCATION)
(insn:TI 25 59 26 2 (set (reg:SI 5 di)
         (reg:SI 0 ax [orig:93 _12 ] [93])) sra-1.c:43 86 {*movsi_internal}
      (nil))

(call_insn:TI 26 25 60 2 (call (mem:QI (symbol_ref:DI ("bar")))


I don't really understand the var-tracking stuff too well, so no idea 
where to go from here. I suppose I'm withdrawing my patch.


Bernd

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: IRA costs tweaks, PR 56069
  2016-04-27 11:33         ` Bernd Schmidt
@ 2016-06-23 15:51           ` Jeff Law
  0 siblings, 0 replies; 9+ messages in thread
From: Jeff Law @ 2016-06-23 15:51 UTC (permalink / raw)
  To: Bernd Schmidt, Vladimir Makarov, GCC Patches; +Cc: Jakub Jelinek

On 04/27/2016 05:33 AM, Bernd Schmidt wrote:
> On 04/27/2016 06:02 AM, Jeff Law wrote:
>> AFAICT the sra-1.c expects to see the incremented value and I'm at a
>> loss to understand what's really going on here.  Can you give more
>> details?
>
> Yeah, maybe my first impression wasn't very accurate.
>
> When I try to run gdb manually, it just crashes:
>
> (gdb) show version
> GNU gdb (Gentoo 7.10.1 vanilla) 7.10.1
> (gdb) b 43
> Breakpoint 1 at 0x40059b: file sra-1.c, line 43.
> (gdb) run
> Starting program: /local/src/egcs/bscommit/gcc/a.out
>
> Breakpoint 1, f3 (k=<optimized out>) at sra-1.c:43
> 43      bar (a.j);        /* { dg-final { gdb-test 43 "a.j" "14" } } */
> (gdb) p a.j
> Segmentation fault (core dumped)
>
[ ... ]

>
>
> I don't really understand the var-tracking stuff too well, so no idea
> where to go from here. I suppose I'm withdrawing my patch.
Based on the above, there's some kind of GDB bug.  So your patch may 
still be a good thing.

I did a build on F23 which has effectively the same version of gdb and 
can reproduce the gdb segfault.  It also reproduces on F24 which has 
gdb-7.11.1

AFAICT gdb thinks the value of "a" has been optimized out, but goes 
absolutely bananas and segfaults if you try to examine a field within "a".

[law@torsion gcc]$ ./xgcc -B./ -g sra-1.c -O2
[law@torsion gcc]$ gdb ./a.out
GNU gdb (GDB) Fedora 7.10.1-30.fc23
[ ... ]
(gdb) b 43
Breakpoint 1 at 0x40056b: file sra-1.c, line 43.
(gdb) r
Starting program: /opt/notnfs/law/gcc-testing/obj/gcc/a.out
Missing separate debuginfos, use: dnf debuginfo-install 
glibc-2.22-10.fc23.x86_64

Breakpoint 1, f3 (k=<optimized out>) at sra-1.c:43
43        bar (a.j);            /* { dg-final { gdb-test 43 "a.j" "14" } 
} */
(gdb) p a
$1 = <optimized out>
(gdb) p a.i
Segmentation fault (core dumped)


My gdb skills are far too rusty to take this further.  I've filed an 
upstream report (BZ20295 in the gdb tracker).  Probably not much we can 
do until the GDB side gets fixed.

I'm going to attach this to 56069 for future reference.
jeff




^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2016-06-23 15:51 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <56D71977.5040204@t-online.de>
2016-03-02 17:30 ` IRA costs tweaks, PR 56069 Bernd Schmidt
2016-03-02 21:53   ` Vladimir Makarov
2016-03-03 17:45     ` Bernd Schmidt
2016-03-07 20:29       ` Richard Sandiford
2016-04-25  9:11     ` Bernd Schmidt
2016-04-27  4:02       ` Jeff Law
2016-04-27 11:33         ` Bernd Schmidt
2016-06-23 15:51           ` Jeff Law
2016-04-27  3:57     ` Jeff Law

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).