| Message ID | 87a9jpj8mq.fsf@talisman.default |
|---|---|
| State | New |
| Headers | show |
On Sat, Sep 7, 2013 at 11:14 AM, Richard Sandiford wrote: > The problem seems to be split across IRA and LRA. In IRA we have: > > FOR_EACH_BB (bb) > FOR_BB_INSNS (bb, insn) > { > if (! INSN_P (insn)) > continue; > for_each_rtx (&insn, set_paradoxical_subreg, (void *)pdx_subregs); > } > > which sets pdx_subregs[R] if R is used in a paradoxical subreg, followed by: > > /* Don't set reg (if pdx_subregs[regno] == true) equivalent to a mem. */ > if (MEM_P (src) && pdx_subregs[regno]) > { > note_stores (set, no_equiv, NULL); > continue; > } > > I think this should only happen when a paradoxical subreg is seen > in a nondebug insn. Agreed. Otherwise these debug insns would have influence on the generated code and that's not supposed to happen. > The patch below fixes the wide-int problem for me. Bootstrapped & > regression-tested on trunk for x86_64-linux-gnu. OK to install? > > Thanks, > Richard > > > gcc/ > * ira.c (update_equiv_regs): Only call set_paradoxical_subreg > for non-debug insns. > * lra.c (new_insn_reg): Take the containing insn as a parameter. > Only modify lra_reg_info[].biggest_mode if it's non-debug insn. > (collect_non_operand_hard_regs, add_regs_to_insn_regno_info): Update > accordingly. I don't think this falls under "RTL optimizers", but the patch looks OK to me. Can you please add a test case? Ciao! Steven
Steven Bosscher <stevenb.gcc@gmail.com> writes:
> Can you please add a test case?
What kind of test would you suggest? Do we have a harness for testing
that -O2 and -O2 -g .text output is identical?
I suppose the main testcase will be bootstrap if wide-int ever does get
accepted.
Thanks,
Richard
On Sat, Sep 7, 2013 at 1:37 PM, Richard Sandiford wrote: > Steven Bosscher <stevenb.gcc@gmail.com> writes: >> Can you please add a test case? > > What kind of test would you suggest? Do we have a harness for testing > that -O2 and -O2 -g .text output is identical? Not .text, but the assembly output or the final RTL dumps. This is what the guality test framework does. Ciao! Steven
On Sat, Sep 07, 2013 at 12:37:14PM +0100, Richard Sandiford wrote: > Steven Bosscher <stevenb.gcc@gmail.com> writes: > > Can you please add a test case? > > What kind of test would you suggest? Do we have a harness for testing > that -O2 and -O2 -g .text output is identical? No, but we have -fcompare-debug, which is even stronger than that (compares final RTL, so can catch even differences that aren't visible in the .s file, but still are relevant). And, the default way of bootstrapping also compares .text etc. after stripping, between stage2 (built without debug) and stage3 (with debug). Jakub
Jakub Jelinek <jakub@redhat.com> writes: > On Sat, Sep 07, 2013 at 12:37:14PM +0100, Richard Sandiford wrote: >> Steven Bosscher <stevenb.gcc@gmail.com> writes: >> > Can you please add a test case? >> >> What kind of test would you suggest? Do we have a harness for testing >> that -O2 and -O2 -g .text output is identical? > > No, but we have -fcompare-debug, which is even stronger than that (compares > final RTL, so can catch even differences that aren't visible in the .s file, > but still are relevant). And, the default way of bootstrapping also > compares .text etc. after stripping, between stage2 (built without debug) > and stage3 (with debug). Right, the bootstrap thing is what forced me to write the patch in the first place. I probably wasn't clear, sorry, but the original problem was a bootstrap comparison failure between stage 2 and stage 3 because the tree.o code depended on whether debug info was enabled. I'll try to use -fcompare-debug though, thanks. Richard
Index: gcc/ira.c =================================================================== --- gcc/ira.c 2013-09-01 12:39:40.739835911 +0100 +++ gcc/ira.c 2013-09-07 09:42:34.993934881 +0100 @@ -2944,11 +2944,8 @@ update_equiv_regs (void) prevent access beyond allocated memory for paradoxical memory subreg. */ FOR_EACH_BB (bb) FOR_BB_INSNS (bb, insn) - { - if (! INSN_P (insn)) - continue; - for_each_rtx (&insn, set_paradoxical_subreg, (void *)pdx_subregs); - } + if (NONDEBUG_INSN_P (insn)) + for_each_rtx (&insn, set_paradoxical_subreg, (void *) pdx_subregs); /* Scan the insns and find which registers have equivalences. Do this in a separate scan of the insns because (due to -fcse-follow-jumps) Index: gcc/lra.c =================================================================== --- gcc/lra.c 2013-07-17 08:36:09.260013240 +0100 +++ gcc/lra.c 2013-09-07 09:42:35.004934955 +0100 @@ -480,13 +480,13 @@ init_insn_regs (void) = create_alloc_pool ("insn regs", sizeof (struct lra_insn_reg), 100); } -/* Create LRA insn related info about referenced REGNO with TYPE - (in/out/inout), biggest reference mode MODE, flag that it is +/* Create LRA insn related info about a reference to REGNO in INSN with + TYPE (in/out/inout), biggest reference mode MODE, flag that it is reference through subreg (SUBREG_P), flag that is early clobbered in the insn (EARLY_CLOBBER), and reference to the next insn reg info (NEXT). */ static struct lra_insn_reg * -new_insn_reg (int regno, enum op_type type, enum machine_mode mode, +new_insn_reg (rtx insn, int regno, enum op_type type, enum machine_mode mode, bool subreg_p, bool early_clobber, struct lra_insn_reg *next) { struct lra_insn_reg *ir; @@ -494,7 +494,8 @@ new_insn_reg (int regno, enum op_type ty ir = (struct lra_insn_reg *) pool_alloc (insn_reg_pool); ir->type = type; ir->biggest_mode = mode; - if (GET_MODE_SIZE (mode) > GET_MODE_SIZE (lra_reg_info[regno].biggest_mode)) + if (GET_MODE_SIZE (mode) > GET_MODE_SIZE (lra_reg_info[regno].biggest_mode) + && NONDEBUG_INSN_P (insn)) lra_reg_info[regno].biggest_mode = mode; ir->subreg_p = subreg_p; ir->early_clobber = early_clobber; @@ -976,7 +977,7 @@ collect_non_operand_hard_regs (rtx *x, l && ! (FIRST_STACK_REG <= regno && regno <= LAST_STACK_REG)); #endif - list = new_insn_reg (regno, type, mode, subreg_p, + list = new_insn_reg (data->insn, regno, type, mode, subreg_p, early_clobber, list); } } @@ -1575,7 +1576,7 @@ add_regs_to_insn_regno_info (lra_insn_re expand_reg_info (); if (bitmap_set_bit (&lra_reg_info[regno].insn_bitmap, uid)) { - data->regs = new_insn_reg (regno, type, mode, subreg_p, + data->regs = new_insn_reg (data->insn, regno, type, mode, subreg_p, early_clobber, data->regs); return; } @@ -1587,8 +1588,9 @@ add_regs_to_insn_regno_info (lra_insn_re if (curr->subreg_p != subreg_p || curr->biggest_mode != mode) /* The info can not be integrated into the found structure. */ - data->regs = new_insn_reg (regno, type, mode, subreg_p, - early_clobber, data->regs); + data->regs = new_insn_reg (data->insn, regno, type, mode, + subreg_p, early_clobber, + data->regs); else { if (curr->type != type)
While doing some work on wide-int, I hit a bootstrap comparison failure that was caused by the RA output depending on debug insns. There was a register R that was used normally by all "real" insns, but which was used in a paradoxical subreg by one of the debug insns. There was also a natural equivalence between R and a memory. The problem was that the paradoxical subreg in the debug insn was stopping that equivalence from being used. The problem seems to be split across IRA and LRA. In IRA we have: FOR_EACH_BB (bb) FOR_BB_INSNS (bb, insn) { if (! INSN_P (insn)) continue; for_each_rtx (&insn, set_paradoxical_subreg, (void *)pdx_subregs); } which sets pdx_subregs[R] if R is used in a paradoxical subreg, followed by: /* Don't set reg (if pdx_subregs[regno] == true) equivalent to a mem. */ if (MEM_P (src) && pdx_subregs[regno]) { note_stores (set, no_equiv, NULL); continue; } I think this should only happen when a paradoxical subreg is seen in a nondebug insn. If we use the equivalence when a debug insn has a paradoxical subreg, the subreg will be replaced with a MEM that might overlap surrounding data, but I think it's a valid transformation when no run-time access will take place. The data in the defined part of the MEM should still be correct. Then in LRA we keep track of the biggest mode that is used to refer to each register (including references via paradoxical subregs). This is done when recording register references in an instruction: static struct lra_insn_reg * new_insn_reg (int regno, enum op_type type, enum machine_mode mode, bool subreg_p, bool early_clobber, struct lra_insn_reg *next) { struct lra_insn_reg *ir; ir = (struct lra_insn_reg *) pool_alloc (insn_reg_pool); ir->type = type; ir->biggest_mode = mode; if (GET_MODE_SIZE (mode) > GET_MODE_SIZE (lra_reg_info[regno].biggest_mode)) lra_reg_info[regno].biggest_mode = mode; This then affects allocation decisions in several places. Here too I think we need to guard for nondebug insns, although we still need to record register references for debug insns. This of course means that when substituting the allocation into the debug insns, we have to cope with paradoxical subregs that are wider than the largest recorded mode. I don't think that's a new requirement though, since we needed the same thing for reload. AFAIK the existing code should already handle it. The patch below fixes the wide-int problem for me. Bootstrapped & regression-tested on trunk for x86_64-linux-gnu. OK to install? Thanks, Richard gcc/ * ira.c (update_equiv_regs): Only call set_paradoxical_subreg for non-debug insns. * lra.c (new_insn_reg): Take the containing insn as a parameter. Only modify lra_reg_info[].biggest_mode if it's non-debug insn. (collect_non_operand_hard_regs, add_regs_to_insn_regno_info): Update accordingly.