Fix a bug in merging uninitialized refs into a single web

Fix a bug in merging uninitialized refs into a single web

[Jie Zhang]

[-- Attachment #1: Type: text/plain, Size: 2253 bytes --]

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?


Regards,
-- 
Jie Zhang


[-- Attachment #2: gcc-pr42631-fix-fix.diff --]
[-- Type: text/x-diff, Size: 1113 bytes --]


	PR debug/42631
	* web.c (entry_register): Don't clobber the number of the
	first uninitialized reference in used[].

	testsuite/
	PR debug/42631
	* gcc.dg/pr42631-2.c: New test.

Index: testsuite/gcc.dg/pr42631-2.c
===================================================================
--- testsuite/gcc.dg/pr42631-2.c	(revision 0)
+++ testsuite/gcc.dg/pr42631-2.c	(revision 0)
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -funroll-loops -fdump-rtl-web" } */
+
+foo()
+{
+}
+
+/* { dg-final { scan-rtl-dump-not "Web oldreg" "web" } } */
+/* { dg-final { cleanup-rtl-dump "web" } } */
Index: web.c
===================================================================
--- web.c	(revision 168504)
+++ web.c	(working copy)
@@ -260,7 +260,11 @@ entry_register (struct web_entry *entry,
      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;
+    {
+      newreg = reg;
+      if (!used[REGNO (reg)])
+	used[REGNO (reg)] = 1;
+    }
   else
     {
       newreg = gen_reg_rtx (GET_MODE (reg));

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

[Jeff Law]

-----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.

jeff
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJNJLPgAAoJEBRtltQi2kC7FjMIAJWlwPynuhRfcEt/ibitrXEc
1Rkk1Yck3gBhiSjvipJzcGFPlsaSqeDSsBrG2Ovf1l6CVnyWpfduQhtGAuShgPVY
+cLlNiqAUlDJC7HTc/DtREacDA5XkcCeRm22q2UxZ5OMgQhPyD60VUGdgGrxMnHS
tFpCAq20Xi3wdu+LD+6XiFQGBnwzUJYBsTDEMSFoJ7O2H9gUH8ffHczGKXsX6+pc
4QDf727I9qLt63GLWgltmtxDBGaYEKbd0+XD8A+z9zfBx39o6ZStw36T0YwvSQXj
ehnERyT2jbq1D3JyQiOlHcUb/8/c3JO1cnQCqVtFmhqzbeQyQZZSiPScR/DqITo=
=ncqV
-----END PGP SIGNATURE-----

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

[Jie Zhang]

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
$


Regards,
-- 
Jie Zhang

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

[Jie Zhang]

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
> $
>
>

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

[Jie Zhang]

PING.

On 01/10/2011 10:47 AM, Jie Zhang wrote:
> 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
>> $
>>
>>

-- 
Jie Zhang

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

[Alexandre Oliva]

On Jan 10, 2011, Jie Zhang <jie@codesourcery.com> wrote:

> 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.

Yeah, now that I looked again at my patch from back then, yours makes
perfect sense to me.  Thanks!

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer

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

[Jie Zhang]

Alex,

On 01/17/2011 11:05 PM, Alexandre Oliva wrote:
> On Jan 10, 2011, Jie Zhang<jie@codesourcery.com>  wrote:
>
>> 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.
>
> Yeah, now that I looked again at my patch from back then, yours makes
> perfect sense to me.  Thanks!
>
Thanks! But I think I still need approval, right?

Jeff, could you approve my patch?

Regards,
-- 
Jie Zhang

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

[Jie Zhang]

Hi Richard,

You approved Alexandre's original patch. Could you please also approve 
my patch which fixes a bug in that patch? Thanks!

On 01/17/2011 11:05 PM, Alexandre Oliva wrote:
> On Jan 10, 2011, Jie Zhang<jie@codesourcery.com>  wrote:
>
>> 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.
>
> Yeah, now that I looked again at my patch from back then, yours makes
> perfect sense to me.  Thanks!
>

-- 
Jie Zhang

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

[Richard Guenther]

On Tue, Jan 25, 2011 at 4:43 AM, Jie Zhang <jie@codesourcery.com> wrote:
> Hi Richard,
>
> You approved Alexandre's original patch. Could you please also approve my
> patch which fixes a bug in that patch? Thanks!

Did I? ...

Well, I'm not familiar with the code, so I'll defer to somebody else.

Richard.

> On 01/17/2011 11:05 PM, Alexandre Oliva wrote:
>>
>> On Jan 10, 2011, Jie Zhang<jie@codesourcery.com>  wrote:
>>
>>> 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.
>>
>> Yeah, now that I looked again at my patch from back then, yours makes
>> perfect sense to me.  Thanks!
>>
>
> --
> Jie Zhang
>
>

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

[Jie Zhang]

On 01/25/2011 08:08 PM, Richard Guenther wrote:
> On Tue, Jan 25, 2011 at 4:43 AM, Jie Zhang<jie@codesourcery.com>  wrote:
>> Hi Richard,
>>
>> You approved Alexandre's original patch. Could you please also approve my
>> patch which fixes a bug in that patch? Thanks!
>
> Did I? ...
>
Yes, you did. See

http://gcc.gnu.org/ml/gcc-patches/2010-01/msg00372.html

> Well, I'm not familiar with the code, so I'll defer to somebody else.
>
Alexandre has already confirmed that my fix is OK. But it seems he could 
not approve it since his original patch was approved by you. So I 
thought you must be familiar with the code and could also approve mine.

>> On 01/17/2011 11:05 PM, Alexandre Oliva wrote:
>>>
>>> On Jan 10, 2011, Jie Zhang<jie@codesourcery.com>    wrote:
>>>
>>>> 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.
>>>
>>> Yeah, now that I looked again at my patch from back then, yours makes
>>> perfect sense to me.  Thanks!
>>>

Regards,
-- 
Jie Zhang

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

[Richard Guenther]

On Tue, Jan 25, 2011 at 1:15 PM, Jie Zhang <jie@codesourcery.com> wrote:
> On 01/25/2011 08:08 PM, Richard Guenther wrote:
>>
>> On Tue, Jan 25, 2011 at 4:43 AM, Jie Zhang<jie@codesourcery.com>  wrote:
>>>
>>> Hi Richard,
>>>
>>> You approved Alexandre's original patch. Could you please also approve my
>>> patch which fixes a bug in that patch? Thanks!
>>
>> Did I? ...
>>
> Yes, you did. See
>
> http://gcc.gnu.org/ml/gcc-patches/2010-01/msg00372.html
>
>> Well, I'm not familiar with the code, so I'll defer to somebody else.
>>
> Alexandre has already confirmed that my fix is OK. But it seems he could not
> approve it since his original patch was approved by you. So I thought you
> must be familiar with the code and could also approve mine.

Heh, I relied on Steven here.  As Jeff already had a look maybe he
can approve your patch.

Richard.

>>> On 01/17/2011 11:05 PM, Alexandre Oliva wrote:
>>>>
>>>> On Jan 10, 2011, Jie Zhang<jie@codesourcery.com>    wrote:
>>>>
>>>>> 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.
>>>>
>>>> Yeah, now that I looked again at my patch from back then, yours makes
>>>> perfect sense to me.  Thanks!
>>>>
>
> Regards,
> --
> Jie Zhang
>
>

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

[Jie Zhang]

On 01/25/2011 08:18 PM, Richard Guenther wrote:
> On Tue, Jan 25, 2011 at 1:15 PM, Jie Zhang<jie@codesourcery.com>  wrote:
>> On 01/25/2011 08:08 PM, Richard Guenther wrote:
>>>
>>> On Tue, Jan 25, 2011 at 4:43 AM, Jie Zhang<jie@codesourcery.com>    wrote:
>>>>
>>>> Hi Richard,
>>>>
>>>> You approved Alexandre's original patch. Could you please also approve my
>>>> patch which fixes a bug in that patch? Thanks!
>>>
>>> Did I? ...
>>>
>> Yes, you did. See
>>
>> http://gcc.gnu.org/ml/gcc-patches/2010-01/msg00372.html
>>
>>> Well, I'm not familiar with the code, so I'll defer to somebody else.
>>>
>> Alexandre has already confirmed that my fix is OK. But it seems he could not
>> approve it since his original patch was approved by you. So I thought you
>> must be familiar with the code and could also approve mine.
>
> Heh, I relied on Steven here.  As Jeff already had a look maybe he
> can approve your patch.
>
I asked Jeff, but there was no reply. So I thought the quickest way to 
get my patch approved might be asking you.

>>>> On 01/17/2011 11:05 PM, Alexandre Oliva wrote:
>>>>>
>>>>> On Jan 10, 2011, Jie Zhang<jie@codesourcery.com>      wrote:
>>>>>
>>>>>> 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.
>>>>>
>>>>> Yeah, now that I looked again at my patch from back then, yours makes
>>>>> perfect sense to me.  Thanks!
>>>>>


Regards,
Jie

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

[Jeff Law]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 01/05/11 19:21, 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":
>>>
>>
[ ... [
Not forgotten.  It's a bit of a hectic week here...  Things will be more
normal time-wise next week.

Jeff
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJNQkL9AAoJEBRtltQi2kC77c0H/AxlNNJB/F1vVXQ/XnxwuwID
/ZJRwr2Xm68p+Cu4/7JjqCH9uSA+Yub5Qbs6MD1itofvhX0AEoBd3hZEgYQpKzwl
djk+Lu2VzaP69yb/inm924vWPSFMfJx4Z+ohR7kDMg0SWAtlMvG64PYqPB7DsnI4
lXyqdtEsAgfW2F3ePloDikStySmTpotXwrdR1wNqM++MSHvsNJNYue6Ko5SCWXmO
PlaEXRdz8GZyn8HnSSNoQO3BbDXxukimyrGseNQdhPHBa7Os2cF022hC5KN0vxJu
w5326qXl+JbHAraAeIYpGPz81BHtVZBdz/RL2h1vt/uWca6oe9K/5DGxaXx8j2E=
=1kKN
-----END PGP SIGNATURE-----

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

[Jie Zhang]

On 01/28/2011 12:15 PM, Jeff Law wrote:
> Not forgotten.  It's a bit of a hectic week here...  Things will be more
> normal time-wise next week.
>
I will wait.


Regards,
-- 
Jie Zhang

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

[Jeff Law]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Sorry for the long delay -- I probably should have asked someone else to
own this since my initial comments were solely on the CLOBBER issue and
I didn't have any familiarity with the web pass.

Regardless,


On 01/05/11 19:21, Jie Zhang wrote:

>>
> 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.
So just so I'm certain I understand the problem.  In the original
testcase a naked CLOBBER was the "set" that triggered the problem, but
this problem can occur for assignments to any uninitialized pseudo, such
as in examples you provided below.

When we see a set to an uninitialized pseudo, we're losing the saved
DF_REF_UID which allows us to combine all the uninitialized uses into a
single web.  Right?

Assuming that those statements are correct, the patch is OK.  I would
suggest including both the testcase derived from dhrystone as well as
the one in this message in the testsuite.

As a 4.7 cleanup, you might consider ignoring naked clobbers in the
FOR_BB_INSNS loops within web_main.


> 
> 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
Please include the testcase above as well as the one from dhrystone in
the testsuite.

Thanks,
Jeff
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJNStuCAAoJEBRtltQi2kC7UGEIAJtLzraBzS3tofASAnXDV+JA
Hcfh9HFhbP+Nf86r0ZX9p1ka5kZ+tFCtfvaNmVSn0WPoVILq5IzXojpN+0L1pd7d
OvxKaHqDsoQ42FrRcsaa6KqvUGPWjkiYFo/48IiTWgkRLsR9U4K9Tr0wdicnfYqK
cRi7pBPLeZR9mROpwXHDjyO5naPNfLOYRiOkJ90Io7MZtPqL6jRi3/5Mrrif4UV/
HR7np3ejGZx1kP6atb21iZZmRyFgThbpT8CA9wxF+FF6+rvUDS/ZYxwN7ym0TYYr
LN5QFOxbkFQ9T3IyZAqEfbIEmmrXVnZbn6rXNy3Gfhy3zmaLG3MS5gPZlMGZsAI=
=3mqj
-----END PGP SIGNATURE-----

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

[Jie Zhang]

[-- Attachment #1: Type: text/plain, Size: 1469 bytes --]

Hi Jeff,

On 02/04/2011 12:44 AM, Jeff Law wrote:
> Sorry for the long delay -- I probably should have asked someone else to
> own this since my initial comments were solely on the CLOBBER issue and
> I didn't have any familiarity with the web pass.
>
No problem. Thanks for review! :-)

> On 01/05/11 19:21, Jie Zhang wrote:
>> 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.
> So just so I'm certain I understand the problem.  In the original
> testcase a naked CLOBBER was the "set" that triggered the problem, but
> this problem can occur for assignments to any uninitialized pseudo, such
> as in examples you provided below.
>
> When we see a set to an uninitialized pseudo, we're losing the saved
> DF_REF_UID which allows us to combine all the uninitialized uses into a
> single web.  Right?
>
Yes. My patch just prevents this losing.

> Assuming that those statements are correct, the patch is OK.  I would
> suggest including both the testcase derived from dhrystone as well as
> the one in this message in the testsuite.
>
That test case is already in the testsuite. I just update that test case 
to check the issue I found.

I have committed the attached patch on trunk.


Regards,
-- 
Jie Zhang

[-- Attachment #2: gcc-pr42631-fix-fix-2.diff --]
[-- Type: text/x-diff, Size: 1749 bytes --]


	PR debug/42631
	* web.c (entry_register): Don't clobber the number of the
	first uninitialized reference in used[].

	testsuite/
	PR debug/42631
	* gcc.dg/pr42631.c: Update test.
	* gcc.dg/pr42631-2.c: New test.

Index: testsuite/gcc.dg/pr42631-2.c
===================================================================
--- testsuite/gcc.dg/pr42631-2.c	(revision 0)
+++ testsuite/gcc.dg/pr42631-2.c	(revision 0)
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -funroll-loops -fdump-rtl-web" } */
+
+foo()
+{
+}
+
+/* { dg-final { scan-rtl-dump-not "Web oldreg" "web" } } */
+/* { dg-final { cleanup-rtl-dump "web" } } */
Index: testsuite/gcc.dg/pr42631.c
===================================================================
--- testsuite/gcc.dg/pr42631.c	(revision 169850)
+++ testsuite/gcc.dg/pr42631.c	(working copy)
@@ -14,10 +14,13 @@
    combine uninitialized uses into a single web.  */
 
 /* { dg-do compile } */
-/* { dg-options "-g -O1 -funroll-loops -fcompare-debug" } */
+/* { dg-options "-g -O1 -funroll-loops -fcompare-debug -fdump-rtl-web" } */
 
 void foo()
 {
   unsigned k;
   while (--k > 0);
 }
+
+/* { dg-final { scan-rtl-dump-not "Web oldreg" "web" } } */
+/* { dg-final { cleanup-rtl-dump "web" } } */
Index: web.c
===================================================================
--- web.c	(revision 169850)
+++ web.c	(working copy)
@@ -260,7 +260,11 @@ entry_register (struct web_entry *entry,
      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;
+    {
+      newreg = reg;
+      if (!used[REGNO (reg)])
+	used[REGNO (reg)] = 1;
+    }
   else
     {
       newreg = gen_reg_rtx (GET_MODE (reg));

Ignore clobber def when replacing register in WEB pass (was Re: Fix a bug in merging uninitialized refs into a single web)

[Jie Zhang]

[-- Attachment #1: Type: text/plain, Size: 1545 bytes --]

Hi Jeff,

On 02/05/2011 08:05 PM, Jie Zhang wrote:
>>> 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.
>> So just so I'm certain I understand the problem. In the original
>> testcase a naked CLOBBER was the "set" that triggered the problem, but
>> this problem can occur for assignments to any uninitialized pseudo, such
>> as in examples you provided below.
>>
>> When we see a set to an uninitialized pseudo, we're losing the saved
>> DF_REF_UID which allows us to combine all the uninitialized uses into a
>> single web. Right?
>>
> Yes. My patch just prevents this losing.
>
While looking into PR 47622, which was caused by my patch, I found my 
patch was wrong. For example

reg 134                 <-- is uninitialized
use (reg 134)           <-- use1
set (reg 134)           <-- def2
use (reg 134)           <-- use2

use1 forms a web, def2 and use2 form another web. In such case, we still 
want (reg 134) in the second web to be renamed. But with my patch, it 
will not. That bug I found in Alex's patch is *not* real and I have 
reverted my patch. Sorry for the huge noise I caused.

A new patch is attached, which just follows Jeff's suggestion to ignore 
clobber in the web pass. Testing is still going on. Is it OK if the 
testing is good?


Regards,
-- 
Jie Zhang


[-- Attachment #2: gcc-web-ignore-clobber.diff --]
[-- Type: text/x-diff, Size: 1369 bytes --]


	* web.c (web_main): Ignore clobber def when replacing register.

	testsuite/
	* gcc.dg/pr42631-2.c: New test.

Index: testsuite/gcc.dg/pr42631-2.c
===================================================================
--- testsuite/gcc.dg/pr42631-2.c	(revision 0)
+++ testsuite/gcc.dg/pr42631-2.c	(revision 0)
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -funroll-loops -fdump-rtl-web" } */
+
+foo()
+{
+}
+
+/* { dg-final { scan-rtl-dump-not "Web oldreg" "web" } } */
+/* { dg-final { cleanup-rtl-dump "web" } } */
Index: web.c
===================================================================
--- web.c	(revision 169997)
+++ web.c	(working copy)
@@ -396,7 +396,17 @@ web_main (void)
 	  for (def_rec = DF_INSN_UID_DEFS (uid); *def_rec; def_rec++)
 	    {
 	      df_ref def = *def_rec;
-	      if (DF_REF_REGNO (def) >= FIRST_PSEUDO_REGISTER)
+	      if (DF_REF_REGNO (def) >= FIRST_PSEUDO_REGISTER
+		  /* Ignore the def if it's a clobber.  For example, reg 134
+		     in the second insn of the following sequence will not be
+		     replaced.
+
+		       (insn (clobber (reg:SI 134)))
+
+		       (insn (set (reg:SI 0 r0) (reg:SI 134)))
+
+		     Thus the later passes can optimize them away.  */
+		  && !DF_REF_FLAGS_IS_SET (def, DF_REF_MUST_CLOBBER))
 		replace_ref (def, entry_register (def_entry + DF_REF_ID (def), def, used));
 	    }
 	}

Re: Ignore clobber def when replacing register in WEB pass (was Re: Fix a bug in merging uninitialized refs into a single web)

[Jie Zhang]

On 02/10/2011 01:05 PM, Jie Zhang wrote:
> Hi Jeff,
>
> On 02/05/2011 08:05 PM, Jie Zhang wrote:
>>>> 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.
>>> So just so I'm certain I understand the problem. In the original
>>> testcase a naked CLOBBER was the "set" that triggered the problem, but
>>> this problem can occur for assignments to any uninitialized pseudo, such
>>> as in examples you provided below.
>>>
>>> When we see a set to an uninitialized pseudo, we're losing the saved
>>> DF_REF_UID which allows us to combine all the uninitialized uses into a
>>> single web. Right?
>>>
>> Yes. My patch just prevents this losing.
>>
> While looking into PR 47622, which was caused by my patch, I found my
> patch was wrong. For example
>
> reg 134 <-- is uninitialized
> use (reg 134) <-- use1
> set (reg 134) <-- def2
> use (reg 134) <-- use2
>
> use1 forms a web, def2 and use2 form another web. In such case, we still
> want (reg 134) in the second web to be renamed. But with my patch, it
> will not. That bug I found in Alex's patch is *not* real and I have
> reverted my patch. Sorry for the huge noise I caused.
>
> A new patch is attached, which just follows Jeff's suggestion to ignore
> clobber in the web pass. Testing is still going on. Is it OK if the
> testing is good?
>
Testing arm-none-linux-gnueabi on qemu shows no regressions. Also 
bootstrapped and regression tested natively on x86_64-unknown-linux-gnu.


-- 
Jie Zhang

Re: Ignore clobber def when replacing register in WEB pass (was Re: Fix a bug in merging uninitialized refs into a single web)

[Jeff Law]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 02/09/11 22:05, Jie Zhang wrote:
> Hi Jeff,
> 
> On 02/05/2011 08:05 PM, Jie Zhang wrote:
>>>> 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.
>>> So just so I'm certain I understand the problem. In the original
>>> testcase a naked CLOBBER was the "set" that triggered the problem, but
>>> this problem can occur for assignments to any uninitialized pseudo, such
>>> as in examples you provided below.
>>>
>>> When we see a set to an uninitialized pseudo, we're losing the saved
>>> DF_REF_UID which allows us to combine all the uninitialized uses into a
>>> single web. Right?
>>>
>> Yes. My patch just prevents this losing.
>>
> While looking into PR 47622, which was caused by my patch, I found my
> patch was wrong. For example
> 
> reg 134                 <-- is uninitialized
> use (reg 134)           <-- use1
> set (reg 134)           <-- def2
> use (reg 134)           <-- use2
> 
> use1 forms a web, def2 and use2 form another web. In such case, we still
> want (reg 134) in the second web to be renamed. But with my patch, it
> will not. That bug I found in Alex's patch is *not* real and I have
> reverted my patch. Sorry for the huge noise I caused.
> 
> A new patch is attached, which just follows Jeff's suggestion to ignore
> clobber in the web pass. Testing is still going on. Is it OK if the
> testing is good?
Is there some reason you don't just do something like

if (NONDEBUG_INSN_P (insn) && GET_CODE (insn) != CLOBBER)

In the outer conditional?

That's more typical of how I ignore naked clobbers.  Otherwise don't you
run the risk of ignoring a clobber which appears inside a normal insn?

jeff
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJNWp2hAAoJEBRtltQi2kC7MAAH/1t4LH7tgJmKZXyANy6BbymM
8sKqqHNcKjx00HYunJw0kLpzCfbKtVWYC8wKWFMuyzKFr21b5NWyKKNO+wAcPaDx
vM6TS/KM3Zz1TrL9T9lUq/Hzx7MDZAICtG9x4/LnT9DivaNiYGMDySlRNz7qylUN
+JuAjZiLW6McbE7Ag9E+grBqnHGuTAE1pXoyHcBHxT44Wu2r/F/71QxaC81Eh2kS
UCxKjzB+3siL1BkfA6OHWsdjPZspM7RoKPKc2Kbnuehnua9njxptnuQUg7V/I9rB
BLYi7/pZpYh5Irpp6BhfMPi7S54sEpTaZoF3CJMSqle6vHjej0l4UJWCxU0LnLw=
=+dCO
-----END PGP SIGNATURE-----

Re: Ignore clobber def when replacing register in WEB pass (was Re: Fix a bug in merging uninitialized refs into a single web)

[Jie Zhang]

On 02/15/2011 11:37 PM, Jeff Law wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 02/09/11 22:05, Jie Zhang wrote:
>> Hi Jeff,
>>
>> On 02/05/2011 08:05 PM, Jie Zhang wrote:
>>>>> 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.
>>>> So just so I'm certain I understand the problem. In the original
>>>> testcase a naked CLOBBER was the "set" that triggered the problem, but
>>>> this problem can occur for assignments to any uninitialized pseudo, such
>>>> as in examples you provided below.
>>>>
>>>> When we see a set to an uninitialized pseudo, we're losing the saved
>>>> DF_REF_UID which allows us to combine all the uninitialized uses into a
>>>> single web. Right?
>>>>
>>> Yes. My patch just prevents this losing.
>>>
>> While looking into PR 47622, which was caused by my patch, I found my
>> patch was wrong. For example
>>
>> reg 134<-- is uninitialized
>> use (reg 134)<-- use1
>> set (reg 134)<-- def2
>> use (reg 134)<-- use2
>>
>> use1 forms a web, def2 and use2 form another web. In such case, we still
>> want (reg 134) in the second web to be renamed. But with my patch, it
>> will not. That bug I found in Alex's patch is *not* real and I have
>> reverted my patch. Sorry for the huge noise I caused.
>>
>> A new patch is attached, which just follows Jeff's suggestion to ignore
>> clobber in the web pass. Testing is still going on. Is it OK if the
>> testing is good?
> Is there some reason you don't just do something like
>
> if (NONDEBUG_INSN_P (insn)&&  GET_CODE (insn) != CLOBBER)
>
> In the outer conditional?
>
> That's more typical of how I ignore naked clobbers.  Otherwise don't you
> run the risk of ignoring a clobber which appears inside a normal insn?
>
I don't know why we should not ignore a clobber inside a normal insn. I 
wrote the patch in that way was trying to ignore a clobber inside a 
normal insn.


Regards,
-- 
Jie Zhang

Re: Ignore clobber def when replacing register in WEB pass (was Re: Fix a bug in merging uninitialized refs into a single web)

[Jie Zhang]

[-- Attachment #1: Type: text/plain, Size: 2680 bytes --]

On 02/16/2011 03:24 PM, Jie Zhang wrote:
> On 02/15/2011 11:37 PM, Jeff Law wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> On 02/09/11 22:05, Jie Zhang wrote:
>>> Hi Jeff,
>>>
>>> On 02/05/2011 08:05 PM, Jie Zhang wrote:
>>>>>> 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.
>>>>> So just so I'm certain I understand the problem. In the original
>>>>> testcase a naked CLOBBER was the "set" that triggered the problem, but
>>>>> this problem can occur for assignments to any uninitialized pseudo,
>>>>> such
>>>>> as in examples you provided below.
>>>>>
>>>>> When we see a set to an uninitialized pseudo, we're losing the saved
>>>>> DF_REF_UID which allows us to combine all the uninitialized uses
>>>>> into a
>>>>> single web. Right?
>>>>>
>>>> Yes. My patch just prevents this losing.
>>>>
>>> While looking into PR 47622, which was caused by my patch, I found my
>>> patch was wrong. For example
>>>
>>> reg 134<-- is uninitialized
>>> use (reg 134)<-- use1
>>> set (reg 134)<-- def2
>>> use (reg 134)<-- use2
>>>
>>> use1 forms a web, def2 and use2 form another web. In such case, we still
>>> want (reg 134) in the second web to be renamed. But with my patch, it
>>> will not. That bug I found in Alex's patch is *not* real and I have
>>> reverted my patch. Sorry for the huge noise I caused.
>>>
>>> A new patch is attached, which just follows Jeff's suggestion to ignore
>>> clobber in the web pass. Testing is still going on. Is it OK if the
>>> testing is good?
>> Is there some reason you don't just do something like
>>
>> if (NONDEBUG_INSN_P (insn)&& GET_CODE (insn) != CLOBBER)
>>
>> In the outer conditional?
>>
>> That's more typical of how I ignore naked clobbers. Otherwise don't you
>> run the risk of ignoring a clobber which appears inside a normal insn?
>>
> I don't know why we should not ignore a clobber inside a normal insn. I
> wrote the patch in that way was trying to ignore a clobber inside a
> normal insn.
>
Anyway I have also tested the attached patch as your advice. Testing 
arm-none-linux-gnueabi on qemu shows no regressions. Also bootstrapped 
and regression tested natively on x86_64-unknown-linux-gnu.

If you like this one better, I do not object. It seems safer than the 
previous one and also fixes the issue I concern.

I also reported a new PR and use that PR for the test case file name.

Regards,
-- 
Jie Zhang


[-- Attachment #2: gcc-web-ignore-clobber-2.diff --]
[-- Type: text/x-diff, Size: 1215 bytes --]


	* web.c (web_main): Ignore naked clobber when replacing register.

	testsuite/
	* gcc.dg/pr47763.c: New test.

Index: testsuite/gcc.dg/pr47763.c
===================================================================
--- testsuite/gcc.dg/pr47763.c	(revision 0)
+++ testsuite/gcc.dg/pr47763.c	(revision 0)
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -funroll-loops -fdump-rtl-web" } */
+
+foo()
+{
+}
+
+/* { dg-final { scan-rtl-dump-not "Web oldreg" "web" } } */
+/* { dg-final { cleanup-rtl-dump "web" } } */
Index: web.c
===================================================================
--- web.c	(revision 169997)
+++ web.c	(working copy)
@@ -377,7 +377,17 @@ web_main (void)
     FOR_BB_INSNS (bb, insn)
     {
       unsigned int uid = INSN_UID (insn);
-      if (NONDEBUG_INSN_P (insn))
+
+      if (NONDEBUG_INSN_P (insn)
+	  /* Ignore naked clobber.  For example, reg 134 in the second insn
+	     of the following sequence will not be replaced.
+
+	       (insn (clobber (reg:SI 134)))
+
+	       (insn (set (reg:SI 0 r0) (reg:SI 134)))
+
+	     Thus the later passes can optimize them away.  */
+	  && GET_CODE (PATTERN (insn)) != CLOBBER)
 	{
 	  df_ref *use_rec;
 	  df_ref *def_rec;

Re: Ignore clobber def when replacing register in WEB pass (was Re: Fix a bug in merging uninitialized refs into a single web)

[Jeff Law]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 02/17/11 20:58, Jie Zhang wrote:
>>
> Anyway I have also tested the attached patch as your advice. Testing
> arm-none-linux-gnueabi on qemu shows no regressions. Also bootstrapped
> and regression tested natively on x86_64-unknown-linux-gnu.
> 
> If you like this one better, I do not object. It seems safer than the
> previous one and also fixes the issue I concern.
> 
> I also reported a new PR and use that PR for the test case file name.
This is OK.  Sorry for the long review cycle times.

Jeff
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJNY9/rAAoJEBRtltQi2kC7+LYIAK72iW9S/XDto5T4IKNvVv6W
sZ6X7Dn+ep10jMaC2BfX/3F6Cly1DS4SmW0pzF4mMkr4KFwfLrZrDxdr0Qn+eTlO
vVkcgEeH+FiiZoDALLgAt6oc0z7hDY3Ny+hqAXe2+z60EQVnzPpQSVr5bxQjA252
eehBhijFbgXVq3EPLqryqA73P+bGrp7ZS9tjDDmUeEEZ777pjxJjO+EhcLFEBa/c
rKV5nmbBQtSHClr4w+Zd5XP4ZGpudkmicnxCHXY8EOXzAg5Vlqi6QUFA6zf+dxNd
c6ggHUeTcO55+WYVukMSt8ZdC9nkO7GVWduzE8gbfOuxXphyc68QjU1vRNrOAtw=
=BOhD
-----END PGP SIGNATURE-----

Re: Ignore clobber def when replacing register in WEB pass (was Re: Fix a bug in merging uninitialized refs into a single web)

[Jie Zhang]

On 02/23/2011 12:10 AM, Jeff Law wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 02/17/11 20:58, Jie Zhang wrote:
>>>
>> Anyway I have also tested the attached patch as your advice. Testing
>> arm-none-linux-gnueabi on qemu shows no regressions. Also bootstrapped
>> and regression tested natively on x86_64-unknown-linux-gnu.
>>
>> If you like this one better, I do not object. It seems safer than the
>> previous one and also fixes the issue I concern.
>>
>> I also reported a new PR and use that PR for the test case file name.
> This is OK.  Sorry for the long review cycle times.
>
Committed on trunk. Thanks!

Regards,
-- 
Jie Zhang