diff mbox

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

Message ID 4D5DEE6D.4040709@codesourcery.com
State New
Headers show

Commit Message

Jie Zhang Feb. 18, 2011, 3:58 a.m. UTC 
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,

Comments

Jeff Law Feb. 22, 2011, 4:10 p.m. UTC | #1 
-----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-----
Jie Zhang Feb. 23, 2011, 12:26 a.m. UTC | #2 
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,
diff mbox

Patch


	* 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;