Message ID | 25298b65-d1ff-f00b-c67a-a98aef794fe8@redhat.com |
---|---|
State | New |
Headers | show |
On 02/14/2017 01:30 AM, Jeff Law wrote: > > So imagine we have two allocnos related by a copy chain (two operand > architecture). > > (gdb) p *cp->first > $11 = {num = 9, regno = 33, mode = DImode, wmode = DImode, aclass = > GENERAL_REGS, dont_reassign_p = 0, > bad_spill_p = 0, assigned_p = 1, conflict_vec_p = 0, hard_regno = > -1, next_regno_allocno = 0x0, > loop_tree_node = 0x1e0b190, nrefs = 13, freq = 8069, class_cost = > 1380, updated_class_cost = 1380, > memory_cost = 29656, updated_memory_cost = 29656, > excess_pressure_points_num = 17, allocno_prefs = 0x0, > allocno_copies = 0x1e4b400, cap = 0x0, cap_member = 0x0, num_objects > = 1, objects = {0x1e8b6a0, 0x0}, > call_freq = 0, calls_crossed_num = 0, cheap_calls_crossed_num = 0, > crossed_calls_clobbered_regs = 0, > hard_reg_costs = 0x1da9510, updated_hard_reg_costs = 0x0, > conflict_hard_reg_costs = 0x0, > updated_conflict_hard_reg_costs = 0x0, add_data = 0x1e04378} > > (gdb) p *cp->second > $12 = {num = 12, regno = 39, mode = SImode, wmode = SImode, aclass = > GENERAL_REGS, dont_reassign_p = 0, > bad_spill_p = 1, assigned_p = 1, conflict_vec_p = 0, hard_regno = 2, > next_regno_allocno = 0x0, > loop_tree_node = 0x1e0b190, nrefs = 2, freq = 388, class_cost = 0, > updated_class_cost = 0, memory_cost = 1552, > updated_memory_cost = 1552, excess_pressure_points_num = 0, > allocno_prefs = 0x0, allocno_copies = 0x1e4b400, > cap = 0x0, cap_member = 0x0, num_objects = 2, objects = {0x1e8b7e0, > 0x1e8b830}, call_freq = 0, > calls_crossed_num = 0, cheap_calls_crossed_num = 0, > crossed_calls_clobbered_regs = 0, > hard_reg_costs = 0x1da9550, updated_hard_reg_costs = 0x0, > conflict_hard_reg_costs = 0x0, > updated_conflict_hard_reg_costs = 0x0, add_data = 0x1e04480} > > > Note how cp->first is mode DImode. > > Now assume that all the real uses of cp->first occur as SUBREG > expressions. But there is a DImode clobber of cp->first. Like this: > > > (insn 7 2 3 2 (clobber (reg/v:DI 33 [ u ])) > "/home/gcc/GIT-2/gcc/libgcc/libgcc2.c":404 -1 > (nil)) > (insn 3 7 4 2 (set (subreg:HI (reg/v:DI 33 [ u ]) 0) > (mem/c:HI (reg/f:HI 9 ap) [4 u+0 S2 A16])) > "/home/gcc/GIT-2/gcc/libgcc/libgcc2.c":404 5 {*movhi_h8300} > (nil)) > (insn 4 3 5 2 (set (subreg:HI (reg/v:DI 33 [ u ]) 2) > (mem/c:HI (plus:HI (reg/f:HI 9 ap) > (const_int 2 [0x2])) [4 u+2 S2 A16])) > "/home/gcc/GIT-2/gcc/libgcc/libgcc2.c":404 5 {*movhi_h8300} > (nil)) > [ ... ] > (insn 35 32 37 5 (parallel [ > (set (reg:SI 39 [ _32 ]) > (lshiftrt:SI (subreg:SI (reg/v:DI 33 [ u ]) 0) > (subreg:QI (reg:HI 38) 1))) > (clobber (scratch:QI)) > ]) "/home/gcc/GIT-2/gcc/libgcc/libgcc2.c":415 229 {*shiftsi} > (expr_list:REG_DEAD (reg:HI 38) > (expr_list:REG_DEAD (reg/v:DI 33 [ u ]) > (expr_list:REG_EQUIV (mem/j/c:SI (plus:HI (reg/f:HI 11 fp) > (const_int -4 [0xfffffffffffffffc])) [1 > w.s.low+0 S4 A16]) > (nil))))) > > There's other references to (reg 33), but again, they all use subregs. > The only real DImode reference to (reg 33) is in the clobber. And > remember that (reg 33) is involved in a copy chain. > > > So we'll eventually call allocno_copy_cost_saving and try to compute a > cost savings using: > > 2764 cost += cp->freq * > ira_register_move_cost[allocno_mode][rclass][rclass]; > > But ira_register_move_cost[DImode] is NULL -- it's never been > initialized, presumably because we never see a real DImode reference > to anything except in CLOBBER statements. > > We can fix this in scan_one_insn via the attached patch. I'm not sure > if this is the best place to catch this or not. > > I haven't included a testcase as this trips just building libgcc on > the H8 target. I could easily reduce it if folks think its worth the > trouble. > > I've verified this allows libgcc to build on the H8 target and > bootstrapped/regression tested the change on x86_64-unknown-linux-gnu > as well. > > Vlad, is this OK for the trunk, or should we be catching this elsewhere? There is no harm to put this fix. We could check initialization at every usage but it will be a big impact on IRA's speed. Another place would be at the beginning of find_costs_and_classes in the following loop: for (i = max_reg_num () - 1; i >= FIRST_PSEUDO_REGISTER; i--) regno_best_class[i] = NO_REGS; Still your place will save CPU cycles most probably. So the patch is ok for me. Thank you, Jeff.
Hi Jeff, On Mon, 13 Feb 2017, Jeff Law wrote: > I've verified this allows libgcc to build on the H8 target and > bootstrapped/regression tested the change on x86_64-unknown-linux-gnu as well. I need to reproduce this, but my latest daily bootstrap on i386-unknown-freebsd10.3 failed with... .../gerald/gcc-HEAD/libquadmath/math/remainderq.c: In function 'remainderq': .../gerald/gcc-HEAD/libquadmath/math/remainderq.c:67:1: internal compiler error: in ira_init_register_move_cost, at ira.c:1580 ...and your message was the only one on gcc-patches the last couple of days that contains the string "ira_init_register_move_cost". Not sure yet that it's your patch, but this looks a little bit like a smoking gun... :-) Gerald
On 02/15/2017 12:42 PM, Gerald Pfeifer wrote: > Hi Jeff, > > On Mon, 13 Feb 2017, Jeff Law wrote: >> I've verified this allows libgcc to build on the H8 target and >> bootstrapped/regression tested the change on x86_64-unknown-linux-gnu >> as well. > > I need to reproduce this, but my latest daily bootstrap on > i386-unknown-freebsd10.3 failed with... > > .../gerald/gcc-HEAD/libquadmath/math/remainderq.c: In function > 'remainderq': > .../gerald/gcc-HEAD/libquadmath/math/remainderq.c:67:1: internal > compiler error: in ira_init_register_move_cost, at ira.c:1580 > > ...and your message was the only one on gcc-patches the last couple > of days that contains the string "ira_init_register_move_cost". > > Not sure yet that it's your patch, but this looks a little bit like a > smoking gun... :-) Already testing a fix. jeff
On Wed, 15 Feb 2017, Jeff Law wrote: >> .../gerald/gcc-HEAD/libquadmath/math/remainderq.c:67:1: internal >> compiler error: in ira_init_register_move_cost, at ira.c:1580 > Already testing a fix. Thanks, Jeff. Just to confirm that things are back to bootstrap land. Gerald
diff --git a/gcc/ChangeLog b/gcc/ChangeLog index f352051..2170e57 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,3 +1,9 @@ +2017-02-13 Jeff Law <law@redhat.com> + + PR target/79404 + * ira-costs.c (scan_one_insn): Initialize register move costs + for pseudos seen in USE/CLOBBER insns. + 2017-02-13 Aaron Sawdey <acsawdey@linux.vnet.ibm.com> PR target/79449 diff --git a/gcc/ira-costs.c b/gcc/ira-costs.c index c561db6..1737430 100644 --- a/gcc/ira-costs.c +++ b/gcc/ira-costs.c @@ -1438,9 +1438,25 @@ scan_one_insn (rtx_insn *insn) return insn; pat_code = GET_CODE (PATTERN (insn)); - if (pat_code == USE || pat_code == CLOBBER || pat_code == ASM_INPUT) + if (pat_code == ASM_INPUT) return insn; + /* If INSN is a USE/CLOBBER of a pseudo in a mode M then go ahead + and initialize the register move costs of mode M. + + The pseudo may be related to another pseudo via a copy (implicit or + explicit) and if there are no mode M uses/sets of the original + pseudo, then we may leave the register move costs uninitialized for + mode M. */ + if (pat_code == USE || pat_code == CLOBBER) + { + rtx x = XEXP (PATTERN (insn), 0); + if (GET_CODE (x) == REG + && REGNO (x) >= FIRST_PSEUDO_REGISTER) + ira_init_register_move_cost_if_necessary (GET_MODE (x)); + return insn; + } + counted_mem = false; set = single_set (insn); extract_insn (insn);