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 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).  Results are good, but
they show many targets need some work...


BIGGER
 5410496   5432744   alpha
 3848987   3849143   arm
 2190276   2190292   blackfin
 2188431   2191503   c6x (but data shrank by more than text grew)
 2212322   2212706   cris
10896454  10896965   i386
 7471891   7488771   parisc
 6168799   6195391   parisc64
 1545840   1545872   shnommu
 1946649   1954149   xtensa

I looked at some of those.  Alpha regresses with s4add things, arm with
add ,lsl things, parisc with shladd things.  I tried to fix the parisc
one (it seemed simplest), and the 32-bit kernel got most (but not all)
of the size difference back; and the 64-bit wouldn't build (an assert
in the kernel failed, and it uses a floating point reg where an integer
one is needed -- I gave up).


SMALLER
 8688171   8688003   powerpc
20252605  20251797   powerpc64
11425334  11422558   s390
12300135  12299767   x86_64

The PowerPC differences are mostly what first was rlwinm;addi;rlwinm;lwz
and now is rlwinm;lwz; i.e. the add is moved after the two rotates, the
rotates are merged, and the add is made part of the offset in the load.


NO DIFF
 3321492   3321492   frv
 3252747   3252747   m32r
 4708232   4708232   microblaze
 3949145   3949145   mips
 5693019   5693019   mips64
 2373441   2373441   mn10300
 6489846   6489846   sh64
 3733749   3733749   sparc
 6075122   6075122   sparc64

Also quite a few without any diff.  Good :-)  Some of those might have
unnecessary add/mult patterns now, but they also have the add/shift.


I didn't see any big differences, and all are of the expected kind.
Some targets need some love (but what else is new).

The patch is approved for trunk.

Thanks,


Segher


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