Re: PR80357: Negative register pressure

2017-04-18 Thread Jeff Law

On 04/14/2017 04:37 AM, Richard Sandiford wrote:

In the PR testcase, there were two instructions that had
a large number of insn_reg_use records for the same register.
model_recompute was instead expecting the records to be unique
and so decremented the register pressure for each one.  We then
ended up with a negative pressure.

I think the records *should* be unique here, and that this is
really a bug in the generic -fsched-pressure code.  Making them
unique could be too invasive for GCC 7 though.  There are at
least two problems:

(1) sched-deps.c uses rtx_insn_lists instead of bitmaps to record
 the set of instructions that use a live register.
 sched-rgn.c then propagates this information between blocks
 in a region using list concatenation:

   succ_rl->uses = concat_INSN_LIST (pred_rl->uses, succ_rl->uses);

 So dependencies for common predecessors will appear multiple
 times in the list.

 In this case (and for the PR), it might be enough to make
 setup_insn_reg_uses detect duplicate entries.  However...

(2) setup_insn_reg_uses adds entries for all queued uses of a register
 at the point that the register is expected to die.  It looks like
 this doesn't work reliably for REG_INC registers: if a register R
 is auto-incremented in I1 and then used for the last time in I2,
 setup_insn_reg_uses will first treat the original R as dying
 in I1 (rightly IMO).  But that use of R in I1 is still in the
 reg_last->uses list when processing I2, so setup_insn_reg_uses
 adds it a second time.

There might be more reasons for multiple records: I stopped looking
at this point.

I think for GCC 7 it'd be more pragmatic to live with the duplicate
entries and keep the fix specific to SCHED_PRESSURE_MODEL.

Tested on aarch64-linux-gnu (which uses SCHED_PRESSURE_MODEL by default)
and on x86_64-linux-gnu.  OK to install?

Thanks,
Richard


gcc/
PR rtl-optimization/80357
* haifa-sched.c (tmp_bitmap): New variable.
(model_recompute): Handle duplicate use records.
(alloc_global_sched_pressure_data): Initialize tmp_bitmap.
(free_global_sched_pressure_data): Free it.

gcc/testsuite/
PR rtl-optimization/80357
* gcc.c-torture/compile/pr80357.c: New test.

OK.

I'm going to go ahead and commit.

Jeff


PR80357: Negative register pressure

2017-04-14 Thread Richard Sandiford
In the PR testcase, there were two instructions that had
a large number of insn_reg_use records for the same register.
model_recompute was instead expecting the records to be unique
and so decremented the register pressure for each one.  We then
ended up with a negative pressure.

I think the records *should* be unique here, and that this is
really a bug in the generic -fsched-pressure code.  Making them
unique could be too invasive for GCC 7 though.  There are at
least two problems:

(1) sched-deps.c uses rtx_insn_lists instead of bitmaps to record
the set of instructions that use a live register.
sched-rgn.c then propagates this information between blocks
in a region using list concatenation:

  succ_rl->uses = concat_INSN_LIST (pred_rl->uses, succ_rl->uses);

So dependencies for common predecessors will appear multiple
times in the list.

In this case (and for the PR), it might be enough to make
setup_insn_reg_uses detect duplicate entries.  However...

(2) setup_insn_reg_uses adds entries for all queued uses of a register
at the point that the register is expected to die.  It looks like
this doesn't work reliably for REG_INC registers: if a register R
is auto-incremented in I1 and then used for the last time in I2,
setup_insn_reg_uses will first treat the original R as dying
in I1 (rightly IMO).  But that use of R in I1 is still in the
reg_last->uses list when processing I2, so setup_insn_reg_uses
adds it a second time.

There might be more reasons for multiple records: I stopped looking
at this point.

I think for GCC 7 it'd be more pragmatic to live with the duplicate
entries and keep the fix specific to SCHED_PRESSURE_MODEL.

Tested on aarch64-linux-gnu (which uses SCHED_PRESSURE_MODEL by default)
and on x86_64-linux-gnu.  OK to install?

Thanks,
Richard


gcc/
PR rtl-optimization/80357
* haifa-sched.c (tmp_bitmap): New variable.
(model_recompute): Handle duplicate use records.
(alloc_global_sched_pressure_data): Initialize tmp_bitmap.
(free_global_sched_pressure_data): Free it.

gcc/testsuite/
PR rtl-optimization/80357
* gcc.c-torture/compile/pr80357.c: New test.

diff --git a/gcc/haifa-sched.c b/gcc/haifa-sched.c
index 07de1bb..0ebf110 100644
--- a/gcc/haifa-sched.c
+++ b/gcc/haifa-sched.c
@@ -930,6 +930,9 @@ static bitmap saved_reg_live;
 /* Registers mentioned in the current region.  */
 static bitmap region_ref_regs;
 
+/* Temporary bitmap used for SCHED_PRESSURE_MODEL.  */
+static bitmap tmp_bitmap;
+
 /* Effective number of available registers of a given class (see comment
in sched_pressure_start_bb).  */
 static int sched_class_regs_num[N_REG_CLASSES];
@@ -2135,10 +2138,11 @@ model_recompute (rtx_insn *insn)
  registers that will be born in the range [model_curr_point, POINT).  */
   num_uses = 0;
   num_pending_births = 0;
+  bitmap_clear (tmp_bitmap);
   for (use = INSN_REG_USE_LIST (insn); use != NULL; use = use->next_insn_use)
 {
   new_last = model_last_use_except (use);
-  if (new_last < point)
+  if (new_last < point && bitmap_set_bit (tmp_bitmap, use->regno))
{
  gcc_assert (num_uses < ARRAY_SIZE (uses));
  uses[num_uses].last_use = new_last;
@@ -7241,6 +7245,8 @@ alloc_global_sched_pressure_data (void)
  saved_reg_live = BITMAP_ALLOC (NULL);
  region_ref_regs = BITMAP_ALLOC (NULL);
}
+  if (sched_pressure == SCHED_PRESSURE_MODEL)
+   tmp_bitmap = BITMAP_ALLOC (NULL);
 
   /* Calculate number of CALL_SAVED_REGS and FIXED_REGS in register classes
 that we calculate register pressure for.  */
@@ -7274,6 +7280,8 @@ free_global_sched_pressure_data (void)
  BITMAP_FREE (region_ref_regs);
  BITMAP_FREE (saved_reg_live);
}
+  if (sched_pressure == SCHED_PRESSURE_MODEL)
+   BITMAP_FREE (tmp_bitmap);
   BITMAP_FREE (curr_reg_live);
   free (sched_regno_pressure_class);
 }
diff --git a/gcc/testsuite/gcc.c-torture/compile/pr80357.c 
b/gcc/testsuite/gcc.c-torture/compile/pr80357.c
new file mode 100644
index 000..a3c6ab0
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/compile/pr80357.c
@@ -0,0 +1,18 @@
+typedef char a;
+a b, c;
+int d, e;
+void f(void *g) { *(volatile int *)g; }
+void j() {
+  a h, i;
+  for (; b; b += 2) {
+d = b;
+i = i >> b;
+if (i)
+  continue;
+f( + (b >> 2));
+h = 0;
+for (; h < 8 / 2; h++)
+  if (i << h)
+e = 0;
+  }
+}