| Message ID | 4D537233.70409@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
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.
-----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-----
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,
* 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)); } }