This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFC]: Remove Mem/address type assumption in combiner


On Fri, 1 May 2015, Segher Boessenkool wrote:
> On Wed, Apr 29, 2015 at 12:03:35PM -0500, Segher Boessenkool wrote:
> > On Wed, Apr 29, 2015 at 09:25:21AM +0000, Kumar, Venkataramanan wrote:
> > > diff --git a/gcc/combine.c b/gcc/combine.c
> > > index 5c763b4..945abdb 100644
> > > --- a/gcc/combine.c
> > > +++ b/gcc/combine.c
> > > @@ -7703,8 +7703,6 @@ make_compound_operation (rtx x, enum rtx_code in_code)
> > >       but once inside, go back to our default of SET.  */
> > >
> > >    next_code = (code == MEM ? MEM
> > > -              : ((code == PLUS || code == MINUS)
> > > -                 && SCALAR_INT_MODE_P (mode)) ? MEM
> > >                : ((code == COMPARE || COMPARISON_P (x))
> > >                   && XEXP (x, 1) == const0_rtx) ? COMPARE
> > >                : in_code == COMPARE ? SET : in_code);
> > >
> > >
> > > On X86_64, it passes bootstrap and regression tests.
> > > But on Aarch64 the test in PR passed, but I got a few test case failures.
> >
> > > There are few patterns based on multiplication operations in Aarch64 backend which are used to match with the pattern combiner generated.
> > > Now those patterns have to be fixed to use SHIFTS.  Also need to see any impact on other targets.
> >
> > Right.  It would be good if you could find out for what targets it matters.
> > The thing is, if a target expects only the patterns as combine used to make
> > them, it will regress (as you've seen on aarch64); but it will regress
> > _silently_.  Which isn't so nice.
> >
> > > But  before that  I wanted to check if the assumption in combiner,  can simply be removed ?
> >
> > Seeing for what targets / patterns it makes a difference would tell us the
> > answer to that, too :-)
> >
> > I'll run some tests with and without your patch.
>
> So I ran the tests.  These are text sizes for vmlinux built for most
> architectures (before resp. after the patch).

Thanks for actually checking the impact.

>  Results are good, but
> they show many targets need some work...
>
>
> BIGGER
>  2212322   2212706   cris

Also observable as noted in PR66171, a regression:

Running /tmp/hpautotest-gcc0/gcc/gcc/testsuite/gcc.target/cris/cris.exp
...
FAIL: gcc.target/cris/biap.c scan-assembler addi
FAIL: gcc.target/cris/biap.c scan-assembler-not lsl

I confess the test-case-"guarded" addi pattern should have been
expressed with a shift in addition to the multiplication. ("In
addition to" as the canonically wrong one used to be the
combine-matching pattern; I'm not sure I should really drop that
just yet.)  I'll have to check that expressing using a shift
fixes this issue.

Supposedly more noteworthy: this now-stricter canonicalization
leads to a requirement to rewrite patterns that used to be:

 (parallel
  [(set reg0 (mem (plus (mult reg1 N) reg2)))
   (set reg3 (plus (mult reg1 N) reg2))])

into the awkwardly asymmetric:

 (parallel
  [(set reg0 (mem (plus (mult reg1 2) reg2)))
   (set reg3 (plus (ashift reg1 M) reg2))])

(where M = log2 N)

and of course a need to verify that combine actually *does* make
use of the pattern after the change at least as much as it did
before.

> Some targets need some love (but what else is new).

Meh.  Well, you now got some whine to go with that cheese.

brgds, H-P


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]