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 <jie@codesourcery.com>]

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