| Message ID | 563229DD.8080900@arm.com |
|---|---|
| State | New |
| Headers | show |
Ping. https://gcc.gnu.org/ml/gcc-patches/2015-10/msg03170.html Thanks, Kyrill On 29/10/15 14:14, Kyrill Tkachov wrote: > > On 29/10/15 14:00, Marcus Shawcroft wrote: >> On 29 October 2015 at 13:50, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote: >> >>>>> Ok for trunk? >>>> rtl.h exposes reg_or_subregno() already doesn't that do what we need here? >>> >>> reg_or_subregno assumes that what it's passed is REG or a SUBREG. >>> It will ICE on any other rtx. Here I want to strip the subreg if it is >>> a subreg, but leave it as it is otherwise. >> OK, I follow. >> >>>> The test case is not aarch64 specific therefore I think convention is >>>> that it should go into a generic directory. >>> >>> Ok, I'll put it in gcc.dg/ >> >> OK with the test case moved. Thanks /Marcus >> > Thanks, but I'd like to do a slight respin. > The testcase is moved to gcc.dg but I also avoid creating the new > helper function and just do the SUBREG extraction once at the very end. > This makes the patch smaller. > > Since you're ok with the approach and this revision is logically equivalent, > I just need an ok from an arm perspective. > > Thanks, > Kyrill > > 2015-10-29 Kyrylo Tkachov <kyrylo.tkachov@arm.com> > > PR target/68088 > * config/arm/aarch-common.c (aarch_accumulator_forwarding): Strip > subregs from accumulator and make sure it's a register. > > 2015-10-29 Kyrylo Tkachov <kyrylo.tkachov@arm.com> > > PR target/68088 > * gcc.dg/pr68088_1.c: New test. >
On 29/10/15 14:14, Kyrill Tkachov wrote: > > On 29/10/15 14:00, Marcus Shawcroft wrote: >> On 29 October 2015 at 13:50, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote: >> >>>>> Ok for trunk? >>>> rtl.h exposes reg_or_subregno() already doesn't that do what we need here? >>> >>> reg_or_subregno assumes that what it's passed is REG or a SUBREG. >>> It will ICE on any other rtx. Here I want to strip the subreg if it is >>> a subreg, but leave it as it is otherwise. >> OK, I follow. >> >>>> The test case is not aarch64 specific therefore I think convention is >>>> that it should go into a generic directory. >>> >>> Ok, I'll put it in gcc.dg/ >> >> OK with the test case moved. Thanks /Marcus >> > Thanks, but I'd like to do a slight respin. > The testcase is moved to gcc.dg but I also avoid creating the new > helper function and just do the SUBREG extraction once at the very end. > This makes the patch smaller. > > Since you're ok with the approach and this revision is logically equivalent, > I just need an ok from an arm perspective. OK, looks good to me and assuming you've tested this with a regression test run targeting cortex-a53 to stress the function ;) regards Ramana > > Thanks, > Kyrill > > 2015-10-29 Kyrylo Tkachov <kyrylo.tkachov@arm.com> > > PR target/68088 > * config/arm/aarch-common.c (aarch_accumulator_forwarding): Strip > subregs from accumulator and make sure it's a register. > > 2015-10-29 Kyrylo Tkachov <kyrylo.tkachov@arm.com> > > PR target/68088 > * gcc.dg/pr68088_1.c: New test. >
commit aa4df340968b330544edbbb8ea706f2d56011381
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date: Tue Oct 27 11:42:19 2015 +0000
[ARM/AArch64] PR 68088: Fix RTL checking ICE due to subregs inside accumulator forwarding check
diff --git a/gcc/config/arm/aarch-common.c b/gcc/config/arm/aarch-common.c
index a940a02..e6668d5 100644
--- a/gcc/config/arm/aarch-common.c
+++ b/gcc/config/arm/aarch-common.c
@@ -460,6 +460,12 @@ aarch_accumulator_forwarding (rtx_insn *producer, rtx_insn *consumer)
return 0;
}
+ if (GET_CODE (accumulator) == SUBREG)
+ accumulator = SUBREG_REG (accumulator);
+
+ if (!REG_P (accumulator))
+ return 0;
+
return (REGNO (dest) == REGNO (accumulator));
}
diff --git a/gcc/testsuite/gcc.dg/pr68088_1.c b/gcc/testsuite/gcc.dg/pr68088_1.c
new file mode 100644
index 0000000..49c6aa1
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr68088_1.c
@@ -0,0 +1,15 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+void bar (unsigned long);
+
+void
+foo (unsigned long aul, unsigned m, unsigned i)
+{
+ while (1)
+ {
+ aul += i;
+ i = aul % m;
+ bar (aul);
+ }
+}