Re: Fix a bug in merging uninitialized refs into a single web

[Jie Zhang <jie at codesourcery dot com>]

Hi Alex,

Since you are the author of the code my patch changes, could you please 
take a look at my patch and confirm that it does the right thing. Thanks!

Jie

On 01/06/2011 10:21 AM, Jie Zhang wrote:
> On 01/06/2011 02:09 AM, Jeff Law wrote:
> > -----BEGIN PGP SIGNED MESSAGE-----
> > Hash: SHA1
> >
> > On 01/05/11 04:04, Jie Zhang wrote:
> > > Hi,
> > >
> > > When investigating a performance regression issue of ARM GCC on
> > > Dhrystone, I found this bug. Dhrystone contains code like this (already
> > > reduced):
> > >
> > > foo ()
> > > {
> > > }
> > >
> > > i.e. returns an uninitialized value. Currently GCC will produce the
> > > following assembly for the above function with option "-O2
> > > -funroll-loops":
> > >
> > > foo:
> > > mov r0, #0
> > > bx lr
> > >
> > > while previously GCC generated
> > >
> > > foo:
> > > bx lr
> > >
> > > I tracked this down to the web pass. Before that pass:
> > >
> > > (insn 8 7 6 2 (clobber (reg:SI 134 [<retval> ])) t.c:1 -1
> > > (nil))
> > >
> > > (insn 6 8 9 2 (set (reg/i:SI 0 r0)
> > > (reg:SI 134 [<retval> ])) t.c:1 168 {*arm_movsi_insn}
> > > (expr_list:REG_DEAD (reg:SI 134 [<retval> ])
> > > (nil)))
> > >
> > > After that pass:
> > >
> > > (insn 8 7 6 2 (clobber (reg:SI 134 [<retval> ])) t.c:1 -1
> > > (nil))
> > >
> > > (insn 6 8 9 2 (set (reg/i:SI 0 r0)
> > > (reg:SI 135 [<retval> ])) t.c:1 168 {*arm_movsi_insn}
> > > (expr_list:REG_DEAD (reg:SI 134 [<retval> ])
> > > (nil)))
> > >
> > > The web pass replaces (reg 134) in insn 6 by (reg 135) because the
> > > definition of (reg 134) in insn 8 and the use of (reg 134) in insn 6 are
> > > in two different webs. It surprised me since the web pass should have
> > > already merged uninitialized refs into a single web. When I looked
> > > closer, I found there was a small bug in the merging code:
> > >
> > > /* In case the original register is already assigned, generate new
> > > one. Since we use USED to merge uninitialized refs into a single
> > > web, we might found an element to be nonzero without our having
> > > used it. Test for 1, because union_defs saves it for our use,
> > > and there won't be any use for the other values when we get to
> > > this point. */
> > > if (used[REGNO (reg)] != 1)
> > > newreg = reg, used[REGNO (reg)] = 1;
> > >
> > > The merging code uses used[] to keep the DF_REF_ID of the first
> > > uninitialized uses of a register. But the above code will clobber it and
> > > set it to 1 when it sees the definition in insn 8. Thus when come the
> > > reference in insn 6, (used[REGNO (reg)] != 1) will not be true and
> > > another code path will be chosen to produce a new register.
> > >
> > > The patch is attached. Bootstrapped and regression tested on
> > > x86_64-linux-gnu. Since it fixes the patch for PR42631, I still use that
> > > PR for record. Is it OK?
> > ISTM the web pass should probably be ignoring this clobber. The only
> > purpose for the clobber is keep the dataflow code from making the return
> > register live across the entire function.
> Thanks for review. Yeah. I thought about ignoring the clobber at first.
> But later I found there was a bug in the code which merges uninitialized
> refs into a single web and fixing that bug should also fix the issue I
> encountered. So I just try to fix that bug which will be safer and
> easier for me.
>
> Actually, currently GCC only merges the first two uninitialized refs but
> still assigns new registers for the remaining refs. Take the testcase
> from PR42631 as an example:
>
> ================== testcase.c ==================
> void foo()
> {
> unsigned k; /* doesn't crash with 'int' or when initialized */
> while (--k > 0);
> }
> ================================================
>
> Before r155765, which added merging uninitialized refs into a single web:
>
> $ ./cc1 -O1 -funroll-loops testcase.c -fdump-rtl-web -quiet ; grep "Web
> oldreg" testcase.c.171r.web
> Web oldreg=62 newreg=66
> Web oldreg=62 newreg=67
> Web oldreg=62 newreg=68
> Web oldreg=62 newreg=69
> Web oldreg=62 newreg=70
> Web oldreg=62 newreg=71
> Web oldreg=62 newreg=72
> Web oldreg=62 newreg=73
> Web oldreg=62 newreg=74
> $
>
> Current GCC creates one less new regs because it combines the first two
> uses of (reg 62)
>
> $ ./cc1 -O1 -funroll-loops testcase.c -fdump-rtl-web -quiet ; grep "Web
> oldreg" testcase.c.171r.web
> Web oldreg=62 newreg=66
> Web oldreg=62 newreg=67
> Web oldreg=62 newreg=68
> Web oldreg=62 newreg=69
> Web oldreg=62 newreg=70
> Web oldreg=62 newreg=71
> Web oldreg=62 newreg=72
> Web oldreg=62 newreg=73
> $
>
> With my patch, all refs to (reg 62) will be combined:
>
> $ ./cc1 -O1 -funroll-loops testcase.c -fdump-rtl-web -quiet ; grep "Web
> oldreg" testcase.c.171r.web
> $