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
