| Message ID | 87shlb5l73.fsf@e105548-lin.cambridge.arm.com |
|---|---|
| State | New |
| Headers | show |
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
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 0000000..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(&c + (b >> 2)); + h = 0; + for (; h < 8 / 2; h++) + if (i << h) + e = 0; + } +}
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.