diff mbox series

regalloc/debug: fix buggy print_hard_reg_set

Message ID 202002111454.01BEsT98003726@ignucius.se.axis.com
State New
Headers show
Series regalloc/debug: fix buggy print_hard_reg_set | expand

Commit Message

Hans-Peter Nilsson Feb. 11, 2020, 2:54 p.m. UTC 
I was using ira-conflicts.c:print_hard_reg_set with a local
patch to gdbinit.in in a debug-session, and noticed the
erroneous output.  I see there's an almost identical function in
ira-color.c and on top of that, there's another function by the
same name and with similar semantics in sel-sched-dump.c, but
the last one doesn't try to print ranges.

I'm guessing the dump functions have been used with targets that
have "impossible" registers at FIRST_PSEUDO_REGISTER - 1.  CRIS
happens to have the condition-code register at
FIRST_PSEUDO_REGISTER - 1; not allocatable but appears a lot in
(clobbered) register sets after decc0ration.

Before, for a target with FIRST_PSEUDO_REGISTER 20, you'd get
"19-18" for (1<<19).  For (1<<18)|(1<<19), you'd get "18".

The diff is unfortunately hairy, but I hope you agree the code
is a bit clearer.  I'm deliberately not consolidating the dump
functions as IMHO that's too much a matter of taste.

	* ira-conflicts.c (print_hard_reg_set): Correct output for sets
	including FIRST_PSEUDO_REGISTER - 1.
	* ira-color.c (print_hard_reg_set): Ditto.

Ok to commit?

---
 gcc/ira-color.c     | 22 ++++++++++++----------
 gcc/ira-conflicts.c | 22 ++++++++++++----------
 2 files changed, 24 insertions(+), 20 deletions(-)

Comments

Vladimir Makarov Feb. 11, 2020, 4:54 p.m. UTC | #1 
On 2/11/20 9:54 AM, Hans-Peter Nilsson wrote:
> I was using ira-conflicts.c:print_hard_reg_set with a local
> patch to gdbinit.in in a debug-session, and noticed the
> erroneous output.  I see there's an almost identical function in
> ira-color.c and on top of that, there's another function by the
> same name and with similar semantics in sel-sched-dump.c, but
> the last one doesn't try to print ranges.
>
> I'm guessing the dump functions have been used with targets that
> have "impossible" registers at FIRST_PSEUDO_REGISTER - 1.  CRIS
> happens to have the condition-code register at
> FIRST_PSEUDO_REGISTER - 1; not allocatable but appears a lot in
> (clobbered) register sets after decc0ration.
>
> Before, for a target with FIRST_PSEUDO_REGISTER 20, you'd get
> "19-18" for (1<<19).  For (1<<18)|(1<<19), you'd get "18".
>
> The diff is unfortunately hairy, but I hope you agree the code
> is a bit clearer.  I'm deliberately not consolidating the dump
> functions as IMHO that's too much a matter of taste.
Well, I guess I missed practically identical functions because I was in 
hurry to include the code (coloring for irregular register file 
architectures) before 2010 stage3 deadline.
> 	* ira-conflicts.c (print_hard_reg_set): Correct output for sets
> 	including FIRST_PSEUDO_REGISTER - 1.
> 	* ira-color.c (print_hard_reg_set): Ditto.
>
> Ok to commit?

Yes, thank you for fixing this.  The code is definitely more clear.
Jeff Law Feb. 12, 2020, 6:09 p.m. UTC | #2 
On Tue, 2020-02-11 at 15:54 +0100, Hans-Peter Nilsson wrote:
> I was using ira-conflicts.c:print_hard_reg_set with a local
> patch to gdbinit.in in a debug-session, and noticed the
> erroneous output.  I see there's an almost identical function in
> ira-color.c and on top of that, there's another function by the
> same name and with similar semantics in sel-sched-dump.c, but
> the last one doesn't try to print ranges.
> 
> I'm guessing the dump functions have been used with targets that
> have "impossible" registers at FIRST_PSEUDO_REGISTER - 1.  CRIS
> happens to have the condition-code register at
> FIRST_PSEUDO_REGISTER - 1; not allocatable but appears a lot in
> (clobbered) register sets after decc0ration.
> 
> Before, for a target with FIRST_PSEUDO_REGISTER 20, you'd get
> "19-18" for (1<<19).  For (1<<18)|(1<<19), you'd get "18".
> 
> The diff is unfortunately hairy, but I hope you agree the code
> is a bit clearer.  I'm deliberately not consolidating the dump
> functions as IMHO that's too much a matter of taste.
> 
> 	* ira-conflicts.c (print_hard_reg_set): Correct output for sets
> 	including FIRST_PSEUDO_REGISTER - 1.
> 	* ira-color.c (print_hard_reg_set): Ditto.
> 
> Ok to commit?
Not strictly a regression fix, but it's just dumping routines.  So OK
for the trunk.
jeff
>
diff mbox series

