| Message ID | 20211116114422.16071-1-rjiejie@linux.alibaba.com |
|---|---|
| State | New |
| Headers | show |
| Series | regrename: Skip renaming if instruction is noop move. | expand |
On Tue, Nov 16, 2021 at 12:45 PM Jojo R via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > Skip renaming if instruction is noop move, and it will > been removed for performance. Is there any (target specific) testcase you can add? Such commits are problematic when later bisected to since the intent isn't clear. > gcc/ > * regrename.c (find_rename_reg): Return satisfied regno > if instruction is noop move. > --- > gcc/regrename.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/gcc/regrename.c b/gcc/regrename.c > index b8a9ca36f22..cb605f5176b 100644 > --- a/gcc/regrename.c > +++ b/gcc/regrename.c > @@ -394,6 +394,9 @@ find_rename_reg (du_head_p this_head, enum reg_class super_class, > this_head, *unavailable)) > return this_head->tied_chain->regno; > > + if (noop_move_p (this_head->first->insn)) > + return best_new_reg; > + > /* If PREFERRED_CLASS is not NO_REGS, we iterate in the first pass > over registers that belong to PREFERRED_CLASS and try to find the > best register within the class. If that failed, we iterate in > -- > 2.24.3 (Apple Git-128) >
— Jojo 在 2021年11月16日 +0800 PM8:12，Richard Biener <richard.guenther@gmail.com>，写道： > On Tue, Nov 16, 2021 at 12:45 PM Jojo R via Gcc-patches > <gcc-patches@gcc.gnu.org> wrote: > > > > Skip renaming if instruction is noop move, and it will > > been removed for performance. > > Is there any (target specific) testcase you can add? Such commits are > problematic > when later bisected to since the intent isn't clear. I made a issue in bugzilla, please check it, thanks. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103296 > > > gcc/ > > * regrename.c (find_rename_reg): Return satisfied regno > > if instruction is noop move. > > --- > > gcc/regrename.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/gcc/regrename.c b/gcc/regrename.c > > index b8a9ca36f22..cb605f5176b 100644 > > --- a/gcc/regrename.c > > +++ b/gcc/regrename.c > > @@ -394,6 +394,9 @@ find_rename_reg (du_head_p this_head, enum reg_class super_class, > > this_head, *unavailable)) > > return this_head->tied_chain->regno; > > > > + if (noop_move_p (this_head->first->insn)) > > + return best_new_reg; > > + > > /* If PREFERRED_CLASS is not NO_REGS, we iterate in the first pass > > over registers that belong to PREFERRED_CLASS and try to find the > > best register within the class. If that failed, we iterate in > > -- > > 2.24.3 (Apple Git-128)
On 11/16/2021 7:20 PM, Jojo R via Gcc-patches wrote: > — Jojo > 在 2021年11月16日 +0800 PM8:12，Richard Biener <richard.guenther@gmail.com>，写道： >> On Tue, Nov 16, 2021 at 12:45 PM Jojo R via Gcc-patches >> <gcc-patches@gcc.gnu.org> wrote: >>> Skip renaming if instruction is noop move, and it will >>> been removed for performance. >> Is there any (target specific) testcase you can add? Such commits are >> problematic >> when later bisected to since the intent isn't clear. > I made a issue in bugzilla, please check it, thanks. > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103296 So what Richi is asking is can you construct a testcase for the testsuite? Having a BZ is helpful because we can reference it in the commit message, but a test, even if it's target specific, is even better from a long term maintenance standpoint. Jeff
— Jojo 在 2021年11月19日 +0800 AM12:13，Jeff Law <jeffreyalaw@gmail.com>，写道： > > > On 11/16/2021 7:20 PM, Jojo R via Gcc-patches wrote: > > — Jojo > > 在 2021年11月16日 +0800 PM8:12，Richard Biener <richard.guenther@gmail.com>，写道： > > > On Tue, Nov 16, 2021 at 12:45 PM Jojo R via Gcc-patches > > > <gcc-patches@gcc.gnu.org> wrote: > > > > Skip renaming if instruction is noop move, and it will > > > > been removed for performance. > > > Is there any (target specific) testcase you can add? Such commits are > > > problematic > > > when later bisected to since the intent isn't clear. > > I made a issue in bugzilla, please check it, thanks. > > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103296 > So what Richi is asking is can you construct a testcase for the > testsuite? Having a BZ is helpful because we can reference it in the > commit message, but a test, even if it's target specific, is even better > from a long term maintenance standpoint. > I found this issue from the ISA extension vector of risc-v target, and It has not been upstream by now, normal test case without vector isa Is difficult to construct for this patch, but I think it’s simple and useful for other ISAs or targets, or recommit this patch after our risc-v Vector ISA Is ready on master branch ? Any suggestions ? > Jeff
On 11/18/2021 11:23 PM, Jojo R wrote: > > — Jojo > 在 2021年11月19日 +0800 AM12:13，Jeff Law <jeffreyalaw@gmail.com>，写道： > > > > On 11/16/2021 7:20 PM, Jojo R via Gcc-patches wrote: > > — Jojo > 在 2021年11月16日 +0800 PM8:12，Richard Biener > <richard.guenther@gmail.com>，写道： > > On Tue, Nov 16, 2021 at 12:45 PM Jojo R via Gcc-patches > <gcc-patches@gcc.gnu.org> wrote: > > Skip renaming if instruction is noop move, and it will > been removed for performance. > > Is there any (target specific) testcase you can add? Such > commits are > problematic > when later bisected to since the intent isn't clear. > > I made a issue in bugzilla, please check it, thanks. > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103296 > > So what Richi is asking is can you construct a testcase for the > testsuite? Having a BZ is helpful because we can reference it in the > commit message, but a test, even if it's target specific, is even > better > from a long term maintenance standpoint. > > I found this issue from the ISA extension vector of risc-v target, and > It has not been upstream by now, normal test case without vector isa > Is difficult to construct for this patch, but I think it’s simple and > useful for > other ISAs or targets, or recommit this patch after our risc-v Vector ISA > Is ready on master branch ? > > Any suggestions ? So I tried to trigger this on x86, but wasn't able with relatively light testing. My recommendation would be to add a little comment like this before the change: /* If this insn is a noop move, then do not rename in this chain as doing so would inhibit removal of the noop move. */ OK with that change. Thanks for your patience. jeff
diff --git a/gcc/regrename.c b/gcc/regrename.c index b8a9ca36f22..cb605f5176b 100644 --- a/gcc/regrename.c +++ b/gcc/regrename.c @@ -394,6 +394,9 @@ find_rename_reg (du_head_p this_head, enum reg_class super_class, this_head, *unavailable)) return this_head->tied_chain->regno; + if (noop_move_p (this_head->first->insn)) + return best_new_reg; + /* If PREFERRED_CLASS is not NO_REGS, we iterate in the first pass over registers that belong to PREFERRED_CLASS and try to find the best register within the class. If that failed, we iterate in