| Message ID | 66755c54deabecd6a35f4b141a186910b98d965a.1415984897.git.segher@kernel.crashing.org |
|---|---|
| State | New |
| Headers | show |
On 11/14/14 12:19, Segher Boessenkool wrote: > With this new field in place, we can have LOG_LINKS for insns that set > more than one register and distribute them properly in distribute_links. > This then allows many more PARALLELs to be combined. > > Also split off new functions can_combine_{def,use}_p from the > create_log_links function. > > > 2014-11-14 Segher Boessenkool <segher@kernel.crashing.org> > > gcc/ > * combine.c (struct insn_link): New field `regno'. > (alloc_insn_link): New parameter `regno'. Use it. > (find_single_use): Check the new field. > (can_combine_def_p, can_combine_use_p): New functions. Split > off from ... > (create_log_links): ... here. Correct data type of `regno'. > Adjust call to alloc_insn_link. > (adjust_for_new_dest): Find regno, use it in call to > alloc_insn_link. > (try_combine): Adjust call to alloc_insn_link. > (distribute_links): Check the new field. Didn't you lose the check that avoids duplicated LOG_LINKs? Or is the claim that the check is no longer needed because there are no duplicates now that we include the register associated with the link? > + > + rtx set = single_set (insn); > + gcc_assert (set); > + > + rtx reg = SET_DEST (set); > + > + while (GET_CODE (reg) == ZERO_EXTRACT > + || GET_CODE (reg) == STRICT_LOW_PART > + || GET_CODE (reg) == SUBREG) > + reg = XEXP (reg, 0); > + gcc_assert (REG_P (reg)); Can REG ever be a hard reg here? If so, then the SUBREG case needs to simplify the hard reg rather than just strip off the SUBREG. Might be OK, depends on answers to questions above -- holding final approval pending those answers. Jeff
On Tue, Nov 25, 2014 at 11:46:52AM -0700, Jeff Law wrote: > On 11/14/14 12:19, Segher Boessenkool wrote: > >With this new field in place, we can have LOG_LINKS for insns that set > >more than one register and distribute them properly in distribute_links. > >This then allows many more PARALLELs to be combined. > > > >Also split off new functions can_combine_{def,use}_p from the > >create_log_links function. > > > > > >2014-11-14 Segher Boessenkool <segher@kernel.crashing.org> > > > >gcc/ > > * combine.c (struct insn_link): New field `regno'. > > (alloc_insn_link): New parameter `regno'. Use it. > > (find_single_use): Check the new field. > > (can_combine_def_p, can_combine_use_p): New functions. Split > > off from ... > > (create_log_links): ... here. Correct data type of `regno'. > > Adjust call to alloc_insn_link. > > (adjust_for_new_dest): Find regno, use it in call to > > alloc_insn_link. > > (try_combine): Adjust call to alloc_insn_link. > > (distribute_links): Check the new field. > Didn't you lose the check that avoids duplicated LOG_LINKs? I don't think so; if I did, that's a bug. > Or is the > claim that the check is no longer needed because there are no duplicates > now that we include the register associated with the link? Are you talking about create_log_links? There can be no duplicates there (anymore), that would be multiple defs of the same reg in the same insn, indeed. I did check all the places that look at links, and adjusted everything that needed adjusting. Could have missed something of course... Segher
On 11/25/14 14:47, Segher Boessenkool wrote: > On Tue, Nov 25, 2014 at 11:46:52AM -0700, Jeff Law wrote: >> On 11/14/14 12:19, Segher Boessenkool wrote: >>> With this new field in place, we can have LOG_LINKS for insns that set >>> more than one register and distribute them properly in distribute_links. >>> This then allows many more PARALLELs to be combined. >>> >>> Also split off new functions can_combine_{def,use}_p from the >>> create_log_links function. >>> >>> >>> 2014-11-14 Segher Boessenkool <segher@kernel.crashing.org> >>> >>> gcc/ >>> * combine.c (struct insn_link): New field `regno'. >>> (alloc_insn_link): New parameter `regno'. Use it. >>> (find_single_use): Check the new field. >>> (can_combine_def_p, can_combine_use_p): New functions. Split >>> off from ... >>> (create_log_links): ... here. Correct data type of `regno'. >>> Adjust call to alloc_insn_link. >>> (adjust_for_new_dest): Find regno, use it in call to >>> alloc_insn_link. >>> (try_combine): Adjust call to alloc_insn_link. >>> (distribute_links): Check the new field. > >> Didn't you lose the check that avoids duplicated LOG_LINKs? > > I don't think so; if I did, that's a bug. > >> Or is the >> claim that the check is no longer needed because there are no duplicates >> now that we include the register associated with the link? > > Are you talking about create_log_links? There can be no duplicates there > (anymore), that would be multiple defs of the same reg in the same insn, > indeed. Yes, I was referring to the code in create_log_links. You dropped the check for duplicate links. It caught my eye when reading the changes, but then I realized the check may no longer be necessary. Hmm, what about an insn that has two destinations, which happen to be upper and lower SUBREGs of a pseudo. Would that create duplicate links after your change? Jeff
On Wed, Nov 26, 2014 at 11:14:36AM -0700, Jeff Law wrote: > >Are you talking about create_log_links? There can be no duplicates there > >(anymore), that would be multiple defs of the same reg in the same insn, > >indeed. > Yes, I was referring to the code in create_log_links. You dropped the > check for duplicate links. It caught my eye when reading the changes, > but then I realized the check may no longer be necessary. > > Hmm, what about an insn that has two destinations, which happen to be > upper and lower SUBREGs of a pseudo. Would that create duplicate links > after your change? Yes it would. And I'm not so certain distribute_log_links handles that situation gracefully. Rats. IMNSHO such RTL should be invalid (it can always be written simpler as one SET); but there seems to be no such rule. I'll add the check back. Wouldn't it be lovely if we could just use DF here... Segher
On 11/26/14 14:59, Segher Boessenkool wrote: > On Wed, Nov 26, 2014 at 11:14:36AM -0700, Jeff Law wrote: >>> Are you talking about create_log_links? There can be no duplicates there >>> (anymore), that would be multiple defs of the same reg in the same insn, >>> indeed. >> Yes, I was referring to the code in create_log_links. You dropped the >> check for duplicate links. It caught my eye when reading the changes, >> but then I realized the check may no longer be necessary. >> >> Hmm, what about an insn that has two destinations, which happen to be >> upper and lower SUBREGs of a pseudo. Would that create duplicate links >> after your change? > > Yes it would. And I'm not so certain distribute_log_links handles that > situation gracefully. Rats. > > IMNSHO such RTL should be invalid (it can always be written simpler as > one SET); but there seems to be no such rule. There's no such rule. And I was just using an example off the top of my head. There may be others that can't necessarily be expressed by a single set. Jeff
diff --git a/gcc/combine.c b/gcc/combine.c index c4d23e3..cf184db 100644 --- a/gcc/combine.c +++ b/gcc/combine.c @@ -328,6 +328,7 @@ static int *uid_insn_cost; struct insn_link { rtx_insn *insn; + unsigned int regno; struct insn_link *next; }; @@ -346,12 +347,13 @@ static struct obstack insn_link_obstack; /* Allocate a link. */ static inline struct insn_link * -alloc_insn_link (rtx_insn *insn, struct insn_link *next) +alloc_insn_link (rtx_insn *insn, unsigned int regno, struct insn_link *next) { struct insn_link *l = (struct insn_link *) obstack_alloc (&insn_link_obstack, sizeof (struct insn_link)); l->insn = insn; + l->regno = regno; l->next = next; return l; } @@ -686,7 +688,7 @@ find_single_use (rtx dest, rtx_insn *insn, rtx_insn **ploc) if (INSN_P (next) && dead_or_set_p (next, dest)) { FOR_EACH_LOG_LINK (link, next) - if (link->insn == insn) + if (link->insn == insn && link->regno == REGNO (dest)) break; if (link) @@ -982,6 +984,43 @@ delete_noop_moves (void) } +/* Return false if we do not want to (or cannot) combine DEF. */ +static bool +can_combine_def_p (df_ref def) +{ + /* Do not consider if it is pre/post modification in MEM. */ + if (DF_REF_FLAGS (def) & DF_REF_PRE_POST_MODIFY) + return false; + + unsigned int regno = DF_REF_REGNO (def); + + /* Do not combine frame pointer adjustments. */ + if ((regno == FRAME_POINTER_REGNUM + && (!reload_completed || frame_pointer_needed)) +#if !HARD_FRAME_POINTER_IS_FRAME_POINTER + || (regno == HARD_FRAME_POINTER_REGNUM + && (!reload_completed || frame_pointer_needed)) +#endif +#if FRAME_POINTER_REGNUM != ARG_POINTER_REGNUM + || (regno == ARG_POINTER_REGNUM && fixed_regs[regno]) +#endif + ) + return false; + + return true; +} + +/* Return false if we do not want to (or cannot) combine USE. */ +static bool +can_combine_use_p (df_ref use) +{ + /* Do not consider the usage of the stack pointer by function call. */ + if (DF_REF_FLAGS (use) & DF_REF_CALL_STACK_USAGE) + return false; + + return true; +} + /* Fill in log links field for all insns. */ static void @@ -1015,67 +1054,39 @@ create_log_links (void) FOR_EACH_INSN_DEF (def, insn) { - int regno = DF_REF_REGNO (def); + unsigned int regno = DF_REF_REGNO (def); rtx_insn *use_insn; if (!next_use[regno]) continue; - /* Do not consider if it is pre/post modification in MEM. */ - if (DF_REF_FLAGS (def) & DF_REF_PRE_POST_MODIFY) - continue; + if (!can_combine_def_p (def)) + continue; - /* Do not make the log link for frame pointer. */ - if ((regno == FRAME_POINTER_REGNUM - && (! reload_completed || frame_pointer_needed)) -#if !HARD_FRAME_POINTER_IS_FRAME_POINTER - || (regno == HARD_FRAME_POINTER_REGNUM - && (! reload_completed || frame_pointer_needed)) -#endif -#if FRAME_POINTER_REGNUM != ARG_POINTER_REGNUM - || (regno == ARG_POINTER_REGNUM && fixed_regs[regno]) -#endif - ) - continue; + use_insn = next_use[regno]; + next_use[regno] = NULL; - use_insn = next_use[regno]; - if (BLOCK_FOR_INSN (use_insn) == bb) - { - /* flow.c claimed: - - We don't build a LOG_LINK for hard registers contained - in ASM_OPERANDs. If these registers get replaced, - we might wind up changing the semantics of the insn, - even if reload can make what appear to be valid - assignments later. */ - if (regno >= FIRST_PSEUDO_REGISTER - || asm_noperands (PATTERN (use_insn)) < 0) - { - /* Don't add duplicate links between instructions. */ - struct insn_link *links; - FOR_EACH_LOG_LINK (links, use_insn) - if (insn == links->insn) - break; + if (BLOCK_FOR_INSN (use_insn) != bb) + continue; - if (!links) - LOG_LINKS (use_insn) - = alloc_insn_link (insn, LOG_LINKS (use_insn)); - } - } - next_use[regno] = NULL; - } + /* flow.c claimed: - FOR_EACH_INSN_USE (use, insn) - { - int regno = DF_REF_REGNO (use); + We don't build a LOG_LINK for hard registers contained + in ASM_OPERANDs. If these registers get replaced, + we might wind up changing the semantics of the insn, + even if reload can make what appear to be valid + assignments later. */ + if (regno < FIRST_PSEUDO_REGISTER + && asm_noperands (PATTERN (use_insn)) >= 0) + continue; - /* Do not consider the usage of the stack pointer - by function call. */ - if (DF_REF_FLAGS (use) & DF_REF_CALL_STACK_USAGE) - continue; - - next_use[regno] = insn; + LOG_LINKS (use_insn) + = alloc_insn_link (insn, regno, LOG_LINKS (use_insn)); } + + FOR_EACH_INSN_USE (use, insn) + if (can_combine_use_p (use)) + next_use[DF_REF_REGNO (use)] = insn; } } @@ -2347,7 +2358,19 @@ adjust_for_new_dest (rtx_insn *insn) /* The new insn will have a destination that was previously the destination of an insn just above it. Call distribute_links to make a LOG_LINK from the next use of that destination. */ - distribute_links (alloc_insn_link (insn, NULL)); + + rtx set = single_set (insn); + gcc_assert (set); + + rtx reg = SET_DEST (set); + + while (GET_CODE (reg) == ZERO_EXTRACT + || GET_CODE (reg) == STRICT_LOW_PART + || GET_CODE (reg) == SUBREG) + reg = XEXP (reg, 0); + gcc_assert (REG_P (reg)); + + distribute_links (alloc_insn_link (insn, REGNO (reg), NULL)); df_insn_rescan (insn); } @@ -2777,7 +2800,9 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0, SUBST (PATTERN (i2), XVECEXP (PATTERN (i2), 0, 0)); SUBST (XEXP (SET_SRC (PATTERN (i2)), 0), SET_DEST (PATTERN (i1))); - SUBST_LINK (LOG_LINKS (i2), alloc_insn_link (i1, LOG_LINKS (i2))); + unsigned int regno = REGNO (SET_DEST (PATTERN (i1))); + SUBST_LINK (LOG_LINKS (i2), + alloc_insn_link (i1, regno, LOG_LINKS (i2))); } } @@ -13865,7 +13890,7 @@ distribute_links (struct insn_link *links) struct insn_link *link2; FOR_EACH_LOG_LINK (link2, place) - if (link2->insn == link->insn) + if (link2->insn == link->insn && link2->regno == link->regno) break; if (link2 == NULL)
With this new field in place, we can have LOG_LINKS for insns that set more than one register and distribute them properly in distribute_links. This then allows many more PARALLELs to be combined. Also split off new functions can_combine_{def,use}_p from the create_log_links function. 2014-11-14 Segher Boessenkool <segher@kernel.crashing.org> gcc/ * combine.c (struct insn_link): New field `regno'. (alloc_insn_link): New parameter `regno'. Use it. (find_single_use): Check the new field. (can_combine_def_p, can_combine_use_p): New functions. Split off from ... (create_log_links): ... here. Correct data type of `regno'. Adjust call to alloc_insn_link. (adjust_for_new_dest): Find regno, use it in call to alloc_insn_link. (try_combine): Adjust call to alloc_insn_link. (distribute_links): Check the new field. --- gcc/combine.c | 135 ++++++++++++++++++++++++++++++++++------------------------ 1 file changed, 80 insertions(+), 55 deletions(-)