This is the mail archive of the
mailing list for the GCC project.
Re: [RFC]: Remove Mem/address type assumption in combiner
- From: Hans-Peter Nilsson <hp at bitrange dot com>
- To: Segher Boessenkool <segher at kernel dot crashing dot org>
- Cc: "Kumar, Venkataramanan" <Venkataramanan dot Kumar at amd dot com>, "Jeff Law (law at redhat dot com)" <law at redhat dot com>, "gcc-patches at gcc dot gnu dot org" <gcc-patches at gcc dot gnu dot org>, "maxim dot kuvyrkov at linaro dot org" <maxim dot kuvyrkov at linaro dot org>
- Date: Fri, 15 May 2015 22:40:48 -0400 (EDT)
- Subject: Re: [RFC]: Remove Mem/address type assumption in combiner
- Authentication-results: sourceware.org; auth=none
- References: <7794A52CE4D579448B959EED7DD0A4723DCE9F34 at satlexdag06 dot amd dot com> <20150429170335 dot GB21715 at gate dot crashing dot org> <20150501151758 dot GA1182 at gate dot crashing dot org>
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...
> 2212322 2212706 cris
Also observable as noted in PR66171, a regression:
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:
[(set reg0 (mem (plus (mult reg1 N) reg2)))
(set reg3 (plus (mult reg1 N) reg2))])
into the awkwardly asymmetric:
[(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
> Some targets need some love (but what else is new).
Meh. Well, you now got some whine to go with that cheese.