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