Patch

diff --git a/gcc/ira-color.c b/gcc/ira-color.c
index 4a726dc7c..0d88c1efe 100644
--- a/gcc/ira-color.c
+++ b/gcc/ira-color.c
@@ -478,24 +478,26 @@  first_common_ancestor_node (allocno_hard_regs_node_t first,
 static void
 print_hard_reg_set (FILE *f, HARD_REG_SET set, bool new_line_p)
 {
-  int i, start;
+  int i, start, end;
 
-  for (start = -1, i = 0; i < FIRST_PSEUDO_REGISTER; i++)
+  for (start = end = -1, i = 0; i < FIRST_PSEUDO_REGISTER; i++)
     {
-      if (TEST_HARD_REG_BIT (set, i))
+      bool reg_included = TEST_HARD_REG_BIT (set, i);
+
+      if (reg_included)
 	{
-	  if (i == 0 || ! TEST_HARD_REG_BIT (set, i - 1))
+	  if (start == -1)
 	    start = i;
+	  end = i;
 	}
-      if (start >= 0
-	  && (i == FIRST_PSEUDO_REGISTER - 1 || ! TEST_HARD_REG_BIT (set, i)))
+      if (start >= 0 && (!reg_included || i == FIRST_PSEUDO_REGISTER - 1))
 	{
-	  if (start == i - 1)
+	  if (start == end)
 	    fprintf (f, " %d", start);
-	  else if (start == i - 2)
-	    fprintf (f, " %d %d", start, start + 1);
+	  else if (start == end + 1)
+	    fprintf (f, " %d %d", start, end);
 	  else
-	    fprintf (f, " %d-%d", start, i - 1);
+	    fprintf (f, " %d-%d", start, end);
 	  start = -1;
 	}
     }
diff --git a/gcc/ira-conflicts.c b/gcc/ira-conflicts.c
index 11d3a86dc..0220e725e 100644
--- a/gcc/ira-conflicts.c
+++ b/gcc/ira-conflicts.c
@@ -611,25 +611,27 @@  build_conflicts (void)
 static void
 print_hard_reg_set (FILE *file, const char *title, HARD_REG_SET set)
 {
-  int i, start;
+  int i, start, end;
 
   fputs (title, file);
-  for (start = -1, i = 0; i < FIRST_PSEUDO_REGISTER; i++)
+  for (start = end = -1, i = 0; i < FIRST_PSEUDO_REGISTER; i++)
     {
-      if (TEST_HARD_REG_BIT (set, i))
+      bool reg_included = TEST_HARD_REG_BIT (set, i);
+
+      if (reg_included)
 	{
-	  if (i == 0 || ! TEST_HARD_REG_BIT (set, i - 1))
+	  if (start == -1)
 	    start = i;
+	  end = i;
 	}
-      if (start >= 0
-	  && (i == FIRST_PSEUDO_REGISTER - 1 || ! TEST_HARD_REG_BIT (set, i)))
+      if (start >= 0 && (!reg_included || i == FIRST_PSEUDO_REGISTER - 1))
 	{
-	  if (start == i - 1)
+	  if (start == end)
 	    fprintf (file, " %d", start);
-	  else if (start == i - 2)
-	    fprintf (file, " %d %d", start, start + 1);
+	  else if (start == end + 1)
+	    fprintf (file, " %d %d", start, end);
 	  else
-	    fprintf (file, " %d-%d", start, i - 1);
+	    fprintf (file, " %d-%d", start, end);
 	  start = -1;
 	}
     }