Re: [PATCH 3/5] combine: add regno field to LOG_LINKS
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
Re: [PATCH 3/5] combine: add regno field to LOG_LINKS
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
Re: [PATCH 3/5] combine: add regno field to LOG_LINKS
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
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
Re: [PATCH 3/5] combine: add regno field to LOG_LINKS
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
> >
> >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
Re: [PATCH 3/5] combine: add regno field to LOG_LINKS
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
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
